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

What error should servers provide if someone tries to mutate an immutable resource? #406

Closed
tgeoghegan opened this issue Feb 7, 2023 · 8 comments · Fixed by #471
Closed
Assignees
Labels

Comments

@tgeoghegan
Copy link
Collaborator

There are a few places in DAP where a client (in the HTTP sense, not the DAP sense) creates an immutable resource using a PUT request involving a unique identifier:

  • DAP clients create reports with a report_id in the body
  • DAP leaders create aggregation jobs with an aggregation-job-id in the path
  • DAP collectors create collection jobs with a collection-job-id in the path

These objects cannot be further mutated using a PUT request (e.g., you can't change the aggregation parameter on a collection job after creating it). The question is: what should the error look like?

@tgeoghegan
Copy link
Collaborator Author

@branlwyd flagged this awkwardness while reviewing the implementation in Janus here. For now, we're going to use HTTP 409 Conflict. I think that might suffice for DAP. I'm not sure if we get anything from specifying another HTTP problem document type that has the same semantics as HTTP 409.

@jbr
Copy link
Contributor

jbr commented Feb 7, 2023

409 conflict doesn't seem wrong, but it does seem more intended for content versioning. I would expect a 422 in that it's a syntactically valid but semantically invalid request

@tgeoghegan
Copy link
Collaborator Author

I see what you mean, but 422 lacks specificity. If none of the HTTP statuses fit neatly, maybe an HTTP 400 with a DAP-specific problem type is the way forward, then.

The case of mutating a report is already handled by the text in the upload section, but we should still do something for collections and aggregation jobs.

@tgeoghegan
Copy link
Collaborator Author

RFC 9205's guidance on HTTP status codes is to use the most general applicable HTTP status and allow more specific information to be provided in problem documents. So I think what we should do here is add DAP language explaining that mutating an aggregation job or a collection is an error, and let implementations decide what 4xx code to use.

@cjpatton
Copy link
Collaborator

cjpatton commented Jun 16, 2023

Sorry to wait to chime in here until I've reviewed the PR. I think in principle I agree that we should try to follow HTTP semantics to the letter wherever possible, but the complexity of the solution (#471) makes me question the motivation somewhat.

If our intent is that you should always be able to retry a PUT (as long as the content hasn't changed), then I think that:

  1. We need to spell out a general mechanism that applies to all requests (the upload request counts here as well, right?)
  2. We need to work out the interaction of that mechanism with storage commitments.

With that in mind, I think it's important to ask that sticking to HTTP semantics is so important that it's worth the cost of adding a bunch of protocol logic to make it happen.

@simon-friedberger
Copy link
Contributor

The way I am reading this it's about which error to return not if the PUT should be possible. The problem with 409 is

This code is used in situations where the user might be able to resolve the conflict and resubmit the request.

A 403 seems appropriate here. I definitely support @tgeoghegan 's suggestion to make it clear in a DAP error what the problem is. I'm not sure if we're doing anybody any favor if we leave the specific response code undefined, though.

@tgeoghegan
Copy link
Collaborator Author

A 403 seems appropriate here. I definitely support @tgeoghegan 's suggestion to make it clear in a DAP error what the problem is. I'm not sure if we're doing anybody any favor if we leave the specific response code undefined, though.

I believe we should abide by the guidance in RFC 9205/BCP 56, which recommend using the most general possible status code but allow implementations to be more specific if they wish. That makes sense to me, since in this case we don't recommend that clients do anything specific if they encounter any specific 4xx error.

@simon-friedberger
Copy link
Contributor

That's also reasonable, this

Instead, applications using HTTP should define their errors to use the most applicable status code, making generous use of the general status codes (200, 400, and 500) when in doubt. Importantly, they should not specify a one-to-one relationship between status codes and application errors, thereby avoiding the exhaustion issue outlined above.

advocates both for using "the most applicable status code" but also for using the general ones when in doubt. I guess this issue clearly signals doubt.

tgeoghegan added a commit that referenced this issue Jun 21, 2023
Note also that once an aggregation job has been continued, it may not be
initialized again.

Resolves #406
tgeoghegan added a commit that referenced this issue Jun 21, 2023
Note also that once an aggregation job has been continued, it may not be
initialized again.

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

Successfully merging a pull request may close this issue.

4 participants