quarkiverse / quarkus-resteasy-problem

Unified error responses for Quarkus REST APIs via Problem Details for HTTP APIs (RFC9457 & RFC7807)
https://docs.quarkiverse.io/quarkus-resteasy-problem/dev
Apache License 2.0
69 stars 12 forks source link

The default for the "instance" field violates the RFC #337

Open rkovarik opened 10 months ago

rkovarik commented 10 months ago

ProblemDefaultsProvider replaces null value of instance with URI of currently served endpoint, i.e /products/123 which violates the rfc7807/rfc9457:

The "instance" member is a JSON string containing a URI reference that identifies the specific occurrence of the problem.

So it isn't supposed to be an URI of the currently served endpoint but each occurrence of the problem should have a unique identifier.

Introduced by https://github.com/TietoEVRY/quarkus-resteasy-problem/pull/39

To Reproduce Steps to reproduce the behavior:

Expected behavior "instance" field not present or has a valid value. Or at least ProblemDefaultsProvider can be disabled (https://github.com/TietoEVRY/quarkus-resteasy-problem/issues/326)

Workaround

@ApplicationScoped
public class FixProblemInstanceProcessor implements ProblemPostProcessor {

    /**
     * Has to run after com.tietoevry.quarkus.resteasy.problem.postprocessing.ProblemDefaultsProvider. See {@link ProblemDefaultsProvider#priority()}
     */
    @Override
    public int priority() {
        return 99;
    }

    @Override
    public HttpProblem apply(HttpProblem problem, ProblemContext context) {
        return HttpProblem.builder(problem)
                .withInstance(null) //or a valid value
                .build();
    }
}
lwitkowski commented 10 months ago

Hi @rkovarik, you are not wrong, but I don't think current implementation violates the RFC in any harmful way - nobody (not even RFC) expects this uri to be resolvable (have any meaningful content when you GET it).

We provide some maybe-not-the-best-default, we could make it optional (opt-in or opt-out). If we would want to make it truly unique (and useful for api clients) we would have to generate some kind of trace identifiers (and log them for debugging purpose) - this is intentionally left for application developer to decide how to implement this.

Or maybe you have any suggestions how to make it more useful for you?

rkovarik commented 10 months ago

Hey @lwitkowski,

nobody (not even RFC) expects this uri to be resolvable (have any meaningful content when you GET it).

Fully agree but the fact that the "instance" field provided by this library actually is/might be resolvable (as it's the path to the endpoint) makes this even worse.

Or maybe you have any suggestions how to make it more useful for you?

I think the easiest fix would be to just not set any default. We have used the "instance" field for some time without realising it violates the spec. Some other users might even process the "instance" without realising its content is not correct.

If we would want to make it truly unique (and useful for api clients) we would have to generate some kind of trace identifiers (and log them for debugging purpose) - this is intentionally left for application developer to decide how to implement this.

Yep, that's what we did. We use the traceId and spanId generated by opentelemetry library (in the form of urn:traceId:xxx:spanId:xxx). I hope this is fine according to the RFC🤞. Happy to hear about another/better format. Introducing dependency to opentelemetry is probably not something you want to do in this library.

Anyway, thanks for the library!