Retrieve the no-argument constructor#1690
Conversation
…erlessPublicConstructor This fixes issue datafaker-net#1689 where the no-argument constructor is not always the one you get
src/main/java/net/datafaker/transformations/JavaObjectTransformer.java
Outdated
Show resolved
Hide resolved
asolntsev
changed the title
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1690 +/- ##
============================================
+ Coverage 92.28% 92.30% +0.02%
- Complexity 3410 3416 +6
============================================
Files 334 334
Lines 6752 6760 +8
Branches 670 671 +1
============================================
+ Hits 6231 6240 +9
+ Misses 355 354 -1
Partials 166 166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that object instantiation uses the parameterless public constructor when available, fixing cases where an arbitrary constructor was previously selected.
- Replace selection of the first declared constructor with a lookup for the parameterless public constructor.
- Add a helper method getParameterlessPublicConstructor and refactor hasParameterlessPublicConstructor to use it.
- Throw a RuntimeException when no parameterless public constructor exists.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return Arrays.stream(clazz.getConstructors()) | ||
| .filter(constructor -> constructor.getParameterCount() == 0) | ||
| .findFirst(); |
There was a problem hiding this comment.
Use the dedicated reflection API to retrieve the public no-arg constructor directly: clazz.getConstructor() is clearer and avoids array streaming. Suggested implementation:
private Optional<Constructor> getParameterlessPublicConstructor(Class clazz) {
try {
return Optional.of(clazz.getConstructor());
} catch (NoSuchMethodException e) {
return Optional.empty();
}
}
| return Arrays.stream(clazz.getConstructors()) | |
| .filter(constructor -> constructor.getParameterCount() == 0) | |
| .findFirst(); | |
| try { | |
| return Optional.of(clazz.getConstructor()); | |
| } catch (NoSuchMethodException e) { | |
| return Optional.empty(); | |
| } |
| if (primaryConstructor != null) { | ||
| result = primaryConstructor.newInstance(); | ||
| } else { | ||
| throw new RuntimeException("Failed to locate a parameterless public constructor for class " + clazz.getName()); |
There was a problem hiding this comment.
Throwing a generic RuntimeException obscures intent; consider using IllegalStateException (or wrapping NoSuchMethodException if switching to clazz.getConstructor()) to more precisely convey the failure to obtain a required public no-arg constructor.
| throw new RuntimeException("Failed to locate a parameterless public constructor for class " + clazz.getName()); | |
| throw new IllegalStateException("Failed to locate a parameterless public constructor for class " + clazz.getName()); |
| try { | ||
| Constructor<?> primaryConstructor = CLASS2CONSTRUCTOR.computeIfAbsent(clazz, c -> c.getDeclaredConstructors()[0]); | ||
| result = primaryConstructor.newInstance(); | ||
| Constructor<?> primaryConstructor = CLASS2CONSTRUCTOR.computeIfAbsent(clazz, c -> getParameterlessPublicConstructor(c).orElse(null)); |
There was a problem hiding this comment.
Returning null from computeIfAbsent means classes without a public no-arg constructor are never cached (causing repeated lookups). Consider caching the negative result, e.g., by changing the cache to Map<Class, Optional>> and storing Optional.empty(), or by using a separate negative cache.
|
Would it be possible to add a few testcases which demonstrate some scenarios? (like only a no arg (private|package|public) constructor, only a constructor with parameters, a class with both a no-arg and parameter constructor, etc?) |
Add a test to cover the scenarios where the no-arg constructor is public, protected and private Improve the code slightly that finds tries to find an alternative constructor. This could be improved further to find one that matches the schema. Currently it just finds the first public constructor in the array returned by clazz.getConstructors()
|
I have added some tests and filtered for public constructors only |
|
Looks great. If you're happy with it, happy to merge it! Thanks for your work on this! |
…using a method called
getParameterlessPublicConstructor.This fixes #1689 where the no-argument constructor is not always the one you get