Skip to content

OpenAPI: Use more clear language in recommending error responses #12376

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.

Sign up for GitHub

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

Merged
sungwy merged 2 commits into from
May 1, 2025
Merged

OpenAPI: Use more clear language in recommending error responses #12376

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c897b68
use more explicit language in which error response type should be used
sungwy Feb 22, 2025
7693c91
shorter language
sungwy Feb 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions open-api/rest-catalog-open-api.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4458,7 +4458,9 @@ components:
# The fields `message` and `type` as indicated here are not presently prescriptive.
UnauthorizedResponse:
description:
Unauthorized. Authentication is required and has failed or has not yet been provided.
Unauthorized. The REST Catalog SHOULD respond with the 401 UnauthorizedResponse when
the access token provided is expired, revoked, malformed, or invalid for other reasons.
Copy link
Contributor

@mrcnc mrcnc Mar 26, 2025

Choose a reason for hiding this comment

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

If an access token is malformed, wouldn't a 400 / BadRequestErrorResponse be a more appropriate response? I'm wondering if we should drop the malformed here?

Copy link
Contributor Author

@sungwy sungwy Mar 26, 2025

Choose a reason for hiding this comment

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

Hi @mrcnc thank you for the review!

IHMO I think if the access token is malformed, I'd still consider 401 to be the appropriate respones type vs 400 which I think would be more appropriate when the format of the Request itself is malformed.

I took this verbiage directly from https://www.rfc-editor.org/rfc/rfc6750 under the invalid_token section:

invalid_token
         The access token provided is expired, revoked, malformed, or
         invalid for other reasons.  The resource SHOULD respond with
         the HTTP 401 (Unauthorized) status code.  The client MAY
         request a new access token and retry the protected resource
         request.

The client MAY request a new access token and retry the request.
content:
application/json:
schema:
Expand Down Expand Up @@ -4566,7 +4568,9 @@ components:

AuthenticationTimeoutResponse:
description:
Credentials have timed out. If possible, the client should refresh credentials and retry.
This is an optional status response type that the REST Catalog can issue when the
token has expired. The client MAY request a new access token and retry the request.
401 UnauthorizedResponse SHOULD be preferred over this response type on token expiry.
Copy link
Contributor

@flyrain flyrain Feb 26, 2025

Choose a reason for hiding this comment

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

I like the new descriptions for both status codes. With the new changes, it seems we actually deprecated it without saying it explicitly. Should we just deprecate it then? cc @danielcweeks @rdblue

Copy link
Contributor Author

@sungwy sungwy Feb 27, 2025

Choose a reason for hiding this comment

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

Thank @flyrain !

Yeah I was actually debating on the last line, but decided to keep it to facilitate this discussion. I believe a deprecation would result in us removing the status code in a subsequent release, after which the status code handling could also be removed from the Iceberg REST client representations which I think may cause confusion as 419 responses from Iceberg REST Catalog servers that are running on older versions of the spec won't be handled as expected. Hence, I'm hesitant to suggest deprecating the status code.

I still think it's still fair to express a preference as we did here, so as to not confuse the REST Catalog server implementer on which status code to choose to use on token expiry.

Copy link
Contributor

@dimas-b dimas-b Feb 27, 2025

Choose a reason for hiding this comment

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

From my POV the "401 should be preferred" language is already effectively deprecating 419.

However, from the spec evolution perspective, I do not think it would be valid to remove 419 from the v1 REST spec.

All in all, I think the "should" language adds enough clarity for both clients and servers.

Copy link
Contributor

@flyrain flyrain Feb 27, 2025

Choose a reason for hiding this comment

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

may cause confusion as 419 responses from Iceberg REST Catalog servers that are running on older versions of the spec won't be handled as expected.

Can you elaborate it? I think the clients are OK to treat a 419 as a 401, or just a normal failure in that case. To retry or not is the client side decision. I believe either behavior(client retry or not retry) is acceptable. My only concern to deprecate it is the SigV4 use case @danielcweeks and @rdblue mentioned, if SigV4 clients must depend on 419, we need to keep it. But I don't have much context, will appreciate any inputs.

Copy link
Contributor Author

@sungwy sungwy Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To retry or not is the client side decision

Yes I agree with you @flyrain . Although I think it'll be confusing for application developers if for example, PyIceberg removed retry handling for 419 status on the next version after the 419 status code is deprecated on the REST Catalog. The application developer would expect 419 status codes from their REST Catalog server to result in retries, and on version upgrade they their PyIceberg application no longer will. Hence, as a community, we may need to keep 419 status code handling for backward compatibility.

Copy link
Contributor

@flyrain flyrain Feb 28, 2025

Choose a reason for hiding this comment

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

The application developer would expect 419 status codes from their REST Catalog server to result in retries

yeah, that's an edge case, in which a server happened to return 419, plus an application happened to rely on 419 for retrying. As we all agreed that 419-based retry is not reliable and not recommended, application shouldn't do that afterward. These are very subtle things though. I'm fine with not deprecate it, but I still prefer to give users more clear message, that 419 shouldn't be used. Deprecating it makes it more clear.

Copy link
Contributor

@flyrain flyrain Mar 7, 2025

Choose a reason for hiding this comment

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

Brought this up in last community catalog meeting. The general agreement was to deprecate 419. Meeting link: https://www.youtube.com/watch?v=vbV2XxOTyT4.

content:
application/json:
schema:
Expand Down