-
Notifications
You must be signed in to change notification settings - Fork 64
feat: java validation support on supplier method #282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @berlam,
Thank you for your input. I've got a few comments/questions but overall appreciate the extended support for the Supplier
interface.
Although I'm wondering whether the support could be further generalized to consider all @FunctionalInterface
annotated types, where the single overridden method expects no arguments and is not void
. But explicitly supporting `Supplier´ and its subtypes may be good enough already.
@@ -112,6 +113,14 @@ public MethodScope findGetter() { | |||
* @return public getter from within the field's declaring class | |||
*/ | |||
private MethodScope doFindGetter() { | |||
if (this.getMember().getType().isInstanceOf(Supplier.class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: While this achieves the specific use-case you're after, it constitutes a breaking change to this library. I suggest making this dependent on the Option.FLATTENED_SUPPLIERS
at the least.
Plus: the supplier's functional interface implementation is not technically a field's getter. The actual getter could have annotations that may be relevant to consider too, which would get lost here now, wouldn't they?
I'm not sure this is the right place for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, this should be handled within the FlattenedWrapperModule
. May need to indicate somehow which method is deemed relevant for this.
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2019 VicTools. | |||
* Copyright 2020 VicTools & Sascha Kohlmann |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the copyright?
This class was originally created in 2019 and while Sascha Kohlmann has provided valuable feedback on multiple occasions and ported the Javax module into the Jakarta module, I don't remember him touching this class here at any time.
* Copyright 2020 VicTools & Sascha Kohlmann | |
* Copyright 2019 VicTools. |
private boolean isSupplier() { | ||
return this.member.getType().isInstanceOf(Supplier.class); | ||
} | ||
|
||
private boolean isOptional() { | ||
return this.getDeclaredType().getErasedType() == Optional.class | ||
&& this.getOverriddenType().getErasedType() == this.getDeclaredType().getTypeParameters().get(0).getErasedType(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: While I agree with adding the support for the Supplier
by default and appreciate the improved readability by extracting these two checks into dedicated methods, I'm not convinced by the method names. The MemberScope
is neither a Supplier
nor an Optional
. It is the member's type that can be either of these two.
The method name should reflect that more clearly, even if that means it becoming more verbose.
Additionally, I'd also include the check of the supplied type against the member's overridden one – same as for Optional
– and not indiscriminately consider all Supplier
subtypes.
@@ -77,7 +80,7 @@ private static String loadResource(String resourcePath) throws IOException { | |||
StringBuilder stringBuilder = new StringBuilder(); | |||
try (InputStream inputStream = IntegrationTest.class | |||
.getResourceAsStream(resourcePath); | |||
Scanner scanner = new Scanner(inputStream, StandardCharsets.UTF_8.name())) { | |||
Scanner scanner = new Scanner(inputStream, StandardCharsets.UTF_8.name())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Let's avoid unnecessary whitespace changes in pull requests for features.
public IntegrationTest.TestSupplier supplierPositiveOrZero; | ||
|
||
public TestSupplierWithAnnotation supplierWithAnnotationPositiveOrZero; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: The tests should also consider a field with type Supplier<@PositiveOrZero Integer>
.
Also a scenario in which the actual getter method holds the annotation, e.g.
@PositiveOrZero
public Supplier<Integer> getSupplierPositiveOrZeroWithGetter() {
return this.supplierPositiveOrzeroWithGetter;
}
ResolvedMethod[] methods = getContext().resolveWithMembers(this.getMember().getType()).getMemberMethods(); | ||
return Stream.of(methods) | ||
.filter(method -> method.getName().equals("get")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Collecting all methods only to then filter by name seems excessive. Isn't there an easy way to just look-up the one method we're interested in?
You can now add annotations on the supplier method. This is useful for value objects, which consist of one value which resides in a base class.