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

Make contentType optional in ExpectedResponse and AdditionalExpectedResponse classes #2081

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Feb 14, 2025

Description of Changes

This PR deals with an issue that was discovered in the context of w3c/wot-discovery#465 and #1776: At the moment, there is always a contentType assumed for an ExpectedResponse or AdditionalExpectedResponse, although there are cases where a server does not return any kind of payload (e.g., in the case of an HTTP No Content response). This PR provides a potential fix for the issue, although there might be some more discussion needed here.

Related Issue

Closes #1780

Type of Change

Check the correct box and add the corresponding label if you are an editor.


Preview | Diff

@JKRhb JKRhb changed the title Make contentType optional in ExpectedResponse class Make contentType optional in ExpectedResponse and AdditionalExpectedResponse classes Feb 14, 2025
@JKRhb JKRhb requested a review from benfrancis February 14, 2025 14:22
@JKRhb JKRhb marked this pull request as ready for review February 14, 2025 14:22
@benfrancis benfrancis requested a review from egekorkan February 14, 2025 14:32
@JKRhb
Copy link
Member Author

JKRhb commented Feb 16, 2025

I noticed in the meantime that there are also assertions related to the ExpectedResponse and AdditionalExpectedResponse classes (e.g., in section 5.3.4.2.2) that need to be adjusted. I will update this PR accordingly.

@egekorkan
Copy link
Contributor

I am fine with this direction given the use case. However, we are not clear about the meaning of the lack of contentType (a bit like #2028 but not exactly). Here, we want to really say that there is no content type in the response header. However, the lack of contentType in the form level means that the defaults should be assumed. Maybe both meanings can be changed at the same time, i.e.:

  • contentType is optional in all levels without any default mechanism
  • Lack of it means contentType SHOULD NOT be used in the request or response

@JKRhb
Copy link
Member Author

JKRhb commented Feb 17, 2025

I've now made a first attempt at adjusting the assertions for the two classes. I have the impression that this requires a bit more care than initially expected 😅 But I hope that the changes I made so far point in the right direction. One area that also needs to be revisited carefully is the table on the content type usage, which I haven't taken a closer look at so far.

As there is no default value defined, the content type of additional
responses MUST be explicitly specified.
</span>
That means that the lack of a content type value implies that the
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be more specific/assertive here: When there is payload in the response, the contentType field MUST be omitted from the expected response.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would allow us to test both cases easily. Having contentType or not

Copy link
Member Author

Choose a reason for hiding this comment

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

When there is payload in the response, the contentType field MUST be omitted from the expected response.

Hmm, do you mean when there is no payload, the field must be omitted? That would sound good to me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes... A very well placed "typo" from my side :D

@benfrancis
Copy link
Member

@egekorkan wrote:

contentType is optional in all levels without any default mechanism

That sounds like a big change. Would that mean that all forms which previously defaulted to application/json would now need an explicit contentType?

@JKRhb
Copy link
Member Author

JKRhb commented Feb 17, 2025

That sounds like a big change. Would that mean that all forms which previously defaulted to application/json would now need an explicit contentType?

Hmm, I think by default, we could stick to application/json as the default content type for input and output data, the latter of which could be overridden via the response classes. One thing, though, I am wondering about just now is whether we also need a way to express that a request should be empty/not have a content type. Maybe, though, we could also use null to specify that?

@egekorkan
Copy link
Contributor

@egekorkan wrote:

contentType is optional in all levels without any default mechanism

That sounds like a big change. Would that mean that all forms which previously defaulted to application/json would now need an explicit contentType?

Not a big change in my opinion. We will have a container to define defaults for all forms anyways. Also see w3c/wot-binding-templates#357 (comment) and the comments below it

@egekorkan
Copy link
Contributor

egekorkan commented Feb 20, 2025

TD call 20 Feb:

  • Some more changes are needed to the assertions around this feature
  • The contentType in the form level (requests) can be optional as well, which will be useful for cases where the request requires no payload (e.g. an action invoke without input such as toggling a light)
  • This will be a breaking change as TD2.0 TDs not having the contentType will mean JSON for TD 1.1 Consumers
  • This can be a non-issue for some protocols that have automatic mechanisms
  • It should be possible to require it at the binding level. Some protocols do not have a mechanism to provide this information in-band. So it has to be out-of-band via TD.
  • The discussion (BACnet media type wot-binding-templates#357) should be taken into account, where only the bindings define the keyword related to contentType
  • We should check the impact to Scripting API algorithms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make contentType optional in ExpectedResponse class
3 participants