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

"method" does not constrain HTTP methods #276

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Contributor

"method" already documented that "method": "get" does not constrain
the LDO to GET. Clarify that "post" also does not constrain the
LDO to HTTP POST, and that the LDO can be used with multiple methods.

The "schema" and "targetSchema" keywords are used with relevant
HTTP methods regardless of the presence, absence, or value of
JSON Hyper-Schema LDO "method". This is consistent with the
existing wording of both "schema" and "targetSchema" around the
construction of PUT requests.

"method" already documented that "method": "get" does not constrain
the LDO to GET.  Clarify that "post" also does not constrain the
LDO to HTTP POST, and that the LDO can be used with multiple methods.

The "schema" and "targetSchema" keywords are used with relevant
HTTP methods regardless of the presence, absence, or value of
JSON Hyper-Schema LDO "method".  This is consistent with the
existing wording of both "schema" and "targetSchema" around the
construction of PUT requests.
@handrews
Copy link
Contributor Author

@awwright @Relequestual I believe this is just a clarification of the existing intent in both draft-wright-json-schema-hyperschema-00 (Draft 05) and draft-wright-json-schema-hyperschema-01 (Draft 06). I'm adding it based on a conversation with @jdesrosiers about interpretation. As a clarification, I think it is appropriate to add to Draft 06 before publication.

On the other hand, if I have this wrong and this would be a change, then it should be deferred to the next draft.

@handrews handrews requested a review from awwright March 20, 2017 18:14
@handrews handrews added this to the draft-next (draft-6) milestone Mar 20, 2017
@jdesrosiers
Copy link
Member

This is most definitely a change. The stuff about "method": "post" not imposing method constraints is good. It looks like we only said that explicitly for "method": "get", but it definitely applies to "method": "post" as well.

The part I have issue with is the statement that you can choose to ignore schema if you want to use that link with a method that isn't defined for an arbitrary request entity. In HTTP's uniform interface, the only method that is compatible with this type of link is POST. This might not be true of a different uniform interface, which is why this paragraph is not contradictory to the previous paragraph. A method should only apply to a link if the method is compatible with the link's structure. It should not alter the link's structure to be compatible with the method.

Since draft-05, the spec explicitly compares the behavior of LDO to HTML Forms. As I understand it, HTML Forms work the way I just described. method="post" doesn't impose the use of POST, but a user-agent that implements HTML Forms knows to use POST when the the protocol is HTTP because it is the only HTTP method that is compatible. It wouldn't make sense if you encountered a form and decided you wanted to ignore the input fields and make a PUT request with it's URI.

@handrews, I think your interpretation is at best contrary to the spirit of the documented symmetry between LDO and HTML Forms. Even if we remove the HTML Form wording, I still think it's the wrong approach. In any case, it is far too controversial to include as a last minute change.

@handrews
Copy link
Contributor Author

@jdesrosiers I am going from @awwright's statements about how and why he changed things between Draft 04 and Draft 05 so please let's wait for him to weigh in. He has made numerous comments that I believe indicate that value of "method" and the choice of HTTP method are completely orthogonal. In particular, from

#48 (comment)

Links are used for learning about new resources, so regardless of what JSON Hyper-schema defines, defining links multiple times doesn't achieve any additional effect for hypermedia purposes.

(in the context of me showing a format that repeated links for each HTTP method, so this is exactly the situation currently under discussion).

and #48 (comment)

You don't need to tell a client how to GET a resource or how to PATCH a resource. It's something both client and server already understand because of the uniform interface.

I'm also going to point out again that definitions for both "schema" and "targetSchema" explicitly separate how PUT is used from POST, and explicitly note that "schema' does not apply to PUT. "targetSchema" is not the schema for a response, unless the response happens to be a representation (e.g. HTTP GET, or an HTTP response with the "Content-Location" header identical to the request URI).

I understand that you disagree with this, but I think your disagreement is with the change that was made in Draft 05, and that ship sailed six months ago. Right or wrong, my concern for Draft 06 is providing clear guidance on what was already decided in Draft 05.

@handrews
Copy link
Contributor Author

handrews commented Mar 20, 2017

In any event, this needs clarifying before we can ship Draft 06. It's obviously not usable in its current form, since just two people have come up with two incompatible readings of the spec as it stands.

@Relequestual
Copy link
Member

"method" already documented that "method": "get" does not constrain the LDO to GET...

I actually had no idea that change took place in the first instance. Possibly I didn't understand it at the time, I don't know. Either way, I'd like time to review this.

@Relequestual Relequestual self-requested a review March 21, 2017 09:44
@handrews
Copy link
Contributor Author

I'd like time to review this.

well of course, that's why it's posted as a pr

@Relequestual
Copy link
Member

I was more letting you know I've seen it and will come back to it soon =]

@Relequestual
Copy link
Member

I see there was some discussion at #94

I totally have used links as a methods list and for my CRUD actions.
The definition of method in the current state of the draft seems to give it no use. If it doesn't define the HTTP method the link should use, then what's the point of that key word? What's the use case?

I couldn't find an associated pull request where method was redefined.

Retrospectivly speaking, I'm strongly opposed to this change. I will conceed that I may not fully understand everything at play here.

I think I'm expecting to know all the "methods" I can perform on an object via LDOs; a sort of API definition. If the intension is that should not be the case, and I should not be able to know the full API definition from the schema document alone, and in stead I should do a GET first and then based on Allows header I should know what I can do, then that's different from what I'm expecting.

Given how totally baffled and confused I am by this change, I can't be the only one... right... anyone? =/

@handrews
Copy link
Contributor Author

handrews commented Mar 22, 2017

@Relequestual I have to confess some frustration here. A pull request clarifying a point on the way out the door for a release is not the right place to re-litigate a decision from many months ago. I'm sorry you did not notice it, but in fact it has seen a great deal of discussion. #96 in particular comes to mind (which @jdesrosiers filed shortly after Draft 05 went out), and of course there was a great deal of discussion in #48 (both before and after Draft 05, although never in the form of a proposal- just philosophical discussion around how things should work).

There is a very important procedural principle at work here: PRs must be evaluated with respect to the current state of things, not to where you wanted them to be six months ago. If you want to block the release of Draft 06 on this topic, then you need to file an issue expressing your concern and proposing a solution. Don't turn a PR into a new issue.

@Relequestual
Copy link
Member

@handrews Sorry, you're right. As such, I'll migrate my comments unrelated to this PR to a new issue for draft-7. I understand "that ship has sailed".

As I'm still not set I understand intension of method or other changes made in Hyper Schema, I don't feel qualified to review this PR, but I am confident in your understanding and ability to judge.

To clarify: I don't have any issues with the changes you suggest, but I'm also not sure my vote counts.

@handrews
Copy link
Contributor Author

As I'm still not set I understand intension of method or other changes made in Hyper Schema, I don't feel qualified to review this PR, but I am confident in your understanding and ability to judge.

As you can see in #96 and #48 you are far from alone. That is why I am trying to get awwright to clarify his intention (with either my wording or his- I don't care one way or the other).

To clarify: I don't have any issues with the changes you suggest, but I'm also not sure my vote counts.

I'm not trying to discourage anyone from commenting, or setting some sort of threshold for whose vote "counts". I just want someone, anyone to attempt to evaluate this PR on its own intent.

Since that has not happened I'm going to close this and re-submit it so that anyone who does attempt to look at this won't get discouraged and put off by the long thread. I will summarize and contextualize the concerns raised here in the new PR.

@handrews handrews closed this Mar 22, 2017
@handrews
Copy link
Contributor Author

It's been brought to my attention that I am unfairly taking some frustration out on @Relequestual and @jdesrosiers, and I'd like to apologize for that.

I'm leaving this closed because I do think it's easier to evaluate the change without this thread, but my stronger comments here were inappropriate.

@jdesrosiers
Copy link
Member

@handrews, I'm sorry you thought my comments were off topic. We certainly went out of the scope of this PR in our full discussion, but we did that in a different thread. I've copied my relevant comments to the new pull request with edits to help make it more clear why I think the current draft supports my interpretation.

@handrews
Copy link
Contributor Author

@jdesrosiers thanks for reworking things into the new PR. I definitely overreacted, for my part.

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

Successfully merging this pull request may close these issues.

3 participants