Use upstream 1.7.1 rest-catalog-open-api.yaml instead. #936

HonahX · 2025-02-04T01:31:37Z

Depends on #935

This is a second follow-up of #906. This PR makes the rest-catalog-open-api.yaml identical to the upstream released version (1.7.1)

HonahX · 2025-02-04T01:34:41Z

spec/rest-catalog-open-api.yaml

@@ -242,8 +236,6 @@ paths:
          $ref: '#/components/responses/OAuthErrorResponse'
        5XX:
          $ref: '#/components/responses/OAuthErrorResponse'
-      security:
-        - BearerAuth: []


It seems in Polaris the /v1/oauth/tokens has this additional security scheme override. This change is included in the original commit of Polaris.

To ensure this PR does not change anything unexpectedly, I extracted this one to polaris-catalog-apis/oauth-tokens-api.yaml. We will likely remove or update this soon anyway according to: #12

Now the /v1/oauth/tokens endpoint is defined in twp files, right? What controls which of the definitions is the one used in runtime?

Hi @dimas-b, thanks for reviewing. Currently we have polaris-catalog-service.yaml as the root yaml for building catalog api service

polaris/spec/polaris-catalog-service.yaml

Lines 76 to 79 in 84ee2e8

/v1/oauth/tokens:

$ref: './rest-catalog-open-api.yaml#/paths/~1v1~1oauth~1tokens'

It uses $ref to control the one used in the runtime. This PR updates the $ref so that it will point to the oauth-tokens-api.yaml

spec/polaris-catalog-service.yaml

spec/rest-catalog-open-api.yaml

spec/polaris-catalog-apis/oauth-tokens-api.yaml

HonahX · 2025-02-07T21:55:45Z

This PR is ready for review since #953 and #935 are in. @jackye1995 @dimas-b @adutra Could you please take another look at this when you have a chance? Thanks!

dimas-b · 2025-02-07T23:46:49Z

LICENSE

@@ -215,7 +215,10 @@ License: https://www.apache.org/licenses/LICENSE-2.0

 This product includes code from Apache Iceberg.

-* spec/rest-catalog-open-api.yaml
+* spec/iceberg-rest-catalog-open-api.yaml
+* spec/polaris-catalog-service.yaml


no, this is Polaris' own file, it is NOT from Apache Iceberg, right?

Thanks for all the great catch here! It seems I have misunderstood the intended use of the CODE_COPIED_TO_POLARIS flag and the corresponding licensing requirements. My current understanding is that this flag should only be applied to files where all content is copied from another project. Could you please confirm if that’s correct?

Based on this understanding, here’s the current status:

iceberg-rest-catalog-open-api.yaml: identical to upstream released spec. Need CODE_COPIED_TO_POLARIS

polaris-catalog-apis/oauth-tokens-api.yaml: everything is copied from IRC spec's token endpoint definition. Need CODE_COPIED_TO_POLARIS

polaris-catalog-service.yaml - only the server and security definition are copied from IRC spec, while the rest are not. Do Not Need CODE_COPIED_TO_POLARIS

bundled-polaris-catalog-service.yaml - generated yaml that contains both IRC spec and polaris defined ones. Do Not Need CODE_COPIED_TO_POLARIS

My current understanding is that this flag should only be applied to files where all content is copied from another project. Could you please confirm if that’s correct?

That's my understanding too :)

polaris-catalog-service.yaml - only the server and security definition are copied from IRC spec, while the rest are not. Do Not Need CODE_COPIED_TO_POLARIS

I agree. Plus we have a general notice with Iceberg attribution. I think that's fine.

@jbonofre WDYT?

LICENSE

spec/generated/bundled-polaris-catalog-service.yaml

spec/polaris-catalog-service.yaml

Make rest-catalog-open-api.yaml identical to 1.7.1 released version of rest-catalog-open-api.yaml Commit added new files add not implemented endpoint as comments Add code copied to Polaris make oauth-tokens-api standalone fix COPIED code issue rename iceberg upstream copied spec add upstream version note Fix lint Fix LICENSE Remove incorrect LICENSE mention Fix style

dimas-b · 2025-02-26T14:47:59Z

@snazy @RussellSpitzer @jackye1995 @collado-mike : This PR LGTM, but since it's an API change - please also have a look :)

flyrain · 2025-02-26T17:13:19Z

spec/polaris-catalog-apis/oauth-tokens-api.yaml

+---
+paths:
+  # The /v1/oauth/tokens endpoint is sourced from iceberg-rest-catalog-open-api.yaml
+  /v1/oauth/tokens:


Sorry to miss the context, why do we copy this from iceberg-rest-catalog-open-api.yaml?

The original related discussion is here: #906 (comment), #936 (comment).

