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

New Protocol Binding section #77

Merged
merged 6 commits into from
May 27, 2021
Merged

Conversation

benfrancis
Copy link
Member

@benfrancis benfrancis commented Apr 22, 2021

This PR includes two commits:

  1. A new structure for the Protocol Binding section, as discussed in Refine Goals and Scope #73
  2. Proposed text to define a protocol binding for the first set of operations for which we already have broad agreement (readproperty, writeproperty, readallproperties, writeallproperties, readmultipleproperties, writemultipleproperties and invokeaction) and notes suggesting other operations which may also be included upon further discussion

Preview | Diff

Copy link
Contributor

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just struggled over the case that PUT needs to return an actual response in the body.

index.html Outdated
Comment on lines 1579 to 1580
<li>A body with the new value of the property serialized in JSON
</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really best practice that PUT returns a value?
I would have said that a PUT response should not return any body value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing in the HTTP specification that says a response to a PUT request shouldn't contain a body. It just says:

If the target resource does have a current representation and that representation is successfully modified in accordance with the state of the enclosed representation, then the origin server MUST send either a 200 (OK) or a 204 (No Content) response to indicate successful completion of the request.

A 204 response obviously shouldn't contain a body, but a 200 OK can. It's pretty common to return a body in HTTP-based APIs in my experience, but it probably comes down to taste. See this post on StackOverflow for some of the pros and cons.

Pro:

  • Provides an extra level of validation that the operation was successful, by echoing back the new state of the property

Con:

  • Requires extra bandwidth to echo back information that the Consumer probably already knows

Note that an empty body is not valid JSON, so if the body is going to be empty implementors also need to make sure that the Content-Type header is not set to application/json. The same would be true if we use DELETE requests anywhere in the protocol binding though (e.g. to cancel an ongoing action).

Since the body is not strictly necessary in this particular case, I'd be fine with changing the response to 204 if there's a consensus around that?

What about PATCH requests though? If in future we support PATCH for partial updates of properties, would that return a body?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MindSphere is following the same approach, e.g., as shown here. What is ECHONET or Oracle Cloud doing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, Oracle also returns a body similar to what MindSphere does. I personally do not like these since these are subprotocols put into HTTP and do not really do things the REST way.

Regarding the current proposal to include the written value in the response, this again creates some sort of a subprotocol, i.e. I need to know that the value I got is the value I have written to the resource so that I can do something with it (validation I guess). Also, there is no way to indicate this properly in a TD for properties. The TD below would not change if the Thing returns such a response for a writeproperty operation:

{
    "@context": "http://www.w3.org/ns/td",
    "title": "MyLampThing",
    "properties": {
        "status": {
            "type": "string",
            "readOnly":false,
            "forms": [{"href": "https://mylamp.example.com/status"}]
        }
    },
}

To go further, this was already discussed in an issue (could not find the number now but @danielpeintner had opened it I think ) about specifying return values for writeproperty operation and in the call it was decided that this would not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the issue, w3c/wot-thing-description#875

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egekorkan OK, as I said I don't mind changing the response to a PUT request to being a 204 with no content, though it sounds like existing implementations do consistently return a body.

With regards to defining a sub-protocol over HTTP, that's exactly what I expect the "Core Profile" to do, by defining enough defaults that a Consumer to take a very simple Thing Description and interpret it as a much more complex (canonical) Thing Description with a rich set of interactions.

I think this will become particularly apparent when we come to defining the protocol binding for actions, which ideally could take the full set of interactions you proposed in w3c/wot-thing-description#302 (comment) (invokeaction, queryaction, updateaction, cancelaction) and express them in a very simple form in a Thing Description, which the Consumer would then apply a set of defaults to to arrive at a much more complex canonical Thing Description describing a rich set of interactions.

I think this is in the spirit of the current set of defaults defined in section 8.3.1 of the Thing Description specification.

Copy link
Contributor

@sebastiankb sebastiankb May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From today's TD call:

  • We had a look into ECHONET and we saw that ECHONET also echo the payload when the PUT method is applied (please see the slide here, slide 20)
  • also this is true for OCF (there POST is used for writing a property)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @sebastiankb, this does seem quite common.

