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

Improve developer experience and documentation of registering custom ProblemPostProcessor #312

Merged
merged 7 commits into from
Oct 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ Make sure JDK 11 is in your PATH, then run:
```shell
mvn io.quarkus:quarkus-maven-plugin:3.5.0:create \
-DprojectGroupId=problem \
-DprojectArtifactId=quarkus-resteasy-problem-playground2 \
-DprojectArtifactId=quarkus-resteasy-problem-playground \
-DclassName="problem.HelloResource" \
-Dpath="/hello" \
-Dextensions="resteasy,resteasy-jackson"
Expand Down Expand Up @@ -219,6 +219,32 @@ quarkus.log.category.http-problem.level=ERROR # only HTTP 5XX problems are logge
quarkus.log.category.http-problem.level=OFF # disables all problems-related logging
```

## Custom ProblemPostProcessor
If you want to intercept, change or augment a mapped `HttpProblem` before it gets serialized into raw HTTP response
body, you can create a bean extending `ProblemPostProcessor`, and override `apply` method.

**Important: Make sure your implementation is thread-safe, as it may potentially be called concurrently if multiple
requests throw exceptions at the same time.**

Example:
```java
@ApplicationScoped
@Startup // makes sure bean is instantiated eagerly on startup
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would a user instantiate such a bean eagerly? AFAIK you register the bean(s) via a static main method (STATIC_INIT) anyway. If your intention is to avoid that Quarkus removes the "unused" bean, I would recommend to use @io.quarkus.arc.Unremovable instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Registering CDI beans as postprocessors happens in runtime (RUNTIME_INIT):

CDI.current().select(ProblemPostProcessor.class)
                .forEach(ExceptionMapperBase.postProcessorsRegistry::register);

If you reference your postprocessor from other beans then this annotation is not needed (Quarkus will create and register it as bean in CDI on startup), but if it's not (and I believe it to be true in most cases) this bean will never be created.
@Unremovable - as far as I undestand - does not affect how the bean is instantiated (lazily or eagerly) - from this extension's point of view Quarkus can remove this bean immediately after startup. But the bean needs to be registered in CDI in RUNTIME_INIT phase for this autoregistration to work correctly.

Copy link
Contributor

@chberger chberger Oct 30, 2023

Choose a reason for hiding this comment

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

Registering CDI beans as postprocessors happens in runtime (RUNTIME_INIT):

CDI.current().select(ProblemPostProcessor.class)
                .forEach(ExceptionMapperBase.postProcessorsRegistry::register);

Sure, my fault.

If you reference your postprocessor from other beans then this annotation is not needed (Quarkus will create and register it as bean in CDI on startup), but if it's not (and I believe it to be true in most cases) this bean will never be created.

@Unremovable - as far as I undestand - does not affect how the bean is instantiated (lazily or eagerly) - from this extension's point of view Quarkus can remove this bean immediately after startup. But the bean needs to be registered in CDI in RUNTIME_INIT phase for this autoregistration to work correctly.

Marking a bean @Unremovable is mentioned in the offical guide especially for that case u have implemented:
Please refer to Remove unused beans.

From my understanding, although the bean looks "unused", it won't get removed. Exactly what we want. Thus a proxy gets instantiated (since it is @ApplicationScoped see nonstandard features) and lazily created when registered at RUNTIME_INIT. Otherwise I cannot explain why this code works ..

@ApplicationScoped
@Unremovable
public class TestProcessor implements ProblemPostProcessor {
    @Override
    public HttpProblem apply(HttpProblem problem, ProblemContext context) {
        return HttpProblem.builder(problem)
                .with("test", "test")
                .build();
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, @Unremovable will work as well (I falsely assume removability is about runtime, but in fact it's about build time), I will add this to the example as possible option to use instead of @Startup.

Nevertheless I don't think there's anything particularly wrong with @Startup, which makes the bean unremovable (by implicitly declaring an observer method)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, totally agree with that. I've just suggested another idea which could state the intention a little bit more explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe one last idea. How about adding an additional build-step which marks each @ProblemPostProcessor as unremovable:

    @BuildStep
    UnremovableBeanBuildItem unremovableBeans() {
        return UnremovableBeanBuildItem.beanTypes(ProblemPostProcessor.class);
    }

By doing so, no additional annotation is required and the user doesn't need to be aware of any Quarkus internals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That looks neat indeed, a tiny bit of magic, but it looks like it's worth it. I'll look into it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@chberger works like a charm, thanks
c69cc6a

class CustomPostProcessor implements ProblemPostProcessor {

@Inject // acts like normal bean, DI works fine etc
Validator validator;

@Override
public HttpProblem apply(HttpProblem problem, ProblemContext context) {
return HttpProblem.builder(problem)
.with("injected_from_custom_post_processor", "hello world " + context.path)
.build();
}

}
```

## Troubles?

