Improve developer experience and documentation of registering custom ProblemPostProcessor

[lwitkowski]

Makes it easy to implement custom ProblemPostProcessor by creating a regular CDI bean.

Closes #301 (in combination with #311)

[lwitkowski]

@chberger please also have a look, for some reason I'm not able to add you as a official 'reviewer'

[pazkooda]

Will take a look later this evening on this.

[pazkooda]

Nice and clean 👍🏻
As of me ready to be merged 🚀

[lwitkowski]

@chberger I'm gonna merge this now, but if you have any feedback from a user (developer) point of view feel free. I'm not releasing it to maven central yet, so there'll still be time for enhancements.

[chberger]

Overall it looks very good! I've dropped just some minor comments.

[chberger]

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.

[lwitkowski]

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.

[chberger]

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();
    }
}

[lwitkowski]

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)

[chberger]

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

[chberger]

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.

[lwitkowski]

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

[lwitkowski]

@chberger works like a charm, thanks
c69cc6a

[chberger]

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

[lwitkowski]

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.

[chberger]

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

[lwitkowski]

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