For reference, in the last Architecture call the most compelling reason to change to a 204 (No Content) response seemed to be the case where a property value payload is very large, e.g. for image or video properties.

index.html Outdated
Comment on lines 1716 to 1717
<li>A body with the new values of the properties serialized in
JSON, as an object keyed by property name</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my previous comment. PUT and an actual response?

index.html Outdated
Comment on lines 1860 to 1861
<li>A body with the new values of the properties serialized in
JSON, as an object keyed by property name</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet again the PUT response question..

@egekorkan
Copy link
Contributor

I have added my comment above regarding PUT response. Some other feedback:

  • For read/write X properties, what happens to non-readable or writeable properties? I think this should be mentioned in the text, i.e. Thing returns the readable properties
  • For readmultipleproperties , it is stated that the properties of interest should be in the payload. This goes against the best practices however the only other way would be in the URI, which sort of makes sense, like doing a google search query with multiple words.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a general comment about this PR. I like the way it blends well with what we are planning for protocol bindings specification. The protocol binding templates documents can explain and define generic terms and defaults for protocols while this profile section restricts usages and defines best practices. I would go on with this direction. 👍🏻

@benfrancis
Copy link
Member Author

benfrancis commented Apr 28, 2021

@egekorkan wrote:

  • For read/write X properties, what happens to non-readable or writeable properties? I think this should be mentioned in the text, i.e. Thing returns the readable properties

OK, I've added some additional wording which hopefully makes this more explicit?

  • For readmultipleproperties , it is stated that the properties of interest should be in the payload. This goes against the best practices however the only other way would be in the URI, which sort of makes sense, like doing a google search query with multiple words.

WebThings doesn't implement the equivalent of readmultipleproperties and I'm not convinced how useful it is, but I derived this payload from the wording in the Thing Description specification:

If not specified otherwise (e.g., through a TD Context Extension), the request data of the readmultipleproperties operation is an Array that contains the intended PropertyAffordances instance names, which is serialized to the content type specified by the Form instance.

I agree it's a bit unconventional to provide a body with a GET request, but as I understand it it's also not forbidden by the HTTP specification.

As I understand it RFC2616 did have some wording that alluded this, but it was obsoleted by RFC7231 which is more lenient in this respect and simply says:

A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.

I'm guessing in theory some proxy servers may discard the body of a GET request and/or a client may end up getting a cached response for a previous request which asked for a different set of property values.

I'm unsure about the idea of using the query string as that's inconsistent with other similar operations, but I'm happy to use that approach if there's a consensus around that? What do existing implementations of readmultipleproperties do?

@egekorkan
Copy link
Contributor

egekorkan commented Apr 28, 2021

@benfrancis wrote:

OK, I've added some additional wording which hopefully makes this more explicit?

Yes, this is perfect I think.

Thank you for finding more reasons on why supplying a payload in GET is bad. I have never tried deploying something in the web scale that uses GET payloads so I cannot speak on what happens in practice (would be interesting if someone has such experience).

Regarding its implementations, there are none as far as I know. The implementation report shows 2 implementations but I know only 1 which is node-wot which actually sends multiple requests on the client side, so a workaround but I think it was a misunderstanding at the time and an honest mistake. There is a related issue that I opened somewhere but I couldn't find it 😔

@benfrancis
Copy link
Member Author

Regarding its implementations, there are none as far as I know.

In that case I would also be happy to remove getmultipleproperties and setmultipleproperties from the protocol binding altogether if that is an acceptable solution?

@egekorkan
Copy link
Contributor

@benfrancis I would agree with the removal however this might require feedback from others

@benfrancis
Copy link
Member Author

@mlagally I think I have now addressed all three resolutions from yesterday's meeting.

There were other comments, e.g. regarding ignoring parts of payloads which don't conform to a data schema, but I think they can be addressed in follow-ups.

Please let me know what you think.

@mlagally
Copy link
Contributor

Arch call on May 27th:
Reviewed "actions" and "error codes" sections. Error codes need some minor tweaks, e.g. eliminate 426 response code.
Also the "teapot" should go away.
"Actions" need more work to define a simple model, need to discuss further whether a synchronous apporach is sufficient. Queues, cancellations etc. are hard.
Canonical TDs cause some verbosity issues, need to revisit.

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.

6 participants