Skip to content

Conversation

@appsdesh
Copy link
Contributor

@appsdesh appsdesh commented Apr 2, 2025

A follow-up from #247

@jischr please take a look

A follow up from openid#247
@appsdesh appsdesh requested a review from a team as a code owner April 2, 2025 00:46
Copy link
Contributor

@jischr jischr left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for adding this to the table!

@iamseanodentity
Copy link
Contributor

Returning a 400 if the delivery method is not supported. I looked through the 4xx error codes. Can we suggest providing a reason phrase with the code to delineate why it's a bad request? Thoughts @jischr

@tulshi
Copy link
Contributor

tulshi commented Apr 21, 2025

Returning a 400 if the delivery method is not supported. I looked through the 4xx error codes. Can we suggest providing a reason phrase with the code to delineate why it's a bad request? Thoughts @jischr

So this is a bug in the current spec?

@gffletch
Copy link

gffletch commented Apr 21, 2025

Would using HTTP status code 406 be better or applicable?

406 - Not Acceptable
When making a request, the client can indicate to the web server what kind if data it will accept back.
The header with the 406 error code is returned if the web server detects that the only response it can generate and return to the client is not acceptable by the client. This error occurs very rarely with web browsers, because most clients accept any data returned from the server.

@tulshi tulshi added the vFuture label Apr 24, 2025
@tulshi
Copy link
Contributor

tulshi commented Apr 24, 2025

The current language says "errors are reported...", which makes the current behavior the equivalent of a "MUST", changing this will break backward compatibility. There is no evidence right now that this is an implementation blocker, so we can review this after v1Final.

@appsdesh
Copy link
Contributor Author

The current language says "errors are reported...", which makes the current behavior the equivalent of a "MUST", changing this will break backward compatibility. There is no evidence right now that this is an implementation blocker, so we can review this after v1Final.

Not sure I understand your comment @atultulshi , this PR was to bring alignment of the text which describes the error and the table which lists these scenarios.

@tulshi
Copy link
Contributor

tulshi commented Apr 24, 2025

The current language says "errors are reported...", which makes the current behavior the equivalent of a "MUST", changing this will break backward compatibility. There is no evidence right now that this is an implementation blocker, so we can review this after v1Final.

Not sure I understand your comment @atultulshi , this PR was to bring alignment of the text which describes the error and the table which lists these scenarios.

Good point. I think in the discussion around this PR we confused the topics of requesting verification events when the stream was paused or disabled. We decided to keep that one out of the scope of v1Final. I won't be there, but you can discuss this PR separately in the meeting on 4/29.

@FragLegs
Copy link
Contributor

@appsdesh The text says that the Transmitter MAY send the 400 code if the delivery method is not available. But the intro to the table says "Errors are signaled" which should be understood more like a MUST. Adding the delivery unavailability to the table would break backwards compatibility.

The solution might be to go through the spec and reword the error tables so that they say MAY in the intro. But that seems contentious enough to push off until after v1.

@appsdesh
Copy link
Contributor Author

@appsdesh The text says that the Transmitter MAY send the 400 code if the delivery method is not available. But the intro to the table says "Errors are signaled" which should be understood more like a MUST. Adding the delivery unavailability to the table would break backwards compatibility.

The solution might be to go through the spec and reword the error tables so that they say MAY in the intro. But that seems contentious enough to push off until after v1.

We are creating two sources of truth for the error codes, one in the table and another buried in the description. I understand MAY vs MUST, but we are likely making it difficult for the implementers to see a list of all expected/suggested errors. We should align on this holistically for v1 and not punt it.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants