Avoid potential NPE in Metrics filter

[geoand]

The NPE could happen if an exception was thrown
in a ContainerRequestFilter

Fixes: #16620

[quarkus-bot]

/cc @ebullient, @jmartisk

[FroMage]

I'm a bit torn about this: it's hard for filter authors to know which (allowed) parameter may be null, and why.

Perhaps it would be better to always have a ResourceInfo even if its methods then throw an exception explaining that there's no resource because we had an exception before we found it.

But this would not make it easier for this particular filter which wants to ignore calls when that happens.

OTOH, is it really normal to invoke response filters when an exception happens that prevents us from finding the target resource?

[FroMage]

Well, the spec says "A response mapped from an exception MUST be processed using the filter chain and the interceptor chain (if an entity is present in the mapped response). "

So yeah, we apparently need to invoke them no matter how broken everything else is.

[geoand]

So yeah, we apparently need to invoke them no matter how broken everything else is.

Unfortunately yes...

[geoand]

Perhaps it would be better to always have a ResourceInfo even if its methods then throw an exception explaining that there's no resource because we had an exception before we found it.

I actually think this would make it even more confusing

[FroMage]

OK hold on, the javadoc of ResourceInfo says:

Methods in this class MAY return null if a resource class and method have not been matched, e.g. in a standalone, pre-matching ContainerRequestFilter that was not provided by a post-matching PreMatching.

So that's our answer that is spec-compliant.

[FroMage]

According to javadoc, the right thing to do is to have a non-null ResourceInfo and a null return value from its methods. Given that both options are equally confusing IMO, we should implement the one that is documented and has a better interop chance.

[geoand]

OK lemme do that

[FroMage]

Great, thanks!

[geoand]

👍🏼

[gsmet]

@geoand this one conflicts (due to the MP 4 changes). Not sure it's worth rewriting the patch? I'll let you judge of that.

[geoand]

I think it's not terribly important, but I'd be up for it if someone complains :)