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

Support the PATCH HTTP method #500

Open
sfackler opened this issue Feb 5, 2020 · 8 comments
Open

Support the PATCH HTTP method #500

sfackler opened this issue Feb 5, 2020 · 8 comments

Comments

@sfackler
Copy link
Member

sfackler commented Feb 5, 2020

Currently, the only supported HTTP methods are GET, PUT, DELETE, and POST. It would be nice to support PATCH as defined in RFC 5789.

@uschi2000
Copy link
Contributor

@sfackler , can you say why this would be nice?

@nmiyake
Copy link
Contributor

nmiyake commented Feb 5, 2020

Besides motivation, there would also be some work in speccing this out. Would Conjure also define the format for the JSON patch? The "op" and "path" fields of a standard patch are defined as strings, but he "value" is basically any JSON value/is dependent on the document type. We would need to figure out how to define the type of the patch and recommend behavior that can be portable across codegen.

I believe all of the above things are possible, but it would probably be a decent amount of work, so we would definitely want to get some consensus around what pain this would solve and whether it would be worth the extra work to make it happen.

@carterkozak
Copy link
Contributor

To build on Nick's point, PATCH functionality is fairly specific to http where a goal of conjure is to provide separation between the API system and the details surrounding the implementation. I'm not sure there's enough upside for this to be worth the trade-off, but I'm open to the possibility that there are needs I'm not aware of.

@sfackler
Copy link
Member Author

sfackler commented Feb 5, 2020

The motivation is that we have endpoints on an internal service that don't behave like a POST or PUT, but more like a PATCH.

Why would Conjure define the format for a PATCH request body any more than it would define the format for a POST or PUT body? The RFC itself doesn't mention any format for the body beyond this:

With PATCH, however, the enclosed entity contains a set of instructions describing how a resource currently residing on the origin server should be modified to produce a new version.

I would expect that a non-HTTP protocol like gRPC would ignore the HTTP method entirely, and the introduction of PATCH would be irrelevant.

@carterkozak
Copy link
Contributor

I would expect that a non-HTTP protocol like gRPC would ignore the HTTP method entirely, and the introduction of PATCH would be irrelevant.

In that case, what is the benefit of adding PATCH to Conjure?

@sfackler
Copy link
Member Author

sfackler commented Feb 5, 2020

I don't understand your question. There is no non-HTTP target for Conjure at the current time AFAIK.

@nmiyake
Copy link
Contributor

nmiyake commented Feb 5, 2020

Ah I see -- since the HTTP PATCH RFC doesn't dictate the content type of a patch, we could do a "pure Conjure" implementation simply by letting you specify the body param as we do now. Then it would be up to the owner of the Conjure spec to define a Conjure type accepted and interpreted by the "PATCH" endpoint. In this world, yes, it would be as simple as just adding "PATCH" as a verb and supporting that registration on the client and server.

However, this does seem a bit non-standard -- to my knowledge, PATCH endpoints are often expected to accept JSON patches in the form of RFC 6902 with a content media type of "application/json-patch+json". Adding generation support for this model is what I described as likely more difficult.

I'm a bit on the fence here -- in the simple model, I understand how adding the verb could make the endpoint reflect the intent better, and it's easy to implement. However, it does delegate all of the work of defining the object and how the patch is handled semantically to each implementation. Since part of the goal of Conjure is to be opinionated and push people towards the right patterns, I kind of feel like if we add PATCH support, it should be opinionated and mirror the more standard JSON patch convention (and provide the code generation/practices to make it easy for implementors to do this), but do realize this may be hard as well.

@sfackler
Copy link
Member Author

sfackler commented Feb 5, 2020

JSON patch was definitely designed with the PATCH HTTP method in mind, but I disagree that it's the canonical thing to use with it. Its RFC says that it "is suitable for use with the HTTP PATCH method", not "all HTTP endpoints using the PATCH method SHOULD/MUST use JSON patches". I don't see there being a standard body type for PATCH any more than there's a standard body type for POST.

In particular, I don't think the use case I ran into with the service I work on can even be described as a JSON patch! The operation I implemented was "atomically ensure that the list of unique strings on the server contains all of these strings, adding to it if necessary". The closest thing to that with a JSON patch (as far as I know) is the "add" op, which doesn't handle the only-if-absent requirement. Even if you fold it into a JSON patch, it seems a lot cleaner IMO to just have a Conjure union of all of the patches you may want to make to a given resource.

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

4 participants