If you have questions, concerns, bug reports, etc, please file an issue in this repository's Issue Tracker. You may also want to have a look at [troubleshooting FAQ](./TROUBLESHOOTING.md).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@ void setupMetrics(ProblemRecorder recorder, ProblemBuildConfig config) {
}
}

@Record(RUNTIME_INIT)
@BuildStep
void registerCustomPostProcessors(ProblemRecorder recorder) {
recorder.registerCustomPostProcessors();
}

protected Logger logger() {
return LoggerFactory.getLogger(FEATURE_NAME);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package com.tietoevry.quarkus.resteasy.problem;

import com.tietoevry.quarkus.resteasy.problem.postprocessing.ProblemContext;
import com.tietoevry.quarkus.resteasy.problem.postprocessing.ProblemPostProcessor;
import io.quarkus.runtime.Startup;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.inject.Inject;
import jakarta.validation.Validator;

@ApplicationScoped
@Startup
class CustomPostProcessor implements ProblemPostProcessor {

@Inject
Validator validator;

@Override
public HttpProblem apply(HttpProblem problem, ProblemContext context) {
return HttpProblem.builder(problem)
.with("injected_from_custom_post_processor", "you called " + context.path)
.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,12 @@ void httpProblemShouldReturnHeaders() {
.body("stacktrace", nullValue());
}

@Test
void shouldRegisterProblemPostProcessorCustomImplementationsFromCDI() {
given()
.queryParam("message", SAMPLE_DETAIL)
.get("/throw/generic/runtime-exception")
.then()
.body("injected_from_custom_post_processor", equalTo("you called /throw/generic/runtime-exception"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,8 @@ public Builder withHeader(String headerName, Object value) {
}

/**
* Adds custom property to the response.
*
* @throws IllegalArgumentException if key is any of type, title, status, detail or instance
*/
public Builder with(String key, Object value) throws IllegalArgumentException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public final class ProblemContext {
public final Throwable cause;

/**
* URI path of current endpoint.
* URI path of the endpoint.
*/
public final String path;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,32 @@

import com.tietoevry.quarkus.resteasy.problem.HttpProblem;
import java.util.Comparator;
import java.util.function.BiFunction;

/**
* Post-processors use, change or enhance Problems created by ExceptionMappers via 'apply' method, before they get passed on to
* serializers.
* Post-processors use, change or enhance HttpProblem created by ExceptionMappers via 'apply' method, before they get
* passed on to serializers.
*/
public interface ProblemPostProcessor extends BiFunction<HttpProblem, ProblemContext, HttpProblem> {
public interface ProblemPostProcessor {

Comparator<ProblemPostProcessor> DEFAULT_ORDERING = Comparator.comparingInt(ProblemPostProcessor::priority).reversed();

/**
* Interceptor method for HttpProblems. In case problem should be changed or enhanced, one can use
* 'HttpProblem.builder(httpProblem)'.
* <p>
* Implementations should be thread-safe.
*
* @param problem Original HttpProblem, possibly processed by other processors with higher priority.
* @param context Additional, internal metadata not included in HttpProblem
* @return Can be original HttpProblem (for peek-type processors), changed copy or completely new HttpProblem (for map-type
* processors.
*/
HttpProblem apply(HttpProblem problem, ProblemContext context);

/**
* Defines order in which processors are triggered. Bigger value means precedence before processors
* with lower priority.
* When two processors have the same priority then order of invocation is undefined.
*/
default int priority() {
return Integer.MIN_VALUE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.tietoevry.quarkus.resteasy.problem.ExceptionMapperBase;
import io.quarkus.runtime.annotations.Recorder;
import jakarta.enterprise.inject.spi.CDI;
import java.util.Set;

/**
Expand All @@ -24,4 +25,9 @@ public void enableMetrics() {
ExceptionMapperBase.postProcessorsRegistry.register(new MicroprofileMetricsCollector());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a user should not call ExceptionMapperBase.postProcessorsRegistry.register directly, maybe it makes sense reducing the visibility of that particular method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not that you should not call it - it's not forbidden, but you're right - it could have been package private. As such change is breaking, we can do it in 4.0 earliest though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, I thought your intention was not to offer this API publicly. But yeah, fine to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't really matter now what my intention was, it would be api breaking change, and - if we want to be predictable and compliant with semver - we can't change it just like that

}

public void registerCustomPostProcessors() {
CDI.current().select(ProblemPostProcessor.class)
pazkooda marked this conversation as resolved.
Show resolved Hide resolved
.forEach(ExceptionMapperBase.postProcessorsRegistry::register);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class TestProcessor implements ProblemPostProcessor {
}

@Override
public HttpProblem apply(HttpProblem problem, ProblemContext problemContext) {
public HttpProblem apply(HttpProblem problem, ProblemContext context) {
invocations.add(priority);
return problem;
}
Expand Down
Loading