[PAUSED - DO NOT REVIEW] Allow for quotes in path for blob storage #1586

adnanhemani · 2025-05-14T05:55:14Z

Fixes #1545. Further context on that ticket.

Java URIs do not allow special characters, such as quotes, as part of RFC 3986. As a result, it is not possible to use paths that contain such characters with Java URIs, which we use to compare storage locations when creating a new catalog.

This change refactors that code to only use URI if the location provided is a local filesystem path - else, it will use Hadoop's Path class in order to normalize the location.

adnanhemani · 2025-05-15T06:06:54Z

I originally used Hadoop Path class because it is the closest thing to what we want and we already bring the Hadoop Client API dependency in other modules, such as Service. I don't personally think adding a client API to this module is the worst thing in the world, but not adding it just makes us to have to normalize the paths ourselves.

So I'm not strong on doing this either way but given there's unanimous pushback on it, I've implemented the normalization manually in the latest push.

@eric-maynard - I thought about creating new XYZLocation types, but I found them to just end up being very simplistic classes after drafting the changes. S3 and GCS don't have the same level of complexity that Azure has, so I've implemented the normalization in StorageLocation itself. Please let me know if this is something you feel strongly about. Thanks all!

eric-maynard · 2025-05-15T16:47:16Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

    }
  }
+
+  private static String scheme(String location) {


nit: method name

Changed to getScheme

Did this get missed?

Sorry - seems like things didn't push :( Will push with the next set of comments.

eric-maynard · 2025-05-15T16:47:24Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+    String path;
+    if (scheme == null) {
+      path = location;
+      scheme = "file";


We don't have an enum or something for this?

Unfortunately not, we are parsing them from the user-provided input. I tried hard-typing these and rejecting any schemes that are not explicitly supported (but including file formats that don't explicitly state a scheme) and then a barrage of test cases started failing due to weird input we've put in - some of them intentionally, others not. So I'm not really in favor of making such a massive shift, it will dramatically increase the scope of this change.

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

eric-maynard · 2025-05-15T16:48:28Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+      path = location.substring(scheme.length() + 1);
+    }
+    path = normalizePath(path);
+    if (scheme.equals("file")) {


ditto, let's at least put this in a const

Roger that, that's reasonable. Added in next revision.

eric-maynard · 2025-05-15T16:48:54Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+    if (scheme.equals("file")) {
+      this.location = URI.create(LOCAL_PATH_PREFIX + path).toString();
+    } else {
+      this.location = scheme + "://" + path;


Doesn't this duplicate the ://?

No - the scheme will be without the "://", as we parse right up to the ":" for it. The path is normalized in the normalizePath function such that we will remove any starting slashes and the parsing for path starts after the ":" character.

eric-maynard · 2025-05-15T16:52:03Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

    } else {
-      this.location = new Path(location).toString();


Looks like this branch used to cover relative paths, but now it won't

That's correct. Trying to deal with these is only going to muddy the waters further here if we try to implement it manually :( And I don't think this is a case that we really need to care heavily for cloud storage providers given that they are really just pseudo-filesystems.

If this is a strong requirement, I strongly recommend that we go back to Hadoop's Path class instead of trying this manually....

dimas-b · 2025-05-15T20:09:35Z

polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java


 import org.apache.polaris.core.storage.StorageLocation;
-import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Assertions;


Why not assertj? They are generally more user-friendly, IMHO

Earlier, I was testing some assertionThrows and JUnit was much easier to work with for those imo. But now that those are gone, I agree for these cases - I'll change them back.

dimas-b · 2025-05-15T20:12:25Z

polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java

+    Assertions.assertEquals(testStorageLocation, baseStorageLocation);
+
+    testStorageLocation = StorageLocation.of("s3://////path'/////*to===///\"file");
+    Assertions.assertEquals("s3://path'/*to===/\"file", testStorageLocation.toString());


nit: in this URI path' is not a "path" but "authority" per RFC 3986

dimas-b · 2025-05-15T20:13:07Z

polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java

+  public void testGcpLocations() {
+    StorageLocation baseStorageLocation = StorageLocation.of("gcs://path/to/file");
+    StorageLocation testStorageLocation = StorageLocation.of("gcs://path/to/////file");
+    Assertions.assertEquals(testStorageLocation, baseStorageLocation);


Is this test materially different from testAwsLocations?

In spirit, no. Let me merge this case and the AWS cases.

dimas-b · 2025-05-15T20:14:40Z

polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java

+  public void testAwsLocations() {
+    StorageLocation baseStorageLocation = StorageLocation.of("s3://path/to/file");
+    StorageLocation testStorageLocation = StorageLocation.of("s3://path/to/////file");
+    Assertions.assertEquals(testStorageLocation, baseStorageLocation);


This test is good, but I believe more test cases are needed to validate the handling of upper / lower case letters and leading / trailing slashes and the special cases of missing scheme.

Good call out - let me add them. Missing scheme is in the function below with this variable: slashLeadingLocation

dimas-b · 2025-05-15T20:23:48Z

Re: #1545 : this PR fixes part of the problem, but I'm not sure it fixes all that needs to be fixed. I believe the use case in #1545 needs to be tested end-to-end (with reading from tables) to be sure it is fixed.

adnanhemani · 2025-05-15T21:55:14Z

I believe the use case in #1545 needs to be tested end-to-end (with reading from tables) to be sure it is fixed.

@dimas-b - I think that's a reasonable ask. Are you saying that we should do this manually once or that this needs an integration test?

dimas-b · 2025-05-16T19:15:24Z

I mostly meant that this PR should not claim that it Fixes #1545 (in the description) :)

I did not mean to expand the scope of this PR. We can merge this incremental improvement, re-test the issue manually and follow-up with more PRs and integration tests as required.

eric-maynard · 2025-05-16T20:35:05Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+    if (schemePos > 0) {
+      return location.substring(0, schemePos).toLowerCase(Locale.ROOT);
+    }
+    return null;


Should we throw here?

No - this is passed early in the making of the StorageLocation and can, as a result, get filesystem locations like /var/sample/location. This is how we're figuring out that this is a local file path.

eric-maynard · 2025-05-16T20:35:19Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+    return null;
+  }
+
+  protected String normalizePath(String path) {


This isn't really normalization, maybe it's canonicalization?

Seems like both terms are synonyms - at least I'm not seeing a difference between these two terms. Can you please explain the difference or provide a link to something that does? For reference, this is what Google is pulling up on this.

Canonicalization usually means adding all the missing parts, such as the scheme and authority, whereas normalization / resolution usually just means making a path absolute.

eric-maynard · 2025-05-16T20:36:21Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+  }
+
+  protected String normalizePath(String path) {
+    path = SLASHES.matcher(path).replaceAll("/");


I don't think this is always correct. IIRC on Azure /foo/bar is not the same as /foo//bar. Do you know how S3 treats this?

It looks like this is just a special case for file:///... which should really just be treated as an empty authority

Ahh yes, great catch. Can't believe I forgot about this but you're correct, S3 is the same as well. I will make changes to make sure this is only happening for local files then.

eric-maynard · 2025-05-16T20:37:02Z

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java

+    if (path.endsWith("/") || path.startsWith("/")) {
+      path =
+          path.substring(
+              path.startsWith("/") && path.length() > 1 ? 1 : 0,
+              path.endsWith("/") ? path.length() - 1 : path.length());
+    }


StringUtils.strip

eric-maynard · 2025-05-16T20:38:50Z

polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java

+    testStorageLocation = StorageLocation.of("gcs://////path'/////*to===///\"file");
+    Assertions.assertEquals("gcs://path'/*to===/\"file", testStorageLocation.toString());


See my comment above -- is this actually correct?

How does it work right now in Polaris?

It's not correct. Actually when looking through Java URI code - we are not currently applying any normalization to URLs...

And I'm not sure that normalizing URLs is entirely necessary either. We've worked through the comments above that double slashes shouldn't be modified and similarly, "." and ".." also shouldn't be normalized for blob storage.

As a result, I think this might be trying to do too much. I'm going to stop work on this and push a new branch that simply just takes the URI create off for the Blob Storage locations and adds additional tests. Will link that PR here when I push it.

adnanhemani · 2025-05-17T20:34:06Z

Created #1604. Will keep this PR open until #1604 merges or at least gets confirmed as the right way to go - but please do not review this PR until further notice.

This is a retry at #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 #1545.

eric-maynard · 2025-05-27T20:54:39Z

Closing in favor of #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.

* 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>

Allow for quotes in path for blob storage

d1a348a

adnanhemani requested review from MonkeyCanCode, RussellSpitzer, adutra, ashvina, collado-mike, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, snazy, takidau and vvcephei as code owners May 14, 2025 05:55

github-project-automation bot added this to Basic Kanban Board May 14, 2025

adnanhemani requested review from HonahX, ebyhr, flyrain, pingtimeout and singhpk234 as code owners May 14, 2025 05:55

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board May 14, 2025

adnanhemani removed request for RussellSpitzer, adutra, dennishuo, ebyhr, flyrain, jbonofre, pingtimeout, snazy and vvcephei May 14, 2025 05:55

SpotlessApply

4654ec5

eric-maynard reviewed May 15, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java Show resolved Hide resolved

eric-maynard reviewed May 15, 2025

View reviewed changes

dimas-b reviewed May 15, 2025

View reviewed changes

flyrain mentioned this pull request May 16, 2025

Make Polaris safe against certain unparseable locations #552

Open

eric-maynard reviewed May 16, 2025

View reviewed changes

adnanhemani mentioned this pull request May 17, 2025

Remove Java URI validations for Blob Storage providers #1604

Merged

adnanhemani changed the title ~~Allow for quotes in path for blob storage~~ [PAUSED] Allow for quotes in path for blob storage May 19, 2025

adnanhemani changed the title ~~[PAUSED] Allow for quotes in path for blob storage~~ [PAUSED - DO NOT REVIEW] Allow for quotes in path for blob storage May 19, 2025

snazy marked this pull request as draft May 21, 2025 08:56

eric-maynard closed this May 27, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board May 27, 2025

		testStorageLocation = StorageLocation.of("gcs://////path'/////*to===///\"file");
		Assertions.assertEquals("gcs://path'/*to===/\"file", testStorageLocation.toString());

[PAUSED - DO NOT REVIEW] Allow for quotes in path for blob storage #1586

[PAUSED - DO NOT REVIEW] Allow for quotes in path for blob storage #1586

Uh oh!

Conversation

adnanhemani commented May 14, 2025

Uh oh!

adnanhemani commented May 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented May 15, 2025

Uh oh!

adnanhemani commented May 15, 2025

Uh oh!

dimas-b commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adnanhemani May 15, 2025 •

edited

Loading

adnanhemani May 15, 2025 •

edited

Loading

dimas-b May 15, 2025 •

edited

Loading

adnanhemani May 15, 2025 •

edited

Loading

dimas-b commented May 16, 2025 •

edited

Loading

eric-maynard May 16, 2025 •

edited

Loading