jooby-project / jooby

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

WIP: jooby-hbv module #3494

Closed kliushnichenko closed 3 weeks ago

kliushnichenko commented 1 month ago

Hi @jknack ,

I am wondering if you are up for reviving the jooby-hbv module 😊?

Apparently, it is not possible to do it the same way as in Jooby 1.x, since we don't have a chain of parsers anymore. So far, I haven't found a better option than to tweak a bit the core functionality of DefaultContext.decode(). As far as I can see this is the only place where we can access the already decoded object before running any client's application logic.

I don't really like mixing decoding and validation, but it seems to be the simplest way without fundamentally reshaping the core logic. Open to any suggestions and improvements.

In this PR you can find some basic sketch, pls let me know if it worth to continue.

jknack commented 1 month ago

Hi @kliushnichenko,

I noticed a few weeks ago that the module was missing... I thought a bit about it and here are my reason for not including it:

We could still write a module to publish Validator instances so then we can access to validator instance. It is a bit more useful for MVC routes if we can do things like:

 @POST("/create")
 public Bean create(@Valid Bean bean) {
    ...
 }

But here @Valid should be available to body, query, form, etc.. (not just body via decode)

Some rendering option for constraint exceptions could be useful too.

Thoughts?

kliushnichenko commented 1 month ago

The HBV API is pretty simple and straightforward to bootstrap.

Agree

On script/lambda routes we just need a single line to validate bean validator.validate(object);

Basically, one line, yes. But it is one line at every single handler that needs to do validation. So, across the project, this would likely result in a lot of identical lines. Additionally, you still need to check the number of violations and take appropriate action afterward. This would likely turn into some helper utility. If you have a set of projects, you would need to carry this helper across them.

In Jooby 1.x, it looked very consistent: you register a class once, and whenever you do ctx.body(), either lambda style or MVC (under the hood), you always get a valid object.

I just tried to reproduce the same in the PR.

Anyway, I'm more of an MVC user, so I can live without it in scripts if you think it's not worth it.

I believe it is vital to support automated validations for MVC. It is a common use case, it is convenient, and it is supported by other frameworks, so we need to remain competitive here.

I was thinking about the same approach with @Valid, and what bothers me about it is:

agentgt commented 1 month ago

@jknack This is actually been a large problem for us as well for some time going back to when I first helped you with the Value converter stuff.

For bean validation I recommend we aim for avaje-validator support. I had some code where I replaced the builtin reflection bean converters to using some annotation processing mapping internal library as well as avaje-validator. Furthermore @SentryMan I know is happy to help and already did a great job with the avaje-inject integration with Jooby.

However the problem with doing the validation on BeanConverter is they do not have access to the request and in some cases that is needed if you plan on providing form validation. I believe I did a threadlocal hack of binding context to the current thread (this obviously will not work for async) and can look later what we did.

This is also a problem with single value converters which I was going to get into #3465 where it is difficult to report back exactly which field is incorrect.

Ultimately what this comes down to is that @Nullable String request parameters are used instead of converter types everywhere in our code base as we cannot just fail with a 400 and some Jooby internal exception. This ends up being a large amount of boiler plate code and I have mitigated with some code generation but it is still painful.

jknack commented 1 month ago

@agentgt I introduced https://jooby.io/#mvc-api-parameters-bind which basically let you call/invoke arbitrary code. So for MVC was thinking to extend or use something similar to this which will be able to call HibernateValidator, avaje-validator, etc.

agentgt commented 1 month ago

@jknack I totally forgot about that as it was recently added but I remember being quite pleased when it was. Unfortunately we are still upgrading in some places to latest Jooby as well as the old stuff works at the moment.

I think the BindParam is the safest thing because folks are using records more and more you cannot really make a partially correct mutable object (pojo) like you would in Spring.

So maybe this is just a documentation and module thing now?

EDIT also I could contribute some annotation processing stuff I wrote as a separate module that basically Context -> Record/POJO.

jknack commented 1 month ago

EDIT also I could contribute some annotation processing stuff I wrote as a separate module that basically Context -> Record/POJO.

That is why I introduced @BindParam you can plug-in any annotation processor you want or manually created it. So, effectively replace the ReflectiveBeanConverter

kliushnichenko commented 3 weeks ago

After digging into this a bit more, I tend to agree that we should stick with MVC-only validation. Supporting validation in scripts/lambda functions would require injecting validation logic everywhere we deal with converting/decoding to beans. This makes it unlikely that we could come up with an elegant solution without introducing breaking changes. Perhaps this is something to consider for Jooby 4 if it becomes critical. For now, I think MVC validation should suffice.

I suggest the following implementation approach:

  1. Create a very simple jooby-validation module. This module will depend primarily on the spec of jakarta-validation-api, but not on a specific implementation. It will contain a utility to run validation on a bean/collection and throw an exception if necessary. It will also include a default handler for this exception.

  2. Use the jooby-validation module from prev step in jooby-apt.

  3. Use the jooby-validation module in any implementation module (e.g., HBV, avaje-validator). This will ensure that the necessary validation helper for jooby-apt is available at runtime, and the exception handler can be reused.

This approach offers several benefits:

I already have a working prototype. Let me know if this approach fits, and we can move forward.

jknack commented 3 weeks ago

I liked your proposal, please send a PR.

kliushnichenko commented 3 weeks ago

@jknack here https://github.com/jooby-project/jooby/pull/3508 still need some work to do, pls check if we are on a right track