Enhance constructor invocation handling with improved matching logic#262
Conversation
There was a problem hiding this comment.
Pull request overview
Improves the core server reflection proxy (SurfInvocationHandlerJava) constructor invocation path by introducing a dedicated ConstructorInvokable and adding more flexible constructor resolution when runtime argument types don’t exactly match the proxy method’s declared parameter types.
Changes:
- Replaced constructor handling from a pre-bound
HandleInvokableto a newConstructorInvokable. - Added constructor selection logic that scores candidate constructors using declared parameter types plus runtime arguments.
- Bumped project version to
1.21.11-2.70.3.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| surf-api-core/surf-api-core-server/src/main/java/dev/slne/surf/surfapi/core/server/impl/reflection/SurfInvocationHandlerJava.java | Adds constructor matching/scoring utilities and a new ConstructorInvokable path for constructor calls. |
| gradle.properties | Version bump from 1.21.11-2.70.2 to 1.21.11-2.70.3. |
| public @Nullable Object invoke(final Object[] args) throws Throwable { | ||
| final Constructor<?> constructor = findConstructor(proxiedClass, method, args); | ||
| final MethodHandle handle = normalizeMethodHandleType( | ||
| privateLookup.unreflectConstructor(constructor) | ||
| ); | ||
|
|
||
| if (handle.type().parameterCount() > 0) { | ||
| return handle.invokeExact(args); | ||
| } else { | ||
| return handle.invokeExact(); | ||
| } |
There was a problem hiding this comment.
ConstructorInvokable resolves the constructor and creates a new MethodHandle on every invocation. This adds repeated reflective lookup/unreflect cost on hot paths. Consider caching the resolved constructor/normalized MethodHandle (e.g., keyed by runtime argument types) inside ConstructorInvokable so repeated calls don’t redo reflection work.
| if (target.equals(runtime)) { | ||
| score += 100; | ||
| } else if (target.equals(declared)) { | ||
| score += 50; | ||
| } else if (target.isAssignableFrom(declared)) { | ||
| score += 25; | ||
| } else { | ||
| score += 10; | ||
| } |
There was a problem hiding this comment.
The constructor scoring currently prefers matching the proxy method’s declared parameter type over choosing the most specific overload for the runtime argument type. For example, if the proxy declares Object but the runtime argument is Integer, this scoring can select an Object-parameter constructor over a Number-parameter constructor, which is usually a worse match. Consider incorporating “distance” from runtime type to target type (more specific = higher score) and using the declared type only as a tie-breaker; also consider detecting ties (equal bestScore) and failing with an ambiguity error since constructor iteration order is not guaranteed.
| ); | ||
| } | ||
|
|
||
| best.setAccessible(true); |
There was a problem hiding this comment.
findConstructor() uses best.setAccessible(true) while the exact-match path uses trySetAccessible(). setAccessible(true) can throw (e.g., InaccessibleObjectException / SecurityException) and would turn constructor resolution into a hard failure even when MethodHandles privateLookup access would otherwise work. Prefer trySetAccessible() (or avoid reflective accessibility changes entirely and rely on the Lookup) and, if access cannot be obtained, surface a clear error.
| best.setAccessible(true); | |
| try { | |
| if (!best.trySetAccessible()) { | |
| throw new NoSuchMethodException( | |
| "Constructor found for " + clazz.getName() | |
| + " with declared parameters " + Arrays.toString(declaredTypes) | |
| + " and runtime arguments " + runtimeTypesToString(args) | |
| + " but it is not accessible" | |
| ); | |
| } | |
| } catch (final SecurityException e) { | |
| final NoSuchMethodException ex = new NoSuchMethodException( | |
| "Constructor found for " + clazz.getName() | |
| + " with declared parameters " + Arrays.toString(declaredTypes) | |
| + " and runtime arguments " + runtimeTypesToString(args) | |
| + " but access was denied by security manager" | |
| ); | |
| ex.initCause(e); | |
| throw ex; | |
| } |
This pull request introduces significant improvements to the reflection-based method invocation logic, particularly around constructor handling in the
SurfInvocationHandlerJavaclass. The changes enhance the flexibility and robustness of constructor resolution, especially when parameter types at runtime may not exactly match the declared types. Additionally, a newConstructorInvokableimplementation is introduced to encapsulate this logic. The project version is also incremented.Reflection and Constructor Invocation Improvements
ConstructorInvokableimplementation to handle constructor invocations, replacing the previous use ofHandleInvokablefor constructors. This allows for more robust and flexible resolution of constructors at runtime. [1] [2] [3]findConstructormethod to select the best matching constructor based on both declared parameter types and actual runtime arguments. This includes a scoring system for matching constructors, handling of primitive types, and improved error reporting when no suitable constructor is found.scoreExecutableMatch), wrapping primitive types, and formatting runtime argument types for error messages.Project Version Update
gradle.propertiesfrom1.21.11-2.70.2to1.21.11-2.70.3.