quarkus-resteasy-problem icon indicating copy to clipboard operation
quarkus-resteasy-problem copied to clipboard

The default for the "instance" field violates the RFC

Open rkovarik opened this issue 1 year ago • 2 comments

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:

  • Pretty much any request which results into a problem response which doesn't explicitly provide the "instance" field.

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

rkovarik avatar Dec 28 '23 11:12 rkovarik

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?

lwitkowski avatar Jan 03 '24 09:01 lwitkowski

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.

  • it isn't a unique identifier of the problem occurrence
  • resolving the reference provides not expected content

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!

rkovarik avatar Jan 03 '24 12:01 rkovarik