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

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

Open
rkovarik opened this issue Dec 28, 2023 · 2 comments
Open

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

rkovarik opened this issue Dec 28, 2023 · 2 comments

Comments

@rkovarik
Copy link

rkovarik commented Dec 28, 2023

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
#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 (#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
Copy link
Collaborator

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
Copy link
Author

rkovarik commented Jan 3, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants