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

problem details in modules #3557

Merged

Conversation

kliushnichenko
Copy link
Contributor

While was working on problem details support in validation modules the general concept a bit changed.
Enabling the ProblemDetailsHandler moved to the hocon configuration.

  1. It greatly simplifies the integration of problem details in modules. Modules can now detect if ProblemDetailsHandler is enabled at an early stage of the module initiation. With the previous approach, it would have been much harder and might have led to implementing awkward solutions.

  2. An additional benefit of the new approach is that ProblemDetailsHandler will be automatically registered as the last handler in the error handlers chain, so dev no longer need to worry about that.

So, to enable ProblemDetailsHandler the next config now required:

problem.details {
  enable: true
  muteCodes: [401, 106]
  muteTypes: ['com.example.MyException']
}

@jknack pls share your thoughts, if you are Ok with the new approach I'll continue with docs update and some minor improvements

@jknack
Copy link
Member

jknack commented Oct 11, 2024

I have fixed feelings. I like it but then is like we need to add the a new property in Jooby class isn't wrong but wonder if we could do it using an Extension + install call

install(new ProblemsDetailsModule());

Once installed take care of configure everything. The issue here is how to integrate with other error handler like validation

@kliushnichenko
Copy link
Contributor Author

Just for clarity, there is no new property in Jooby class, just a helper method that reads the config. We can move this helper to any other place, all it needs is access to the config.

Regarding module

install(new ProblemsDetailsModule());

If a new module only sets the same hocon config as it is now(just code-driven approach) it will work. But this module should always be registered before validation modules.

If the module should also register the handler error(new ProblemDetailsHandler()), that's the issue.
Because the global handler should be the last in the chain. Even if we made this module lateinit=true it will fix one issue (global err handler now registered the last), but we introduce another issue - the config properties are set too late, and validation modules won't detect that ProblemDetails is enabled.
So, the last option is to move the detection of ProblemDetails in validation modules to the stage when the first error is thrown - I believe this is the worst option. That's why I ended up with a hocon config.

@jknack
Copy link
Member

jknack commented Oct 12, 2024

@kliushnichenko thanks for explaining let's proceed with documentation.

@kliushnichenko
Copy link
Contributor Author

docs and some minor improvements added.
The major change is the removal of a type property in custom problem errors. Since JSON Pointer spec clearly defines how to point to the whole document(a "" pointer). Ready to merge

@@ -192,4 +201,25 @@ private void logProblem(Context ctx, HttpProblem problem, Throwable cause) {
private String buildLogMsg(Context ctx, HttpProblem problem, StatusCode statusCode) {
return "%s | %s".formatted(ErrorHandler.errorMessage(ctx, statusCode), problem.toString());
}

public static ProblemDetailsHandler fromConfig(Config config) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to from and let's add javadoc

* @author kliushnichenko
* @since 3.4.2
*/
public class JsonPointer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool

Copy link
Member

@jknack jknack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just few minor changes

@jknack jknack added this to the 3.4.2 milestone Oct 14, 2024
@jknack jknack merged commit 03b6c8f into jooby-project:3.x Oct 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants