-
Notifications
You must be signed in to change notification settings - Fork 16
Adds a default validation exception handler #670
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
base: master
Are you sure you want to change the base?
Conversation
|
@mechite how's this? |
mechite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SentryMan hard to review on a phone 🤣 I like it
|
@SentryMan I'm reviewing here now on a computer.
We can do something like: @SentryMan I see JSON Schema for HTTP Problems which is a specification for how the responses are meant to look. While the following doesn't exclude the "avaje" name from the response, it does make it more abstract: If we upload this schema to |
idk what to make of this statement |
this guy
I mean that we basically can just point to a class file in Avaje, instead of a web URL Ideally to me, this JSON schema I said that the RFC contains, could have been uploaded to e.g. ietf.org,
This suggestion was to say that we upload the official JSON schema, which is probably more useful |
|
d55a7a6 - OK, but we better verify that this is indeed valid for the |
Would this really be worth it over adding json-core or even full jsonb - since they have streaming writers? |
nahhh it's probably fine |
Pull request was converted to draft
|
🤣 he says it's fine, then optimizes anyway |
Removed unused getter methods from ValidationResponse.
|
I'm not convinced by the "packaging" of where this should be yet - for example the Helidon implementation probably should go into avaje-nima as that is the opinionated combination of validator + inject + helidon + jsonb. This has to rely on optional dependencies and reflection which makes me go "Hmmm". |
|
it's no worse than the automatic validator registration for http |
|
BTW, just another note. try {
// access an optional dep
} catch (NoClassDefFoundError error) {
// ignore
}This works perfectly fine at runtime and avoids reflection API, and at the |
|
you know I forgot you could catch those, time to replace every |
What of the people that want to use hibernate or a custom implementation? This PR is supposed to be agnostic and not favor any specific validator implementation.
Reflection is gone now, and we even have an optimized path through JPMS lookup when running on the module path. What downsides do you see of optional dependencies? |
http-inject-plugin/src/main/java/io/avaje/http/inject/ValidationResponse.java
Outdated
Show resolved
Hide resolved
Updated the detail message to reflect the specific exception thrown and removed the type field from the JSON output.
This is a hack, but I've done it a lot and personally think it could be a better way of handling optional dependencies over `Class.forName` The thing is, `Class.forName` was designed for this very use-case of optional dependencies. Javalin claims "no reflection" even in the very readme file, so there are two ways to back this claim - Remove all non-test usage of reflection entirely, opting for techniques like these (which work and in my opinion have no downsides, except that readers might be annoyed we are catching `Error`s? This even works properly in a `native-image` as well) - Remove the claim of "no reflection" Please note that I have no clue on writing idiomatic Kotlin. Related-to: avaje/avaje-http#670 (comment) Related-to: avaje/avaje-inject#932 Related-to: avaje/avaje-http#675 Signed-off-by: Mahied Maruf <[email protected]>
Resolves #669 by adding a default handler via the inject plugin