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

PUT is a full replacement, not a partial one #445

Open
steveklabnik opened this issue Dec 17, 2013 · 12 comments
Open

PUT is a full replacement, not a partial one #445

steveklabnik opened this issue Dec 17, 2013 · 12 comments

Comments

@steveklabnik
Copy link
Contributor

HTTP Spec nerd-ery time: we're doing PUT wrong. https://docs.balancedpayments.com/1.1/api/cards/?language=bash#update-a-card

This should either be PATCH or POST.

@mahmoudimus
Copy link
Contributor

We support PATCH, let's change it to use that.

@mjallday
Copy link
Contributor

One thing to note with this is that it's not so simple to formulate a PATCH payload for rev 1.1

E.g.

PUT

{"field": "value"}

PATCH

{
    "op": "replace",
    "path": "/cards/0/links/customer",
    "value": "{customer,customers.id}"
}

Does PATCH support a simpler syntax for ad-hoc examples? This would be good for CURL examples.

(I concede that since client libraries do the heavy lifting this may be a non-issue).

@mahmoudimus
Copy link
Contributor

The normie use case is the simplicity of the curl command. I strongly urge that we do not deviate from simple curl commands that people can paste in their terminals.

@steveklabnik
Copy link
Contributor Author

PATCH supports 'a diff media type.' The details don't matter. Technically, JSON API uses JSON PATCH with PATCH requests currently. That said, PATCH fudging it with the type is better than PUT being straight-up wrong, so it's fine to not be mega ultra orthodox here...

@mahmoudimus
Copy link
Contributor

@steveklabnik why not use the diff media type? I want to use hypermedia as much as possible - our internal python frameworks should be updated to handle these cases!

@steveklabnik
Copy link
Contributor Author

The only diff type that currently exists is JSON PATCH, which has the problem you're talking about with cURL.

Someone is/was working on getting text/diff for the output of the diff command, and I was working on https://github.com/steveklabnik/json-merge_patch ... and I was talking with some people about adding an extension to application/www-form-encoded to have diff semantics, which would be the way that the current cURL command works.

TL;DR: it's complicated. That's why I suggested POST.

@mahmoudimus
Copy link
Contributor

@steveklabnik So our issue is:

curl -u ${KEY}: https://api.balancedpayments.com/cards/:card_id \
      -X PATCH \
      -d "op=replace" \
      -d "path=/cards/0/links/customer" \
      -d "value={customer,customer.id}"

vs

curl -u ${KEY}: https://api.balancedpayments.com/cards/:card_id \
      -X POST \
      -d "customer=customer.id"

?

@steveklabnik
Copy link
Contributor Author

almost. the PATCH would be

curl -u ${KEY}: https://api.balancedpayments.com/cards/:card_id \
      -X PATCH \
      -d '{ "op": "replace", "path": "/cards/0/links/customer", "value": "{customer,customer.id}"'

You gotta send JSON, not application/www-form-encoded.

@mahmoudimus
Copy link
Contributor

@steveklabnik should we introduce our own application/www-form-encoded-diff media type then? The intention is for easier usage via curl commands.

Ideally, you'd pass in an accept header though:

curl -u ${KEY}: https://api.balancedpayments.com/cards/:card_id \
      -H 'Accept: application/www-url-encoded-patch'
      -X PATCH \
      -d "op[0]=replace" \
      -d "path[0]=/cards/0/links/customer" \
      -d "value[0]={customer,customer.id}"

@steveklabnik
Copy link
Contributor Author

It would have to be application/vnd.balanced+patch, but yes, we could absolutely do that. I think that'd be a great step towards an RFC to apply these semantics for application/www-url-encoded. I have a private email that enumerates some of the issues that would need to be addressed there.

@mjallday
Copy link
Contributor

If we did that can we document the content type behavior as not supporting array updates and always having a default operation of replace? E.g. make it so that it behaves the same as the partial PUT that we support now?

curl -u ${KEY}: https://api.balancedpayments.com/cards/:card_id \
      -H 'Accept: application/www-url-encoded-patch'
      -X PATCH \
      -d "property=value"

@steveklabnik
Copy link
Contributor Author

Yes, we could define it however we wanted.

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

3 participants