jooby-project / jooby

The modular web framework for Java and Kotlin
https://jooby.io
Apache License 2.0
1.71k stars 197 forks source link

Feat/jooby hibernate validator #3508

Closed kliushnichenko closed 1 month ago

kliushnichenko commented 2 months ago

Still need some docs, tests, cleanup, but the basic logic seems done

agentgt commented 2 months ago

@kliushnichenko @jknack

I'm not sure I like the name of jooby-validation if it is going to have a hard dependency on jakarta.validation. Is the plan to make the validation pluggable in that module and it is just jakarta for now?

Otherwise I think it should be jooby-jakarta-validation.

Also the jooby-apt module should not have any dependency on basically anything ideally. I haven't code dived to see why you made it that way. (you don't need the actual classes you depend on with generated code)

EDIT actually maybe you changed it. When you get a chance @kliushnichenko maybe squash the commits to one or two?

SentryMan commented 2 months ago

The way I've done it with other libs I've worked was to have an extensible Validator interface which clients can extend to provide custom validation. Example hibernate impl and avaje validation impl . Perhaps you guys could try something similar if you want to eliminate the dependencies

jknack commented 2 months ago

@SentryMan thank you. I feel here isn't necessary to add another abstraction over the jakarta one. Leaving jakarta-validator outside of core is enough.

kliushnichenko commented 2 months ago

Updated. Pls verify the changes and I'll proceed with the documentation

kliushnichenko commented 2 months ago

@SentryMan could you pls shed some light on avaje-validator. I thought it is fully compliant with jakarta-validation-api. But I can't find how I can get the instance of jakarta.validation.Validator baked by avaje-validator. All the entry points are available only at your custom abstraction io.avaje.validation.Validator.

Is it a trade-off of a reflection-free solution or maybe you can provide us jakarta-compliant entry point in the future release?

SentryMan commented 2 months ago

Is it a trade-off of a reflection-free solution

Jakarta validation is mainly reflection-based so we couldn't fully implement its interfaces. Hence why we took a page from spring in our http solution by defining another abstraction so we could support both jakarta and avaje validation

kliushnichenko commented 2 months ago

Getting back to avaje-validator support...

As SentryMan clarified, there is no way to smoothly support both (hbv/avaje) relying solely on jakarta.validation.Validator So, my initial plan didn't quite work out 😄

Options that are running through my mind in this regard:

  1. Write a custom implementation of the jakarta.validation.Validator that will trigger io.avaje.validation.Validator under the hood Concerns:
    way too odd. Everything except one method validate() will be stubbed as Unsupported.
    Also, it will hide the rest of avaje-validator options, not ok.

  2. Each module can implement its own helper for validation. In jooby-apt pick the one that is available in classpath. Concerns:
    Complicates jooby-apt, probably duplicates some logic in two places, error-prone, complicates further support.

  3. Introduce one more abstraction, as SentryMan pointed out, to cover jooby-apt logic over a single interface. Concerns: plus one more abstraction, a bit more cumbersome, but, seems the most acceptable compromise

@jknack what's your take on it? maybe some other options?

jknack commented 2 months ago

Seems 3 it is OK. jooby-apt should call io.jooby.validation.BeanValidator and extra logic goes here (jooby-jakarta-validation) and probably on the jooby-avaje-validator module.

So jooby-apt will do what is doing here, wrap the call around BeanValidator

kliushnichenko commented 1 month ago

Updated.

Please note the following:

Naming is getting tough with a lot of Validator abstractions, any suggestions are welcomed.
@jknack pls confirm we are good with this solution and can move on

kliushnichenko commented 1 month ago

minor improvements, javadoc, asciidoc has been added. Ready to final review/merge.
avaje-validator-module I'll send in a separate PR

kliushnichenko commented 1 month ago

Yes, we can. Just a minor thing, master and my branch are under 3.2.10-SNAPSHOT, but I see 3.3.0 has already been released, is it expected, or we need to update both to 3.3.1-SNAPSHOT?

jknack commented 1 month ago

oh yea, let me push the version bump

jknack commented 1 month ago

@kliushnichenko done

kliushnichenko commented 1 month ago

@jknack updated, feel free to merge