Improve test coverage for invalid inputs in Policy APIs #1665
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
williamhyun
requested review from
adutra,
ashvina,
collado-mike,
dennishuo,
dimas-b,
eric-maynard,
jackye1995,
jbonofre and
vvcephei
as code owners
williamhyun
requested review from
MonkeyCanCode,
RussellSpitzer,
flyrain,
snazy and
takidau
as code owners
Member
Author
|
cc: @HonahX |
adnanhemani
suggested changes
May 23, 2025
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Show resolved
Hide resolved
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
HonahX
reviewed
May 23, 2025
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Show resolved
Hide resolved
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
Contributor
|
Thanks for working on this! It looks great, left some comments. |
Member
Author
|
Thank you for the review @adnanhemani and @HonahX! |
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
...ts/src/main/java/org/apache/polaris/service/it/test/PolarisPolicyServiceIntegrationTest.java
Outdated
Show resolved
Hide resolved
Member
Author
|
cc: @eric-maynard |
eric-maynard
previously approved these changes
May 27, 2025
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
adnanhemani
reviewed
May 27, 2025
| policyApi | ||
| .request( | ||
| "polaris/v1/{cat}/namespaces/{ns}/policies/{policy}", | ||
| Map.of("cat", currentCatalogName, "ns", "NS1", "policy", INVALID_POLICY)) |
Contributor
There was a problem hiding this comment.
nit: This is similar to the comment I left earlier - ideally we should not have "magic strings" anywhere in the tests either.
So instead of using "NS1", we should be using variable NS1 and in all locations instead of using NS1 inline on the assertion, make the string into "namespace: " + NS1 (and substitute all other locations of this, as required).
This is a retry at apache#1586, which I'll close if this PR is on the right direction instead. Java URI does not actually apply any normalization to URIs if we do not call URI.normalize() (which we currently do not). Additionally, blob storage providers like S3 and GCS can provide ".." and "." as valid fragments in URLs - which Java URI would attempt to normalize incorrectly. As a result, attempting to validate and/or normalize URIs for blob storage providers using the Java URI class is the incorrect behavior. While we may want to add location validation via regex later, removing it first should at least unblock the bug we see in apache#1545.
flyrain
approved these changes
May 27, 2025
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Contributor
|
Thanks a lot for working on it, @williamhyun! Thanks everyone for the review. |
snazy
added a commit
to snazy/polaris
that referenced
this pull request
Jun 13, 2025
* fix: Remove duplicated code in IcebergCatalog (apache#1681) * Fix SparkClient listGenericTable to use ListGenericTablesRESTResponse ListTableResponse is the class used by iceberg endpoint, which is the same as ListGenericTablesRESTResponse. However, we are suppose to use ListGenericTablesRESTResponse to be correct * fix: Remove info log about deprecated internal method from PolarisConfiguration (apache#1672) Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages. Phasing out old property names requires coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code. Fixes apache#1666 * Create a single binary distribution bundle (apache#1589) * fix(quickstart): Correct Quickstart Instructions (apache#1673) * Remove Java URI validations for Blob Storage providers (apache#1604) This is a retry at apache#1586, which I'll close if this PR is on the right direction instead. Java URI does not actually apply any normalization to URIs if we do not call URI.normalize() (which we currently do not). Additionally, blob storage providers like S3 and GCS can provide ".." and "." as valid fragments in URLs - which Java URI would attempt to normalize incorrectly. As a result, attempting to validate and/or normalize URIs for blob storage providers using the Java URI class is the incorrect behavior. While we may want to add location validation via regex later, removing it first should at least unblock the bug we see in apache#1545. * Improve test coverage for invalid inputs in Policy APIs (apache#1665) * Fix getting-started docker start by PR apache#1532 (apache#1687) * Fix the manual test broken by PR apache#1532 (apache#1688) * Fix credentials printing twice (apache#1682) * main: Update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.0.4 (apache#1690) * INFO: last merged commit 493de03 --------- Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com> Co-authored-by: gh-yzou <167037035+gh-yzou@users.noreply.github.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Yufei Gu <yufei@apache.org> Co-authored-by: Adnan Hemani <adnan.h@berkeley.edu> Co-authored-by: William Hyun <william@apache.org> Co-authored-by: Christopher Lambert <xn137@gmx.de> Co-authored-by: Mend Renovate <bot@renovateapp.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to improve the Policy API test coverage per #1582.