Since the token endpoint is deprecated and will soon be removed from the upstream IRC spec, we want to extract it from the spec. This allows us to control its removal timeline, update the endpoint as needed, or continue supporting it temporarily for compatibility. Issue #12.

flyrain · 2025-02-26T18:10:45Z

spec/polaris-catalog-apis/oauth-tokens-api.yaml

+# under the License.
+#
+
+# CODE_COPIED_TO_POLARIS


Should we also give a version like this? Given that future Iceberg rest spec may not even have it.

# CODE_COPIED_TO_POLARIS # From Apache Iceberg Version: 1.7.1

I agree that in the current form CODE_COPIED_TO_POLARIS is too vague.

I'd argue for dependency-specific labels, e.g. CODE_COPIED_FROM_ICEBERG: 1.7.1

Certainly not for this PR, but for follow-up change.. if people are in agreement :)

Thanks for the suggestions! I've added the comment but I will leave this thread open to continue the discussion on dependency-specific labels : )

spec/polaris-catalog-apis/oauth-tokens-api.yaml

HonahX · 2025-02-26T18:42:56Z

Thanks everyone for reviewing! I've updated the PR. I saw currently Merging is blocked because

This branch must not contain merge commits

but I've cleaned up/rebased commits. Does it mean I have to close and re-open this one or open a new PR?

HonahX commented Feb 4, 2025

View reviewed changes

spec/polaris-catalog-service.yaml Show resolved Hide resolved

HonahX commented Feb 4, 2025

View reviewed changes

spec/rest-catalog-open-api.yaml Show resolved Hide resolved

adutra reviewed Feb 4, 2025

View reviewed changes

spec/polaris-catalog-apis/oauth-tokens-api.yaml Show resolved Hide resolved

jackye1995 reviewed Feb 4, 2025

View reviewed changes

spec/polaris-catalog-apis/oauth-tokens-api.yaml Outdated Show resolved Hide resolved

jackye1995 reviewed Feb 4, 2025

View reviewed changes

spec/polaris-catalog-apis/oauth-tokens-api.yaml Outdated Show resolved Hide resolved

HonahX mentioned this pull request Feb 7, 2025

Support Iceberg Rest Catalog Scan Planning Endpoints #966

Open

HonahX marked this pull request as ready for review February 7, 2025 21:53

HonahX requested review from ashvina, dennishuo, eric-maynard, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners February 7, 2025 21:53

dimas-b reviewed Feb 7, 2025

View reviewed changes

dimas-b approved these changes Feb 8, 2025

View reviewed changes

HonahX mentioned this pull request Feb 26, 2025

Remove the duplicated notification API spec in rest-catalog-open-api.yaml #1069

Closed

HonahX force-pushed the honahx_use_upstream_released_iceberg_spec_instead branch from 95a57cf to c1be23b Compare February 26, 2025 07:52

HonahX force-pushed the honahx_use_upstream_released_iceberg_spec_instead branch from c1be23b to d98d4c5 Compare February 26, 2025 08:03

flyrain reviewed Feb 26, 2025

View reviewed changes

spec/polaris-catalog-apis/oauth-tokens-api.yaml Outdated Show resolved Hide resolved

flyrain approved these changes Feb 26, 2025

View reviewed changes

update comments

1d4746f

flyrain enabled auto-merge (squash) February 26, 2025 18:46

flyrain approved these changes Feb 26, 2025

View reviewed changes

flyrain merged commit 4f7cf97 into apache:main Feb 26, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use upstream 1.7.1 rest-catalog-open-api.yaml instead. #936

Use upstream 1.7.1 rest-catalog-open-api.yaml instead. #936

HonahX commented Feb 4, 2025

HonahX Feb 4, 2025

dimas-b Feb 4, 2025

HonahX Feb 4, 2025

HonahX commented Feb 7, 2025

dimas-b Feb 7, 2025

HonahX Feb 8, 2025

dimas-b Feb 8, 2025

dimas-b Feb 8, 2025

dimas-b commented Feb 26, 2025 •

edited

Loading

flyrain Feb 26, 2025

HonahX Feb 26, 2025

flyrain Feb 26, 2025 •

edited

Loading

dimas-b Feb 26, 2025

HonahX Feb 26, 2025

HonahX commented Feb 26, 2025


	/v1/oauth/tokens:
	$ref: './rest-catalog-open-api.yaml#/paths/~1v1~1oauth~1tokens'

Use upstream 1.7.1 rest-catalog-open-api.yaml instead. #936

Use upstream 1.7.1 rest-catalog-open-api.yaml instead. #936

Conversation

HonahX commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonahX commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b commented Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonahX commented Feb 26, 2025

dimas-b commented Feb 26, 2025 •

edited

Loading

flyrain Feb 26, 2025 •

edited

Loading