-
Notifications
You must be signed in to change notification settings - Fork 381
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
[#6092] docs(core): add credential openapi document #6088
base: main
Are you sure you want to change the base?
Conversation
…che#6039) ### What changes were proposed in this pull request? Remove the unnecessary `CONTRIBUTING` file under `.github` folder. ### Why are the changes needed? It's not necessary to add this file in `.github` folder. Fix: apache#6008 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? N/A
@jerryshao @mchades @yuqi1129 PTAL |
docs/open-api/credentials.yaml
Outdated
get: | ||
tags: | ||
- credentials | ||
summary: List credentials for metadata object |
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.
Summary will be used as the title in the sidebar (https://gravitino.apache.org/docs/0.7.0-incubating/api/rest/list-catalogs), it should not be too long.
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.
updated
$ref: "#/components/examples/CredentialResponse" | ||
"400": | ||
$ref: "./openapi.yaml#/components/responses/BadRequestErrorResponse" | ||
"404": |
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.
Since this is a "list" operation rather than a "get", we won't get a "404" error, right?
If there are no credentials, returning an empty list should be desirable and it is not supposed to be treated as a "NotFound" error.
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.
Yes, it's odd to return 404 for list operations. removed NoSuchCredential
from the API
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 observe that the request path specifies the metalake name; if the metalake is nonexistent, should a 404 error be returned?
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.
updated, and noticed some tag
API missing NoSuchXX
too, also fixed in this PR.
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.
If a metalake or a schema or whatever upper-level entity is not found, this request should return a 400. It is a misconfigured REST request, because the path
cannot be matched by the Web layer.
404 means not found, it means and only means the meta object is not found.
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.
404 accurately indicates that the requested resource, including any part of the URL path, cannot be found on the server.
400 Bad Request, on the other hand, is more suitable for malformed requests, such as invalid syntax or improper formatting, rather than for resources that simply don't exist.
metalake is also a meta object
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.
404 accurately indicates that the requested resource, including any part of the URL path, cannot be found on the server.
No. If the URL path is incorrectly formatted, such as containing illegal characters, the request is not valid. The error is from the client side. So the return value should be 400.
404 has a crystal clear definition - the target resource is not found. The target resource is a singular object of a specific type. You cannot use 404 to indicate that the metalake is not found or the schema is not found or the meta-object is not found.
This is an obvious abuse of the HTTP status code.
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.
404
is widely used for NoSuchMetalakeException
exception, How about creating a separate issue to track this?
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.
This is an obvious abuse of the HTTP status code.
Your interpretation of "target resource" seems too narrow in the context of RESTful APIs. The HTTP specification (RFC 7231) states that 404 applies when "the origin server did not find a current representation for the target resource." In REST, each path segment is considered a resource, not just the final element.
GitHub's API provides a clear example of this practice. Consider their endpoint for getting a repository:
GET /repos/{owner}/{repo}
If either the {owner} or {repo} doesn't exist, GitHub returns a 404 status code. For instance, requesting https://api.github.com/repos/nonexistent-user/some-repo returns 404, even though the error is in the 'owner' part of the path, not the final resource.
You can verify this behavior and see the documentation here:
https://docs.github.com/en/rest/repos/repos#get-a-repository
This approach aligns with RESTful design principles, providing clear feedback about resource availability throughout the URL path, and is consistent with industry best practices.
docs/open-api/credentials.yaml
Outdated
properties: | ||
credentialType: | ||
type: string | ||
description: The type of the credential |
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 believe we have a enum
for this field.
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.
we didn't define an enum
because we should support custom credentials. add the build-in credentials
docs/open-api/credentials.yaml
Outdated
description: The type of the credential | ||
expireTimeInMs: | ||
type: long | ||
description: The expire time of the credential |
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.
Please amend the description so that users know what this long number is.
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.
updated
@@ -68,7 +59,7 @@ components: | |||
type: string | |||
description: The type of the credential, for example, s3-token, s3-secret-key, oss-token, oss-secret-key, gcs-token, adls-token, azure-account-key, etc. | |||
expireTimeInMs: | |||
type: long | |||
type: integer | |||
description: The expiration time of the credential in milliseconds since the epoch, 0 means not expire. |
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.
Gocha. :)
This is not supposed to be a time elapsed since epoch.
It is supposed to be time duration in milliseconds when the credential (e.g. a certificate) will expire.
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.
It's not an time duration, because the credential may need some time to transfer from server side to client side, which makes duration not correct.
properties: | ||
credentialType: | ||
type: string | ||
description: The type of the credential, for example, s3-token, s3-secret-key, oss-token, oss-secret-key, gcs-token, adls-token, azure-account-key, etc. |
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.
Thanks for mentioning the credential type examples.
Although not in the scope of this PR, I'm still wondering if we need to add an API for users to retrieve a list of supported credential types.
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 couldn't figure out usage scenarios about this API, we could add it if requried.
- expireTimeInMs | ||
- credentialInfo | ||
properties: | ||
credentialType: |
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 wouldn't call this credentialType
because the property has a well-defined scope.
E.g. when I'm modeling a Person
, I'll simply add a gender
property rather than
naming it as personGender
.
This can be pretty simple, e.g.
{"type": "gcs-token", "expireTimeInMs": 12345, "data": {"token": "secret"}}
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.
Yes, thanks for pointing this out, I agree with you, how about keeping this as it has been published as API? I will try to use the short style for new features.
What changes were proposed in this pull request?
add credential openapi document
Why are the changes needed?
Fix: #6092
Does this PR introduce any user-facing change?
no
How was this patch tested?
just document