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
@ValidatedFor annotation support for validation groups #137
Conversation
…e to new restx-validation module. [BREAKING] It means that to activate bean validation, you will need to explicitely add a dependency on restx-validation module on your project. Startup time on project not using restx-validation will then be improved (no hibernate validator will be loaded) for ~250ms
…"hibernate.validator") [BREAKING]
…, in order to make interpolation work in validation error messages (like @SiZe)
validation groups which will be used during bean validation
…date POJO against bean validation groups
…lidation validator
On another topic, I'm thinking about generating I'm just wondering if I should make this as a default behaviour (thus breaking backward compatibility) or enabling it only when I'm not a big fan of the latter since we would have a different behaviour between body and query parameter validation (the first one not requiring |
c49578f
to
710f27e
Compare
Hi, Sounds nice overall! For the problem of using the
I've tried calling it like this:
and it seems to work as expected, tests are passing. Now about extracting it in a separate module, indeed the module is very simple, but as you say it makes it more pluggable, so I'm ok for it. I'd just add a Last, about validating query parameters, why not now that they also use |
And about "tuning classes for hibernate validator performance at startup (even if nowadays, I didn't found such sort of configuration classes)" the call to |
…datedFor.value() since in most of cases, plain Class instance won't be available yet at annotation processing time Note that current impl has a drawback by relying on com.sun.tools.javac.code hidden package
…ation() call if needed
Just made some benchmarking with JMH about the Here is a tldr; of the benchmark results :
Full log can be seen in this gist, and benchmark sources can be seen here. My thoughts about this benchmarking results :
I tested in real life the bean validation behaviour to see how it behaves on hot reload/compile (my comments in the logs below, starting with
=> We can make 2 conclusions here :
Do you agree with these assumptions ? |
…to avoid some backward incompatibilities on existing generated code
a3d9a09
to
8be4965
Compare
b960e7d
to
6bff1cc
Compare
Thanks for this thorough analysis, and I agree with your conclusions. Having a separate issue for hot reload is fine, it's not due to the changes introduced here and can be addressed separately. BTW for using classes on |
Yup no problems with your implementation thanks for it. I'll implement |
Perfect, thanks. Xavier
|
I just started to implement Particularly when looking at RestxAnnotationProcessor which seems to only handle I'm wondering if I'm looking at the right place you expected me to generate
The use case I see (and use on a regular basis) for POJOs in query parameters is search criteria.
.. but it doesn't seem to be supported yet by restx. If we need to support this prior to calling |
The use of Still the usage is limited, it only convert one parameter at a time, and doesn't support Supporting your example would really be interesting, but it requires some changes in the API: currently the But that would better go in a separate issue / PR. WDYT? |
Merged through a516b4e |
Proposal to support bean validation groups during validation.
I'm not really satisfied by the current implementation and would like to open the discussion about it.
At first, I was planning to implement usage below :
with group declaration :
But I faced some issues at annotation processing time when retrieving
@ValidatedFor.value()
classes : didn't completely understood why, but some classes (either inrestx-core
or insamplest
) were not accessible.You can see some testing I made in commit 7f88a79 (from branch validations-validatedfor-with-class, some github comments describe what's going weird)
Thus, I had to fallback from
Class[]
toString[]
in order to directly provide validation group FQN used for source generation.Which makes declaration look like :
and group declaration :
If you see any workaround for this issue, I'm opened to discuss it.
Another way would be to rely on reflection but I know this is something we should avoid, especially for such trivial things.