-
Notifications
You must be signed in to change notification settings - Fork 124
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
Treat missing default-base-location in UpdateCatalogRequest as no-change to default-base-location #440
Conversation
…nge to default-base-location The way the API model for Catalogs splits out the default-base-location as a required field separate from "additionalProperties" makes it easy for client libraries to forget to forget to "echo" an existing default-base-location when trying to set new properties. Since the default-base-location is required during catalog creation, it definitely wasn't correct to allow silently clobbering it in an update. So the only possibilities are: 1. Throw explicit error if it's missing 2. Treat it the same as when an UpdateCatalogRequest doesn't specify a StorageConfigInfo, which is to assume that the unspecified portion of the request should be unaltered Given the structure of the API making it easy to miss this fact, I'd say the most correct semantic here is to treat the same as unspecified StorageConfigInfo, and just leave default-base-location unchanged if it's missing from the UpdateCatalogRequest.
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 was definitely the original intention that if something was omitted during an update, that meant to leave it unchanged.
…te-catalog-as-noop
With this change, it looks like calls to |
Done. |
…as-noop' of github.com:dennishuo/polaris into dhuo-handle-no-default-base-location-in-update-catalog-as-noop
Sorry, I'm late on this PR. Even if it can be seen as a breaking change, I think it makes sense to do it because the catalog could be in a inconsistent state. Thanks ! |
The way the API model for Catalogs splits out the default-base-location as a required field separate from "additionalProperties" makes it easy for client libraries to forget to forget to "echo" an existing default-base-location when trying to set new properties.
Since the default-base-location is required during catalog creation, it definitely wasn't correct to allow silently clobbering it in an update. So the only possibilities are:
Given the structure of the API making it easy to miss this fact, I'd say the most correct semantic here is to treat the same as unspecified StorageConfigInfo, and just leave default-base-location unchanged if it's missing from the UpdateCatalogRequest.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Please delete options that are not relevant.