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.

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

Gather bean validation violations in a single field error #2201

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

dpolysiou
Copy link
Contributor

Change BeanValidationError path to go from root to field, in accordance with the spec.

@jmartisk
Copy link
Member

jmartisk commented Oct 1, 2024

@dpolysiou could you please rebase, specifically we need the branch to contain a076d7d (which I just merged a few minutes ago) to have the CI tests work properly

EDIT: now also 1dd98ec :)

Fix BeanValidationError path according to the spec
@dpolysiou dpolysiou force-pushed the 2198-bean-validation-field-error branch from 793d9c6 to e255e76 Compare October 1, 2024 08:20
@dpolysiou
Copy link
Contributor Author

I saw one test in the wildfly-graphql repository that breaks (specifically, this one), because it has an assertion for the existing BeanValidationError extensions. It's a small change, but I don't know how the changes between these repositories should be coordinated.

@jmartisk
Copy link
Member

jmartisk commented Oct 1, 2024

If you're willing to help us fix the test too, then this is how we normally do it:

  • I create a special branch in the wildfly-graphql-feature-pack repo named smallrye-graphql-2.11 or so
  • That branch will capture all the changes that we plan to apply along with upgrading to SR-GQL 2.11 (so, fixing the test..)
  • We temporarily change the SR-GQL CI here: https://github.com/smallrye/smallrye-graphql/blob/main/.github/workflows/build.yml#L61 to run against that temporary branch instead of main
  • After the upgrade, we switch the CI back to wildfly-graphql-feature-pack's main

So if you want to help with that, I can create the smallrye-graphql-2.11 branch in wildfly-graphql-feature-pack and you can send a PR to it. But no problem if you want to let us handle it ourselves.

@jmartisk
Copy link
Member

jmartisk commented Oct 1, 2024

Ah, GraphQLBeanValidationTest in Quarkus will have the exact same problem. I already have a Quarkus branch (https://github.com/jmartisk/quarkus/tree/smallrye-graphql-2.11) where this would have to be fixed :)

Do you want me to handle the WildFly and Quarkus tests?

@dpolysiou
Copy link
Contributor Author

Thank you, I will try to make a PR to these branches as soon as possible.

@jmartisk jmartisk requested a review from a team as a code owner October 2, 2024 09:14
@jmartisk jmartisk force-pushed the 2198-bean-validation-field-error branch from 0a909b1 to 04c5dd1 Compare October 2, 2024 09:23
@jmartisk
Copy link
Member

jmartisk commented Oct 2, 2024

Thanks for all the PRs!
I've updated the CI to run against the temporary branches... Hopefully it will all pass :)

@jmartisk jmartisk force-pushed the 2198-bean-validation-field-error branch from 04c5dd1 to 58a6ae1 Compare October 2, 2024 09:39
@jmartisk jmartisk merged commit 05f8618 into smallrye:main Oct 2, 2024
5 checks passed
@jmartisk
Copy link
Member

jmartisk commented Oct 2, 2024

It worked, thanks a lot!
I'll take care of updating the wildlfly and quarkus repositories after releasing smallrye-graphql 2.11.

@schulzp
Copy link

schulzp commented Nov 2, 2024

@dpolysiou - This fails now, if the invalidValue is null, since Map.of does not allow null values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants