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

Add additionalResponses to Form #1042

Merged
merged 9 commits into from
Feb 24, 2021
Merged

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented Feb 3, 2021

In order to handle error responses (and possibly other forms of "alternative" responses, for instance, using different contentTypes) add "additionalResponses" which effectively extends "response" into an array (with "response" identifying the "main", "normal" or "successful" response; having both "response" and "additionalResponses" maintains backward compatibility, as discussed).

Ontology, content, and JSON schemas all updated.

However, not quite done, since we need

  1. An example, ideally showing how to use protocol-specific error codes to distinguish responses (some examples are provided in the issues below, they just need to be tweaked a bit)
  2. Consideration of the use case provided by directories. It bothers me a bit that a single data schema is used for all responses for example; what if I want different payload structures but the same content type (e.g. JSON) for different return codes?

At any rate, this is a starting point.

Related issues:


Preview | Diff

@mmccool mmccool marked this pull request as draft February 3, 2021 01:45
@relu91
Copy link
Member

relu91 commented Feb 3, 2021

Consideration of the use case provided by directories. It bothers me a bit that a single data schema is used for all responses for example; what if I want different payload structures but the same content type (e.g. JSON) for different return codes?

Yes, that's my only concern with this proposal. A naive solution would be to add a field schema in the ExpectedResponse. Also, I would not mention "different contentTypes" as a use case for this feature. It is a little bit confusing since, if I am not wrong, multiple content types can also be conveyed as follows:

{
  // properties.someproperty
  // ....
  forms: [{
      href: "uri/to/pro",
      contentType: "application/json"
  },{
      href: "uri/to/pro",
      contentType: "application/xml"
  }
  ]
  //
}

@sebastiankb sebastiankb changed the title Add additionalResponses to Form WIP: Add additionalResponses to Form Feb 3, 2021
@sebastiankb
Copy link
Contributor

sebastiankb commented Feb 3, 2021

From today' TDs call:

  • this PR is still in wip
  • we need an assertion to clarify that when an additionalResponse is used there should be a also a response
  • in the forms section we should also provide some examples
  • we need also good clarification about the error responses. @egekorkan provides an example:
[{
{"success":{"/lights/1/state/bri":200}},     
{"success":{"/lights/1/state/on":true}},     
{"success":{"/lights/1/state/hue":50000}} 
}]

Philipps Hue provides the errors in that way:

[  { 
"error": {             
 "type": 7,            
  "address": "/lights/1/state/on",             
 "description": "invalid value, 123,, for parameter, on"         
 } }]
  • please provide your view and share your comments

@mmccool
Copy link
Contributor Author

mmccool commented Feb 17, 2021

Added an issue about adding DataSchema to ExpectedResponse: #1053

One issue is that ExpectedResponse is also used for "response", where we probably don't want to add a data schema (it would be redundant with e.g. the output schema in actions). So I probably need to create a subclass of ExpectedResponse, etc. etc. In other words, doable, but some ontology bashing needed.

@relu91
Copy link
Member

relu91 commented Feb 17, 2021

It might worth introducing a mechanism that can state which one among the additionalResposes should be treated as an error or a success response. Scripting runtimes could behave accordingly and throw the response as an Error.

In today's call, we discussed a possible boolean property, isError default set to true.

@mmccool
Copy link
Contributor Author

mmccool commented Feb 24, 2021

Spec itself is ready to merge, but still needs an example; to discuss.
Notes:

  1. Data schema can be specified using the keyword "schema". Other options? Note that "data" and "output" are used elsewhere.
  2. contentType is just a singleton, but seems to be an array in Forms and response. Should contentType in additionalResponses also be an array?
  3. added a flag to indicate whether or not an "additionalResponse" should be considered an error, but used the word "success" so the default value of "false" could be consistent with other default values. Also, "success" is most likely the rarer state.

@mmccool mmccool marked this pull request as ready for review February 24, 2021 03:30
@mmccool mmccool changed the title WIP: Add additionalResponses to Form Add additionalResponses to Form Feb 24, 2021
@mmccool
Copy link
Contributor Author

mmccool commented Feb 24, 2021

Small update to resolve point 2, making contentType an array, consistent with its use elsewhere. Note however, that unlike in ExpectedResponse, in AdditionalExpectedResponse it is optional.

@mmccool
Copy link
Contributor Author

mmccool commented Feb 24, 2021

  • Also need an assertion to state that if "additionalResponses" is used there should also be a "response". Or should there be? In the case of a property, for example, if the "default" response is correct (same contentType and data format as the input), and "additionalResponses" covers errors, it seems like it would be reasonable to use "additionalResponses" without "response". In other words, "additionalResponses" is not just the "tail" of the responses array, it's a slightly different thing (e.g. "unusual" responses).

@mmccool
Copy link
Contributor Author

mmccool commented Feb 24, 2021

In minute from last meeting just noticed a request to add some text about when using "oneOf" is appropriate. I also don't mention "success" in the text.
Suggest we merge this PR and I can deal with this and an example in another PR.

Base automatically changed from master to main February 24, 2021 15:22
@sebastiankb
Copy link
Contributor

From today's TD call we will merge this PR. A editor note will be provided by @mmccool to point out that schema is still evaluated and if there is an alternative solution.

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