Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: java validation support on supplier method #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/FieldScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.lang.reflect.AnnotatedParameterizedType;
import java.lang.reflect.AnnotatedType;
import java.lang.reflect.Field;
import java.util.function.Supplier;
import java.util.stream.Stream;

/**
Expand Down Expand Up @@ -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)) {
Copy link
Member

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.

Copy link
Member

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.

ResolvedMethod[] methods = getContext().resolveWithMembers(this.getMember().getType()).getMemberMethods();
return Stream.of(methods)
.filter(method -> method.getName().equals("get"))
Comment on lines +117 to +119
Copy link
Member

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?

.findFirst()
.map(method -> this.getContext().createMethodScope(method, this.getDeclaringTypeMembers()))
.orElse(null);
}
String capitalisedFieldName = this.getDeclaredName().substring(0, 1).toUpperCase() + this.getDeclaredName().substring(1);
String getterName1 = "get" + capitalisedFieldName;
String getterName2 = "is" + capitalisedFieldName;
Expand Down
16 changes: 12 additions & 4 deletions jsonschema-generator/src/main/java/com/github/victools/jsonschema/generator/MemberScope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2019 VicTools.
* Copyright 2020 VicTools & Sascha Kohlmann
Copy link
Member

@CarstenWickner CarstenWickner Sep 12, 2022 •
edited
Loading

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.

Suggested change
* Copyright 2020 VicTools & Sascha Kohlmann
* Copyright 2019 VicTools.

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -25,6 +25,7 @@
import java.lang.annotation.Annotation;
import java.lang.reflect.Member;
import java.util.Optional;
import java.util.function.Supplier;

/**
* Representation of a single introspected field or method.
Expand Down Expand Up @@ -321,14 +322,21 @@ public <A extends Annotation> A getContainerItemAnnotationConsideringFieldAndGet
if (this.isFakeContainerItemScope()) {
return this.getContainerItemAnnotationConsideringFieldAndGetter(annotationClass);
}
if (this.getOverriddenType() != null
&& this.getDeclaredType().getErasedType() == Optional.class
&& this.getOverriddenType().getErasedType() == this.getDeclaredType().getTypeParameters().get(0).getErasedType()) {
if (this.getOverriddenType() != null && (isSupplier() || isOptional())) {
return this.getContainerItemAnnotationConsideringFieldAndGetter(annotationClass);
}
return null;
}

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();
}
Comment on lines +331 to +338
Copy link
Member

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.


/**
* Returns the name to be used to reference this member in its parent's "properties".
*
Expand Down
27 changes: 26 additions & 1 deletion ...c/test/java/com/github/victools/jsonschema/module/jakarta/validation/IntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,15 @@
import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Null;
import jakarta.validation.constraints.Pattern;
import jakarta.validation.constraints.PositiveOrZero;
import jakarta.validation.constraints.Size;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.Optional;
import java.util.Scanner;
import java.util.function.Supplier;
import org.junit.jupiter.api.Test;
import org.skyscreamer.jsonassert.JSONAssert;
import org.skyscreamer.jsonassert.JSONCompareMode;
Expand All @@ -63,6 +65,7 @@ public void testIntegration() throws Exception {
JakartaValidationOption.INCLUDE_PATTERN_EXPRESSIONS);
SchemaGeneratorConfig config = new SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2019_09, OptionPreset.PLAIN_JSON)
.with(Option.NULLABLE_ARRAY_ITEMS_ALLOWED)
.with(Option.FLATTENED_SUPPLIERS)
.with(module)
.build();
SchemaGenerator generator = new SchemaGenerator(config);
Expand All @@ -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())) {
Copy link
Member

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.

while (scanner.hasNext()) {
stringBuilder.append(scanner.nextLine()).append('\n');
}
Expand Down Expand Up @@ -128,5 +131,27 @@ static class TestClass {
@DecimalMin(value = "0", inclusive = false)
@DecimalMax(value = "1", inclusive = false)
public double exclusiveRangeDouble;

@PositiveOrZero
public IntegrationTest.TestSupplier supplierPositiveOrZero;

public TestSupplierWithAnnotation supplierWithAnnotationPositiveOrZero;
}
Copy link
Member

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;
}


static class TestSupplier implements Supplier<Integer> {

@Override
public Integer get() {
return 0;
}
}

static class TestSupplierWithAnnotation implements Supplier<Integer> {

@Override
@PositiveOrZero
public Integer get() {
return 0;
}
}
}
8 changes: 8 additions & 0 deletions ...ces/com/github/victools/jsonschema/module/jakarta/validation/integration-test-result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@
"type": "string",
"minLength": 5,
"maxLength": 12
},
"supplierPositiveOrZero": {
"type": "integer",
"minimum": 0
},
"supplierWithAnnotationPositiveOrZero": {
"type": "integer",
"minimum": 0
}
},
"required": ["notBlankText", "notEmptyList", "notEmptyPatternText", "notNullEmail", "notNullList"]
Expand Down
24 changes: 24 additions & 0 deletions ...src/test/java/com/github/victools/jsonschema/module/javax/validation/IntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.List;
import java.util.Optional;
import java.util.Scanner;
import java.util.function.Supplier;
import javax.validation.constraints.DecimalMax;
import javax.validation.constraints.DecimalMin;
import javax.validation.constraints.Email;
Expand All @@ -39,6 +40,7 @@
import javax.validation.constraints.NotNull;
import javax.validation.constraints.Null;
import javax.validation.constraints.Pattern;
import javax.validation.constraints.PositiveOrZero;
import javax.validation.constraints.Size;
import org.junit.jupiter.api.Test;
import org.skyscreamer.jsonassert.JSONAssert;
Expand Down Expand Up @@ -128,5 +130,27 @@ static class TestClass {
@DecimalMin(value = "0", inclusive = false)
@DecimalMax(value = "1", inclusive = false)
public double exclusiveRangeDouble;

@PositiveOrZero
public IntegrationTest.TestSupplier supplierPositiveOrZero;

public TestSupplierWithAnnotation supplierWithAnnotationPositiveOrZero;
}

static class TestSupplier implements Supplier<Integer> {

@Override
public Integer get() {
return 0;
}
}

static class TestSupplierWithAnnotation implements Supplier<Integer> {

@Override
@PositiveOrZero
public Integer get() {
return 0;
}
}
}
8 changes: 8 additions & 0 deletions ...urces/com/github/victools/jsonschema/module/javax/validation/integration-test-result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@
"type": "string",
"minLength": 5,
"maxLength": 12
},
"supplierPositiveOrZero": {
"type": "integer",
"minimum": 0
},
"supplierWithAnnotationPositiveOrZero": {
"type": "integer",
"minimum": 0
}
},
"required": ["notBlankText", "notEmptyList", "notEmptyPatternText", "notNullEmail", "notNullList"]
Expand Down