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

WIP: jooby-hbv module #3494

Closed
wants to merge 22 commits into from

Conversation

kliushnichenko
Copy link
Contributor

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
Copy link
Member

jknack commented Aug 6, 2024

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:

  • The HBV API is pretty simple and straightforward to bootstrap.
  • On script/lambda routes we just need a single line to validate bean validator.validate(object);

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
Copy link
Contributor Author

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:

  • We need to do some magic in the jooby-apt module. Is it OK to make jooby-apt partially responsible for validation?
  • The Validator instance provider will still be placed in jooby-hbv, so, the whole solution will be spread across two modules. This complicates testing, and further support, and violates some aspects of the SRP.

@agentgt
Copy link
Contributor

agentgt commented Aug 9, 2024

@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
Copy link
Member

jknack commented Aug 9, 2024

@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
Copy link
Contributor

agentgt commented Aug 9, 2024

@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
Copy link
Member

jknack commented Aug 10, 2024

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

jknack and others added 12 commits August 15, 2024 18:14
415 unsupported media type when using mount to add routes
…pdates

Bumps the dependencies group with 19 updates in the / directory:

| Package | From | To |
| --- | --- | --- |
| [com.amazonaws:aws-java-sdk-bom](https://github.com/aws/aws-sdk-java) | `1.12.767` | `1.12.769` |
| io.jstach:jstachio | `1.3.5` | `1.3.6` |
| io.jstach:jstachio-apt | `1.3.5` | `1.3.6` |
| org.slf4j:slf4j-api | `2.0.13` | `2.0.16` |
| org.slf4j:log4j-over-slf4j | `2.0.13` | `2.0.16` |
| org.slf4j:jcl-over-slf4j | `2.0.13` | `2.0.16` |
| org.slf4j:jul-to-slf4j | `2.0.13` | `2.0.16` |
| org.slf4j:slf4j-simple | `2.0.13` | `2.0.16` |
| [ch.qos.logback:logback-classic](https://github.com/qos-ch/logback) | `1.5.6` | `1.5.7` |
| [com.graphql-java:graphql-java](https://github.com/graphql-java/graphql-java) | `22.1` | `22.2` |
| [org.hibernate.orm:hibernate-core](https://github.com/hibernate/hibernate-orm) | `6.5.2.Final` | `6.6.0.Final` |
| [org.junit.jupiter:junit-jupiter-api](https://github.com/junit-team/junit5) | `5.10.3` | `5.11.0` |
| [org.junit.jupiter:junit-jupiter-engine](https://github.com/junit-team/junit5) | `5.10.3` | `5.11.0` |
| [org.junit.jupiter:junit-jupiter-params](https://github.com/junit-team/junit5) | `5.10.3` | `5.11.0` |
| [org.codehaus.mojo:exec-maven-plugin](https://github.com/mojohaus/exec-maven-plugin) | `3.3.0` | `3.4.1` |
| [org.apache.maven.plugins:maven-gpg-plugin](https://github.com/apache/maven-gpg-plugin) | `3.2.4` | `3.2.5` |
| [io.reactivex.rxjava3:rxjava](https://github.com/ReactiveX/RxJava) | `3.1.8` | `3.1.9` |
| org.apache.commons:commons-lang3 | `3.15.0` | `3.16.0` |
| [io.projectreactor:reactor-core](https://github.com/reactor/reactor-core) | `3.6.8` | `3.6.9` |



Updates `com.amazonaws:aws-java-sdk-bom` from 1.12.767 to 1.12.769
- [Changelog](https://github.com/aws/aws-sdk-java/blob/master/CHANGELOG.md)
- [Commits](aws/aws-sdk-java@1.12.767...1.12.769)

Updates `io.jstach:jstachio` from 1.3.5 to 1.3.6

Updates `io.jstach:jstachio-apt` from 1.3.5 to 1.3.6

Updates `io.jstach:jstachio-apt` from 1.3.5 to 1.3.6

Updates `org.slf4j:slf4j-api` from 2.0.13 to 2.0.16

Updates `org.slf4j:log4j-over-slf4j` from 2.0.13 to 2.0.16

Updates `org.slf4j:jcl-over-slf4j` from 2.0.13 to 2.0.16

Updates `org.slf4j:jul-to-slf4j` from 2.0.13 to 2.0.16

Updates `org.slf4j:slf4j-simple` from 2.0.13 to 2.0.16

Updates `org.slf4j:log4j-over-slf4j` from 2.0.13 to 2.0.16

Updates `org.slf4j:jcl-over-slf4j` from 2.0.13 to 2.0.16

Updates `org.slf4j:jul-to-slf4j` from 2.0.13 to 2.0.16

Updates `ch.qos.logback:logback-classic` from 1.5.6 to 1.5.7
- [Commits](qos-ch/logback@v_1.5.6...v_1.5.7)

Updates `com.graphql-java:graphql-java` from 22.1 to 22.2
- [Release notes](https://github.com/graphql-java/graphql-java/releases)
- [Commits](graphql-java/graphql-java@v22.1...v22.2)

Updates `org.hibernate.orm:hibernate-core` from 6.5.2.Final to 6.6.0.Final
- [Release notes](https://github.com/hibernate/hibernate-orm/releases)
- [Changelog](https://github.com/hibernate/hibernate-orm/blob/6.6.0/changelog.txt)
- [Commits](hibernate/hibernate-orm@6.5.2...6.6.0)

Updates `org.junit.jupiter:junit-jupiter-api` from 5.10.3 to 5.11.0
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.10.3...r5.11.0)

Updates `org.junit.jupiter:junit-jupiter-engine` from 5.10.3 to 5.11.0
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.10.3...r5.11.0)

Updates `org.junit.jupiter:junit-jupiter-params` from 5.10.3 to 5.11.0
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.10.3...r5.11.0)

Updates `org.junit.jupiter:junit-jupiter-engine` from 5.10.3 to 5.11.0
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.10.3...r5.11.0)

Updates `org.codehaus.mojo:exec-maven-plugin` from 3.3.0 to 3.4.1
- [Release notes](https://github.com/mojohaus/exec-maven-plugin/releases)
- [Commits](mojohaus/exec-maven-plugin@3.3.0...3.4.1)

Updates `org.apache.maven.plugins:maven-gpg-plugin` from 3.2.4 to 3.2.5
- [Release notes](https://github.com/apache/maven-gpg-plugin/releases)
- [Commits](apache/maven-gpg-plugin@maven-gpg-plugin-3.2.4...maven-gpg-plugin-3.2.5)

Updates `io.reactivex.rxjava3:rxjava` from 3.1.8 to 3.1.9
- [Release notes](https://github.com/ReactiveX/RxJava/releases)
- [Commits](ReactiveX/RxJava@v3.1.8...v3.1.9)

Updates `org.apache.commons:commons-lang3` from 3.15.0 to 3.16.0

Updates `org.slf4j:slf4j-simple` from 2.0.13 to 2.0.16

Updates `io.projectreactor:reactor-core` from 3.6.8 to 3.6.9
- [Release notes](https://github.com/reactor/reactor-core/releases)
- [Commits](reactor/reactor-core@v3.6.8...v3.6.9)

Updates `org.junit.jupiter:junit-jupiter-params` from 5.10.3 to 5.11.0
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit5@r5.10.3...r5.11.0)

---
updated-dependencies:
- dependency-name: com.amazonaws:aws-java-sdk-bom
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.jstach:jstachio
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.jstach:jstachio-apt
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.jstach:jstachio-apt
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:slf4j-api
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:log4j-over-slf4j
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:jcl-over-slf4j
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:jul-to-slf4j
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:slf4j-simple
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:log4j-over-slf4j
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:jcl-over-slf4j
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.slf4j:jul-to-slf4j
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: ch.qos.logback:logback-classic
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: com.graphql-java:graphql-java
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.hibernate.orm:hibernate-core
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.junit.jupiter:junit-jupiter-api
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.junit.jupiter:junit-jupiter-engine
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.junit.jupiter:junit-jupiter-params
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.junit.jupiter:junit-jupiter-engine
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.codehaus.mojo:exec-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.apache.maven.plugins:maven-gpg-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.reactivex.rxjava3:rxjava
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.apache.commons:commons-lang3
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.slf4j:slf4j-simple
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.projectreactor:reactor-core
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.junit.jupiter:junit-jupiter-params
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: dependencies
...

Signed-off-by: dependabot[bot] <[email protected]>
…aven/dependencies-d14dd83f9f

build(deps): bump the dependencies group across 1 directory with 19 updates
Bumps the dependencies group with 9 updates:

| Package | From | To |
| --- | --- | --- |
| [io.dropwizard.metrics:metrics-core](https://github.com/dropwizard/metrics) | `4.2.26` | `4.2.27` |
| [io.dropwizard.metrics:metrics-healthchecks](https://github.com/dropwizard/metrics) | `4.2.26` | `4.2.27` |
| [io.dropwizard.metrics:metrics-jvm](https://github.com/dropwizard/metrics) | `4.2.26` | `4.2.27` |
| [com.google.guava:guava](https://github.com/google/guava) | `33.2.1-jre` | `33.3.0-jre` |
| [org.apache.maven:maven-plugin-api](https://github.com/apache/maven) | `3.9.8` | `3.9.9` |
| [org.apache.maven:maven-core](https://github.com/apache/maven) | `3.9.8` | `3.9.9` |
| [org.apache.maven.plugin-tools:maven-plugin-annotations](https://github.com/apache/maven-plugin-tools) | `3.13.1` | `3.14.0` |
| [org.apache.maven.plugins:maven-surefire-plugin](https://github.com/apache/maven-surefire) | `3.3.1` | `3.4.0` |
| [org.apache.maven.plugins:maven-plugin-plugin](https://github.com/apache/maven-plugin-tools) | `3.13.1` | `3.14.0` |


Updates `io.dropwizard.metrics:metrics-core` from 4.2.26 to 4.2.27
- [Release notes](https://github.com/dropwizard/metrics/releases)
- [Commits](dropwizard/metrics@v4.2.26...v4.2.27)

Updates `io.dropwizard.metrics:metrics-healthchecks` from 4.2.26 to 4.2.27
- [Release notes](https://github.com/dropwizard/metrics/releases)
- [Commits](dropwizard/metrics@v4.2.26...v4.2.27)

Updates `io.dropwizard.metrics:metrics-jvm` from 4.2.26 to 4.2.27
- [Release notes](https://github.com/dropwizard/metrics/releases)
- [Commits](dropwizard/metrics@v4.2.26...v4.2.27)

Updates `io.dropwizard.metrics:metrics-healthchecks` from 4.2.26 to 4.2.27
- [Release notes](https://github.com/dropwizard/metrics/releases)
- [Commits](dropwizard/metrics@v4.2.26...v4.2.27)

Updates `io.dropwizard.metrics:metrics-jvm` from 4.2.26 to 4.2.27
- [Release notes](https://github.com/dropwizard/metrics/releases)
- [Commits](dropwizard/metrics@v4.2.26...v4.2.27)

Updates `com.google.guava:guava` from 33.2.1-jre to 33.3.0-jre
- [Release notes](https://github.com/google/guava/releases)
- [Commits](https://github.com/google/guava/commits)

Updates `org.apache.maven:maven-plugin-api` from 3.9.8 to 3.9.9
- [Release notes](https://github.com/apache/maven/releases)
- [Commits](apache/maven@maven-3.9.8...maven-3.9.9)

Updates `org.apache.maven:maven-core` from 3.9.8 to 3.9.9
- [Release notes](https://github.com/apache/maven/releases)
- [Commits](apache/maven@maven-3.9.8...maven-3.9.9)

Updates `org.apache.maven.plugin-tools:maven-plugin-annotations` from 3.13.1 to 3.14.0
- [Release notes](https://github.com/apache/maven-plugin-tools/releases)
- [Commits](apache/maven-plugin-tools@maven-plugin-tools-3.13.1...maven-plugin-tools-3.14.0)

Updates `org.apache.maven.plugins:maven-surefire-plugin` from 3.3.1 to 3.4.0
- [Release notes](https://github.com/apache/maven-surefire/releases)
- [Commits](apache/maven-surefire@surefire-3.3.1...surefire-3.4.0)

Updates `org.apache.maven.plugins:maven-plugin-plugin` from 3.13.1 to 3.14.0
- [Release notes](https://github.com/apache/maven-plugin-tools/releases)
- [Commits](apache/maven-plugin-tools@maven-plugin-tools-3.13.1...maven-plugin-tools-3.14.0)

---
updated-dependencies:
- dependency-name: io.dropwizard.metrics:metrics-core
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.dropwizard.metrics:metrics-healthchecks
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.dropwizard.metrics:metrics-jvm
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.dropwizard.metrics:metrics-healthchecks
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: io.dropwizard.metrics:metrics-jvm
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: com.google.guava:guava
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.apache.maven:maven-plugin-api
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.apache.maven:maven-core
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
- dependency-name: org.apache.maven.plugin-tools:maven-plugin-annotations
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.apache.maven.plugins:maven-surefire-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
- dependency-name: org.apache.maven.plugins:maven-plugin-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: dependencies
...

Signed-off-by: dependabot[bot] <[email protected]>
…aven/dependencies-096661fcd5

build(deps): bump the dependencies group with 9 updates
…xternal process

- unload module on .class file changed
- fix jooby-project#3505
@kliushnichenko
Copy link
Contributor Author

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:

  • Jooby core remains free from any validation dependencies.
  • jooby-apt depends only on the jakarta-validation-api specification.
  • validation execution code and the default exception handler are centralized in one place.
  • Bonus: Utility methods from jooby-validation can be utilized in lambda/functions, simplifying validation to a single line.

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

@jknack
Copy link
Member

jknack commented Aug 24, 2024

I liked your proposal, please send a PR.

# Conflicts:
#	modules/jooby-hbv/pom.xml
#	modules/jooby-hbv/src/main/java/io/jooby/hbv/HbvModule.java
@kliushnichenko
Copy link
Contributor Author

@jknack here #3508
still need some work to do, pls check if we are on a right track

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