Skip to content

fix: Prefer accessible interface methods over internal implementation classes in method resolution#485

Merged
lukaszlenart merged 12 commits intomainfrom
claude/investigate-issue-286-011CUu6zHFNnjsKYYHoJb97W
Nov 29, 2025
Merged

fix: Prefer accessible interface methods over internal implementation classes in method resolution#485
lukaszlenart merged 12 commits intomainfrom
claude/investigate-issue-286-011CUu6zHFNnjsKYYHoJb97W

Conversation

@lukaszlenart
Copy link
Copy Markdown
Collaborator

Description

Fixes #286 - OGNL choosing method on unexported class rather than exported interface

This PR enhances OGNL's method resolution logic to prefer accessible methods from public interfaces over internal implementation classes, preventing IllegalAccessException errors when working with Java 9+ module system restrictions.

Problem

When calling methods on objects like X509Certificate array elements, OGNL was selecting methods from internal JDK implementation classes (e.g., sun.security.x509.X509CertImpl) instead of the public interface (java.security.cert.X509Certificate). This caused IllegalAccessException because the module system doesn't export internal packages:

…mentation classes (#286)

This fix addresses issue #286 where OGNL was selecting methods from
internal JDK implementation classes (like sun.security.x509.X509CertImpl)
instead of their public interfaces (like java.security.cert.X509Certificate),
causing IllegalAccessException due to Java module system restrictions.

Changes:
- Added isLikelyAccessible() helper method to check if a class is likely
  accessible considering the Java module system
- Enhanced findBestMethod() tie-breaking logic to prefer methods in this order:
  1. Accessible classes over inaccessible ones
  2. Interface methods over class methods (better encapsulation)
  3. Public classes over non-public classes
- Added comprehensive test cases in Issue286Test.java

The fix uses the Java Module API to check if packages are exported and
includes heuristics for known internal packages (sun.*, jdk.internal.*, etc).
This ensures OGNL selects the most accessible method variant when multiple
methods with the same signature exist in the class hierarchy.

Fixes #286
@lukaszlenart lukaszlenart added java Pull requests that update Java code improvement labels Nov 7, 2025
@lukaszlenart lukaszlenart added this to the 3.5.0 milestone Nov 7, 2025
- Added 8 new test cases covering edge cases and different scenarios
- Tests now include: varargs, overloaded methods, abstract classes, generics,
  collections, interface inheritance, factory patterns, null handling, and primitives
- Added assertions on actual values, not just non-null checks
- Comprehensive coverage of method resolution tie-breaking logic
- Tests exercise different code paths in isLikelyAccessible() and findBestMethod()
- Focus on the core issue: prefer accessible over inaccessible methods
- Remove aggressive interface-over-class preference that could break existing tests
- Keep original tie-breaking logic (prefer public classes) when both methods
  have the same accessibility level
- Add null/empty package name check for safety
- This should fix #286 while maintaining backward compatibility
- Removed interfaceMethodWithVarargs test that was failing due to OGNL varargs syntax limitations
- Removed unused VarargsInterface and VarargsImplementation classes
- Cleaned up unused imports (SSLContext, certificates, etc.)
- Simplified x509CertificateMethodResolution test comment
- Still have 14 comprehensive tests covering the fix
- Added tests with actual JDK classes (HashMap, ArrayList, StringBuilder)
- Added test for package-private class to test accessibility logic
- Added tests for multiple interface inheritance
- Added tests with java.util classes to exercise different code paths
- Tests now cover Map vs HashMap, List vs ArrayList scenarios
- Total of 19 test cases covering various method resolution scenarios
- Added tests for CharSequence and Comparable interfaces
- Added test to verify methods from both interfaces and classes are found
- Added HashMap/Map test to verify interface vs concrete class handling
- Tests cover various JDK interface scenarios
- Now 22 test cases total providing comprehensive coverage of accessible code paths

Note: Tests for inaccessible internal classes (sun.*, com.sun.*, etc.) cannot
be easily added as we cannot instantiate those classes in unit tests. The
inaccessibility detection logic is designed to handle real-world scenarios
where OGNL encounters actual internal JDK implementation classes.
- Changed isLikelyAccessible() from private to package-private for testing
- Added OgnlRuntimeAccessibilityTest with comprehensive coverage
- Tests verify interface detection, standard JDK classes, and internal package detection
- Tests attempt to load actual sun.* and com.sun.* classes to verify inaccessibility detection
- Handles ClassNotFoundException gracefully for classes that may not be available
- Significantly improves code coverage by testing the method directly
- Created sun.test.SimulatedInternalClass to simulate sun.* internal classes
- Created sun.test.PublicTestInterface for testing interface preference
- Created com.sun.test.AnotherInternalClass to test com.sun.* detection
- Added tests that verify classes in sun.* and com.sun.* are detected as inaccessible
- Added simulatedInternalClassVsInterface test to exercise the key code path
- Added simulatedInternalClassIsDetectedAsInaccessible for direct verification
- These simulated classes allow coverage of the (!currentAccessible && newAccessible) branch
- Updated OgnlRuntimeAccessibilityTest with additional test cases

This significantly improves coverage by allowing us to test the actual
accessibility preference logic without needing real internal JDK classes.
Issue286Test is in package ognl.test and cannot access the package-private
isLikelyAccessible() method in package ognl. Direct testing of that method
is already covered in OgnlRuntimeAccessibilityTest which is in the correct
package.

The simulatedInternalClassVsInterface() test still provides end-to-end
coverage of the fix by using classes in sun.test package.
- Removed defaultPackageClassIsAccessible() which had no assertions
- Removed jdkInternalPackageWouldBeInaccessible() which only tested string operations
- These tests provided no actual coverage or value
- All meaningful test cases are already covered by other tests
@lukaszlenart lukaszlenart removed the java Pull requests that update Java code label Nov 9, 2025
@lukaszlenart lukaszlenart marked this pull request as ready for review November 9, 2025 17:53
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Nov 9, 2025

@lukaszlenart lukaszlenart merged commit 074b980 into main Nov 29, 2025
5 checks passed
@lukaszlenart lukaszlenart deleted the claude/investigate-issue-286-011CUu6zHFNnjsKYYHoJb97W branch November 29, 2025 12:03
lukaszlenart added a commit that referenced this pull request Nov 29, 2025
…backport #485 to ognl-3-4-x)

Fixes #286 - OGNL choosing method on unexported class rather than exported interface

This backport adapts PR #485 from main branch for Java 8 compatibility:
- Added isLikelyAccessible() helper method (Java 8 compatible)
- Enhanced findBestMethod() tie-breaking logic to prefer accessible methods
- Added comprehensive test coverage (36 new tests: 10 unit + 26 integration)

Java 8 adaptations:
- Use clazz.getPackage().getName() instead of getPackageName()
- Remove Module API checks (Java 9+ only)
- Use Arrays.asList() instead of List.of() in tests
- Convert from JUnit 5 to JUnit 4
- Swap assertion parameter order (message first in JUnit 4)
- Replace assertDoesNotThrow() with try-catch pattern

Test results: 993 tests passed (957 existing + 36 new), 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
lukaszlenart added a commit that referenced this pull request Nov 29, 2025
…backport #485 to ognl-3-4-x)

Fixes #286 - OGNL choosing method on unexported class rather than exported interface

This backport adapts PR #485 from main branch for Java 8 compatibility:
- Added isLikelyAccessible() helper method (Java 8 compatible)
- Enhanced findBestMethod() tie-breaking logic to prefer accessible methods
- Added comprehensive test coverage (36 new tests: 10 unit + 26 integration)

Java 8 adaptations:
- Use clazz.getPackage().getName() instead of getPackageName()
- Remove Module API checks (Java 9+ only)
- Use Arrays.asList() instead of List.of() in tests
- Convert from JUnit 5 to JUnit 4
- Swap assertion parameter order (message first in JUnit 4)
- Replace assertDoesNotThrow() with try-catch pattern

Test results: 993 tests passed (957 existing + 36 new), 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
lukaszlenart added a commit that referenced this pull request Nov 29, 2025
…backport #485 to ognl-3-4-x)

Fixes #286 - OGNL choosing method on unexported class rather than exported interface

This backport adapts PR #485 from main branch for Java 8 compatibility:
- Added isLikelyAccessible() helper method (Java 8 compatible)
- Enhanced findBestMethod() tie-breaking logic to prefer accessible methods
- Added comprehensive test coverage (36 new tests: 10 unit + 26 integration)

Java 8 adaptations:
- Use clazz.getPackage().getName() instead of getPackageName()
- Remove Module API checks (Java 9+ only)
- Use Arrays.asList() instead of List.of() in tests
- Convert from JUnit 5 to JUnit 4
- Swap assertion parameter order (message first in JUnit 4)
- Replace assertDoesNotThrow() with try-catch pattern

Test results: 993 tests passed (957 existing + 36 new), 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
lukaszlenart added a commit that referenced this pull request Nov 29, 2025
…backport #485 to ognl-3-4-x) (#503)

Fixes #286 - OGNL choosing method on unexported class rather than exported interface

This backport adapts PR #485 from main branch for Java 8 compatibility:
- Added isLikelyAccessible() helper method (Java 8 compatible)
- Enhanced findBestMethod() tie-breaking logic to prefer accessible methods
- Added comprehensive test coverage (36 new tests: 10 unit + 26 integration)

Java 8 adaptations:
- Use clazz.getPackage().getName() instead of getPackageName()
- Remove Module API checks (Java 9+ only)
- Use Arrays.asList() instead of List.of() in tests
- Convert from JUnit 5 to JUnit 4
- Swap assertion parameter order (message first in JUnit 4)
- Replace assertDoesNotThrow() with try-catch pattern

Test results: 993 tests passed (957 existing + 36 new), 0 failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ognl choosing method on unexported class rather than exported interface

2 participants