stevenzwu commented on code in PR #15280:
URL: https://github.com/apache/iceberg/pull/15280#discussion_r3042984414
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1227,7 +1234,17 @@ paths:
type: string
description: The plan ID that has been used for server-side scan
planning
- $ref: '#/components/parameters/referenced-by'
- description: Load vended credentials for a table from the catalog.
+ - $ref: '#/components/parameters/storage-refresh-token'
+ description:
+ Load vended credentials for a table from the catalog.
+
+
+ An optional `storageRefreshToken` query parameter can be provided to
refresh credentials
+ for a staged table. See the `storageRefreshToken` parameter definition
for detailed behavior.
+
+
+ The `planId` and `storageRefreshToken` parameters are mutually
exclusive. If both are
+ provided, the server must return a 400 Bad Request error.
Review Comment:
The OpenAPI spec conventions use `MUST` (uppercase).
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1979,6 +1998,29 @@ components:
schema:
type: string
+ storage-refresh-token:
+ name: storageRefreshToken
+ in: query
+ description: >
+ Opaque token returned on a `StorageCredential` from a previous
response (e.g., staged table
+ creation, loadTable, or loadCredentials). When provided, the server
uses this token to resolve
+ the credential context (e.g., a staged table's storage location) and
return fresh credentials.
+
+
+ If the token resolves to a staged or committed table matching the
table name in the request
Review Comment:
If the table is already committed, the token shouldn't be
needed—loadCredentials already works by table name. The description conflates
two cases. Is the token specifically for staged tables? if the table has since
been committed, the server MAY resolve the token to the committed table (or the
client can simply omit the token)?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3470,6 +3512,13 @@ components:
type: object
additionalProperties:
type: string
+ storage-refresh-token:
+ type: string
+ description: >
+ Opaque token that clients pass back to the server to refresh this
credential.
+ Returned on the loadCredentials or loadTable endpoint via the
storageRefreshToken
Review Comment:
> Returned on the loadCredentials or loadTable endpoint via the
storageRefreshToken query parameter.
This sentence seems problematic to me. The refresh token is returned on the
`loadTable` or `loadCredentials` response. But it seems odd to say `via the
storageRefreshToken query parameter`. It is passed back to the server via the
query param for `loadCredentials` endpoint.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1227,7 +1234,17 @@ paths:
type: string
description: The plan ID that has been used for server-side scan
planning
- $ref: '#/components/parameters/referenced-by'
- description: Load vended credentials for a table from the catalog.
+ - $ref: '#/components/parameters/storage-refresh-token'
Review Comment:
A `createTable` response can return multiple `StorageCredential` objects
(one per prefix). Each credential in the PR gets its own
`storage-refresh-token`. But the `loadCredentials` endpoint accepts only one
`storageRefreshToken` query parameter.
The spec doesn't clarify:
- Should clients use the token from the first credential? The
longest-matching-prefix credential?
- Can different credentials in the same response have different refresh
tokens?
- If the server returns multiple credentials with different tokens, does a
single refresh call return all of them?
This **1:N** mismatch needs explicit guidance. If the token is really
per-table (not per-credential), it might belong on the `LoadTableResult` or
`LoadCredentialsResponse` rather than on individual `StorageCredential` objects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
