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
Content types: optional tree prefixes and parameters, split req/res content negotiation #329
Conversation
5a6a7a4
to
c128ca9
Compare
I see you updated the rules for this in TD Server for HTTP. If we also accept PR #334, which supports use of CoAP for TD Servers, we need equivalent updates for CoAP's Content-Format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a little more discussion, especially around the wording of the assertion with "contains" for application/json. I think it is technically correct, but a little confusing.
</span> | ||
A successful response from an HTTP-based TD Server providing a <a>TD</a> | ||
MUST have 200 (OK) status and the <a>TD</a> in the body.</span> | ||
<span class="rfc2119-assertion" id="service-http-resp-content-type"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that breaking these into multiple assertions is a good idea.
A successful response from an HTTP-based TD Server providing a <a>TD</a> | ||
MUST have 200 (OK) status and the <a>TD</a> in the body.</span> | ||
<span class="rfc2119-assertion" id="service-http-resp-content-type"> | ||
A successful response with JSON serialization MUST contain `application/json` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of "contain" is a bit strange here. I can understand it if I interpret the content-types and defining sets, and td+json is a subset of json, but perhaps there is a better word than "contain" to make this clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead be more explicit and say "MAY" and "SHOULD" for the various options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content-Type can be for e.g. application/td+json; charset=utf-8
and include any other parameters. The assertion mandates containing the type and leaves it open for adding parameters.
Note that "contain" was already there and this PR isn't adding that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, regarding #315! thanks. About the content-type set to application/json I would probably need some clarification today. I thought Ben's feedback was about allowing clients to request application/json in the Accept header not giving the possibility to arbitrary return application/json or application/td+json.
IMHO this assertion should already cover the WebThings gateway requirements:
An HTTP-based TD Server providing a TD MAY provide alternative representations through server-driven content negotiation, that is by honoring the request's Accept and Accept-Encoding headers and responding with the supported TD serialization and equivalent Content-Type and Content-Encoding headers
Isn't?
This PR is addressing another feedback from Ben:
|
propose merging, follow up with fixes in #341 |
td
/ld
tree prefixes (related to Feedback on latest Editor's Draft #299). This effectively allows usingapplication/json
instead ofapplication/td+json
andapplication/ld+json
.application/td+json
/application/ld+json
as mandatory Content Type, so this isn't a technically a new requirement, but rather to clarify the spec.application/json
content types are already UTF-8 by specification. This PR makes it possible to explicitly specify that the charset is UTF-8 or something else.Preview | Diff