Skip to content
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

Jump to bottom

hibernate validator: Remember annotated interface methods #5308

Merged
merged 2 commits into from Nov 12, 2019
Merged

hibernate validator: Remember annotated interface methods #5308

merged 2 commits into from Nov 12, 2019

Conversation

ebullient
Copy link
Contributor

@ebullient ebullient commented Nov 7, 2019 •
edited

resolves #1888

This includes #1913

aguibert
aguibert approved these changes Nov 7, 2019
View reviewed changes
...ent/src/main/java/io/quarkus/hibernate/validator/deployment/HibernateValidatorProcessor.java Outdated
Comment on lines 262 to 266
Set<String> methodSet = inheritedAnnotationsToBeValidated.get(clazz.name());
if (methodSet == null) {
methodSet = new HashSet<String>();
inheritedAnnotationsToBeValidated.put(clazz.name(), methodSet);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could collapse this down a bit using computeIfAbsent()

Copy link
Contributor Author

@ebullient ebullient Nov 7, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it isn't a concurrent set.. and my brain only remembered them there.

@gsmet gsmet self-assigned this Nov 7, 2019
@gsmet
Copy link
Member

gsmet commented Nov 7, 2019

I'm away for a few days. I'll have a look when I'll get back.

@geoand
Copy link
Contributor

geoand commented Nov 8, 2019

I'll also give it a look over the weekend but I'll leave the final approval to @gsmet since he's the sme on this

@geoand
Copy link
Contributor

geoand commented Nov 8, 2019

Nice job!

I do see however that the original issue mentions inherited methods from classes as well as interfaces and unless I misunderstood the changes, the class case is not handled?

@ebullient
Copy link
Contributor Author

Nice job!

I do see however that the original issue mentions inherited methods from classes as well as interfaces and unless I misunderstood the changes, the class case is not handled?

I will double check. I'll have to add an additional test, I think. I stuck with the testcase provided by @isikemre, which focused on interfaces.

@geoand
Copy link
Contributor

geoand commented Nov 8, 2019

Cool, thanks!

@ebullient ebullient mentioned this pull request Nov 11, 2019
Closed
@cescoffier cescoffier merged commit 8c589a1 into quarkusio:master Nov 12, 2019
@ebullient ebullient deleted the quarkus-1888 branch November 12, 2019 15:38
@cescoffier
Copy link
Member

it looks it introduces issues in other PR (CI builds), I'm going to revert it, and @gsmet will have another look

@cescoffier cescoffier mentioned this pull request Nov 13, 2019
Closed
@gsmet
Copy link
Member

gsmet commented Nov 13, 2019 •
edited 

ERROR] Tests run: 9, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 5.453 s <<< FAILURE! - in io.quarkus.it.hibernate.validator.HibernateValidatorFunctionalityTest
[ERROR] testInheritedImplementsConstraints  Time elapsed: 0.047 s  <<< FAILURE!
java.lang.AssertionError:
1 expectation failed.
Response body doesn't match expectation.
Expected: is "passed\nfailed: echoZipCode.arg0 (size must be between 5 and 5)"
  Actual: passed

@geoand
Copy link
Contributor

geoand commented Nov 13, 2019

So there is flaky test

@ebullient ebullient restored the quarkus-1888 branch November 13, 2019 16:06
@geoand
Copy link
Contributor

geoand commented Nov 13, 2019

Question: Did the test only fail in native mode?

@gsmet
Copy link
Member

gsmet commented Nov 13, 2019

The one I saw was in native.

@ebullient ebullient deleted the quarkus-1888 branch November 13, 2019 20:50
@gsmet gsmet added this to the 1.1.0 milestone Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cescoffier cescoffier cescoffier approved these changes

@aguibert aguibert aguibert approved these changes

Assignees

@gsmet gsmet

Labels
None yet
Projects
None yet
Milestone
1.1.0.CR1
Development

Successfully merging this pull request may close these issues.

Quarkus cannot use the hibernate-validator constraints in the inherited class/interface
5 participants
@ebullient @gsmet @geoand @cescoffier @aguibert