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

Use POST instead of PUT #450

Closed
ekr opened this issue Apr 28, 2023 · 11 comments
Closed

Use POST instead of PUT #450

ekr opened this issue Apr 28, 2023 · 11 comments
Labels

Comments

@ekr
Copy link
Collaborator

ekr commented Apr 28, 2023

RFC 9110 says:

The fundamental difference between the POST and PUT methods is highlighted by the different intent for the enclosed representation. The target resource in a POST request is intended to handle the enclosed representation according to the resource's own semantics, whereas the enclosed representation in a PUT request is defined as replacing the state of the target resource. Hence, the intent of PUT is idempotent and visible to intermediaries, even though the exact effect is only known by the origin server.

As we are not replacing the resource at: {leader}/tasks/{task-id}/reports we should use POST

@cjpatton
Copy link
Collaborator

I don't have a well-formed view point here. I think from a protocol flow perspective the main thing we need is to be able to distinguish the initial message for an aggregation job (or collection job) with a continue (or poll) message. I think we already get this from media types but I'll check if/when we put a PR for this.

cc/ @tgeoghegan

@ekr
Copy link
Collaborator Author

ekr commented Apr 28, 2023

Right. I think this generally should not be encoded in methods.

@martinthomson @mnot?

@mnot
Copy link

mnot commented Apr 30, 2023

Context?

@tgeoghegan
Copy link
Collaborator

The semantic I focused on when I selected PUT for these requests is idempotence. I did consider doing the REST thing of transferring the entire representation of the resources in requests and responses, but concluded that this is quite wasteful in the aggregation sub-protocol: there's no reason for the helper to echo report shares and aggregation parameters back to the leader.

That said, my main takeaway from having spent time on this during DAP-04 is that it doesn't really matter what methods we use because we don't expect DAP to be executed by a generic HTTP client (e.g. a web browser) but rather by dedicated DAP implementations. That means that it's not the end of the world if play a little fast and loose with method semantics.

Long story short: I don't object to using different methods here.

@martinthomson
Copy link
Contributor

If what you are doing is asking for a change to a resource, rather than its replacement PATCH works over PUT. Idempotence is not guaranteed in that case, but it could be, depending on how you structure the request. The same applies to POST, which could be idempotent and therefore safe to retry with that knowledge.

@divergentdave
Copy link
Collaborator

One alternate resolution: we could keep using the PUT method, but change the URL to include the report ID (rather than it being part of the request body), i.e. PUT /{leader}/tasks/{task-id}/reports/{report-id}. (This was previously considered during the recent HTTP API redesign, but we ultimately reconsidered)

@simon-friedberger
Copy link
Contributor

@divergentdave 's suggestion seems fine but I don't think we're getting anything out of the visibly idempotent behavior so we could just use POST. I would prefer this because

successful PUT of a given representation would suggest that a subsequent GET on that same target resource will result in an equivalent representation being sent in a 200 (OK) response.

Which doesn't really make sense for report submission. It would also make more sense to use POST for job creation.

@cjpatton
Copy link
Collaborator

cjpatton commented Nov 7, 2023

This issue was discussed at IETF 118 but we did not reach a conclusion. The proposal was to keep the PUT, but add the report ID to the the resource URI. @mnot pointed out at the mic that "resources" always need a GET, which suggested to us that our attempts to define a "resource-oriented" API for DAP may have fallen short. I suggest we wait for @mnot's review of the draft before we take action here.

@mnot
Copy link

mnot commented Nov 17, 2023

Probably best to request a httpdir review.

@branlwyd
Copy link
Collaborator

#532, once it lands, will remove the last reason (AFAIK) for collection job polling to use a POST rather than a GET. That PR will leave the POST in place for now, but collection job polling semantics may be worthy of review by the httpdir folks.

@tgeoghegan
Copy link
Collaborator

We changed the request method for collection jobs in #544 and for report uploads in #543. I don't think there's anything else to do here.

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

No branches or pull requests

8 participants