You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adnanhemani (via GitHub)" <gi...@apache.org> on 2025/05/17 20:32:33 UTC
[PR] Remove Java URI validations for Blob Storage providers [polaris]
adnanhemani opened a new pull request, #1604:
URL: https://github.com/apache/polaris/pull/1604
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
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 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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2096484269
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java:
##########
@@ -44,8 +44,10 @@ protected StorageLocation(@Nonnull String location) {
this.location = URI.create(location.replaceFirst("file:/+", LOCAL_PATH_PREFIX)).toString();
} else if (location.startsWith("/")) {
this.location = URI.create(location.replaceFirst("/+", LOCAL_PATH_PREFIX)).toString();
Review Comment:
Do we have tests for multiple leading `/` chars?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java:
##########
@@ -44,8 +44,10 @@ protected StorageLocation(@Nonnull String location) {
this.location = URI.create(location.replaceFirst("file:/+", LOCAL_PATH_PREFIX)).toString();
} else if (location.startsWith("/")) {
this.location = URI.create(location.replaceFirst("/+", LOCAL_PATH_PREFIX)).toString();
- } else {
+ } else if (location.startsWith(LOCAL_PATH_PREFIX)) {
Review Comment:
What if location is `FILE:////////tmp/smth`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2098578243
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java:
##########
@@ -18,18 +18,63 @@
*/
package org.apache.polaris.service.storage;
+import java.net.URISyntaxException;
import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.azure.AzureLocation;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
public class StorageLocationTest {
@Test
public void testOfDifferentPrefixes() {
- StorageLocation StandardLocation = StorageLocation.of("file:///path/to/file");
+ StorageLocation standardLocation = StorageLocation.of("file:///path/to/file");
StorageLocation slashLeadingLocation = StorageLocation.of("/path/to/file");
+ StorageLocation manySlashLeadingLocation = StorageLocation.of("////////path/to/file");
StorageLocation fileSingleSlashLocation = StorageLocation.of("file:/path/to/file");
- Assertions.assertThat(slashLeadingLocation.equals(StandardLocation)).isTrue();
- Assertions.assertThat(fileSingleSlashLocation.equals(StandardLocation)).isTrue();
+ StorageLocation fileTooManySlashesLocation = StorageLocation.of("file://///////path/to/file");
+ Assertions.assertThat(slashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(manySlashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileSingleSlashLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileTooManySlashesLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(standardLocation instanceof AzureLocation).isFalse();
Review Comment:
This is a good question! `AzureLocation` is an extension of `StorageLocation` (due to Azure's very specific way of defining locations). All other locations remain instances of `StorageLocation`. So if I test that any of these locations (including an instance of AzureLocation) are an instance of `StorageLocation`, they'll always return true.
This test is basically just testing that we are not creating `AzureLocation` subclassed objects for these paths.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2890470439
Can you elaborate on when `..` would be a valid in a path? For me, `..` is a path-traversal in the presence of it raises the question "why is this not the source of a path-traversal-attack".
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2096639901
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java:
##########
@@ -44,8 +44,10 @@ protected StorageLocation(@Nonnull String location) {
this.location = URI.create(location.replaceFirst("file:/+", LOCAL_PATH_PREFIX)).toString();
} else if (location.startsWith("/")) {
this.location = URI.create(location.replaceFirst("/+", LOCAL_PATH_PREFIX)).toString();
- } else {
+ } else if (location.startsWith(LOCAL_PATH_PREFIX)) {
Review Comment:
Good call out - fixed!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2892234327
> Can you elaborate on when .. would be a valid in a path? For me, .. is a path-traversal in the presence of it raises the question "why is this not the source of a path-traversal-attack".
Blob Storage providers like S3 and GCS do not recognize ".." as a path traversal. If you store an object in S3 with the path "s3://abcd/a/b/../c", S3 stores the object exactly as such - it will not normalize the path into "s3://abcd/a/c". While it may be an anti-pattern to make such a location, we should not deviate from what the blob storage provider allows/does. The same can be said about multiple forward slashes together ("s3://abcd/a/b//c" is not the same as "s3://abcd/a/b/c").
As a result, we should only allow ".." and "." normalization in local filesystem calls where these can be resolved - and those are the code paths where Java URI is being used in this PR. WDYT?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2893541163
In ADLS `/` is a literally a [path separator](https://learn.microsoft.com/en-us/azure/storage/blobs/data-lake-storage-introduction-abfs-uri#uri-syntax), so I guess that has to be normalized.
Sure, in S3 + GCS the path is a plain string. But are patterns like `/../` or `///` in S3/GCS locations used in practice? I've seen usages of special characters like `"`, but not `/../` or `///`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2097983082
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java:
##########
@@ -18,18 +18,63 @@
*/
package org.apache.polaris.service.storage;
+import java.net.URISyntaxException;
import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.azure.AzureLocation;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
public class StorageLocationTest {
@Test
public void testOfDifferentPrefixes() {
- StorageLocation StandardLocation = StorageLocation.of("file:///path/to/file");
+ StorageLocation standardLocation = StorageLocation.of("file:///path/to/file");
StorageLocation slashLeadingLocation = StorageLocation.of("/path/to/file");
+ StorageLocation manySlashLeadingLocation = StorageLocation.of("////////path/to/file");
StorageLocation fileSingleSlashLocation = StorageLocation.of("file:/path/to/file");
- Assertions.assertThat(slashLeadingLocation.equals(StandardLocation)).isTrue();
- Assertions.assertThat(fileSingleSlashLocation.equals(StandardLocation)).isTrue();
+ StorageLocation fileTooManySlashesLocation = StorageLocation.of("file://///////path/to/file");
+ Assertions.assertThat(slashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(manySlashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileSingleSlashLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileTooManySlashesLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(standardLocation instanceof AzureLocation).isFalse();
Review Comment:
What is the meaning of this assertion? Is it not preferable to assert exact expected type rather that asserting what the type is _not_?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2100981589
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java:
##########
@@ -18,18 +18,63 @@
*/
package org.apache.polaris.service.storage;
+import java.net.URISyntaxException;
import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.azure.AzureLocation;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
public class StorageLocationTest {
@Test
public void testOfDifferentPrefixes() {
- StorageLocation StandardLocation = StorageLocation.of("file:///path/to/file");
+ StorageLocation standardLocation = StorageLocation.of("file:///path/to/file");
StorageLocation slashLeadingLocation = StorageLocation.of("/path/to/file");
+ StorageLocation manySlashLeadingLocation = StorageLocation.of("////////path/to/file");
StorageLocation fileSingleSlashLocation = StorageLocation.of("file:/path/to/file");
- Assertions.assertThat(slashLeadingLocation.equals(StandardLocation)).isTrue();
- Assertions.assertThat(fileSingleSlashLocation.equals(StandardLocation)).isTrue();
+ StorageLocation fileTooManySlashesLocation = StorageLocation.of("file://///////path/to/file");
+ Assertions.assertThat(slashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(manySlashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileSingleSlashLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileTooManySlashesLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(standardLocation instanceof AzureLocation).isFalse();
Review Comment:
TIL this exists in AssertJ :) Thanks @dimas-b - pushed changes with this. PTAL!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2902564451
> There are too many unrelated changes in this PR after the last push, it seems
Sorry, made a mistake in how I rebased it and got everything played on top of this PR. I've forced-push to correct - changes should be back to normal :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard merged PR #1604:
URL: https://github.com/apache/polaris/pull/1604
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2892219306
> I'm also confused on having multiple PRs for the same issue.
Please see https://github.com/apache/polaris/pull/1586#discussion_r2094201688 and https://github.com/apache/polaris/pull/1586#issuecomment-2888573434
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2901662322
Please rebase to get the latest CI changes.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2890472291
I'm also confused on having multiple PRs for the same issue.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2894411303
I've heard stories about `/../` being used in S3 URI. I think it is preferable for Polaris _not_ to interpret storage URI paths beyond determining "prefix" matches.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2098710239
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java:
##########
@@ -18,18 +18,63 @@
*/
package org.apache.polaris.service.storage;
+import java.net.URISyntaxException;
import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.azure.AzureLocation;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
public class StorageLocationTest {
@Test
public void testOfDifferentPrefixes() {
- StorageLocation StandardLocation = StorageLocation.of("file:///path/to/file");
+ StorageLocation standardLocation = StorageLocation.of("file:///path/to/file");
StorageLocation slashLeadingLocation = StorageLocation.of("/path/to/file");
+ StorageLocation manySlashLeadingLocation = StorageLocation.of("////////path/to/file");
StorageLocation fileSingleSlashLocation = StorageLocation.of("file:/path/to/file");
- Assertions.assertThat(slashLeadingLocation.equals(StandardLocation)).isTrue();
- Assertions.assertThat(fileSingleSlashLocation.equals(StandardLocation)).isTrue();
+ StorageLocation fileTooManySlashesLocation = StorageLocation.of("file://///////path/to/file");
+ Assertions.assertThat(slashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(manySlashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileSingleSlashLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileTooManySlashesLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(standardLocation instanceof AzureLocation).isFalse();
Review Comment:
Cf. `assertThat(...).isExactlyInstanceOf(...)`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on PR #1604:
URL: https://github.com/apache/polaris/pull/1604#issuecomment-2895355640
> But are patterns like /../ or /// in S3/GCS locations used in practice?
Agreed with @dimas-b here. Although users should _probably not_ be doing this, I'd prefer we not make Polaris opinionated on this (since we have no good reason to do so).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2102837479
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java:
##########
@@ -44,8 +44,10 @@ protected StorageLocation(@Nonnull String location) {
this.location = URI.create(location.replaceFirst("file:/+", LOCAL_PATH_PREFIX)).toString();
} else if (location.startsWith("/")) {
this.location = URI.create(location.replaceFirst("/+", LOCAL_PATH_PREFIX)).toString();
- } else {
+ } else if (location.startsWith(LOCAL_PATH_PREFIX)) {
Review Comment:
This line of code is gone now, but I think the question about upper case URIs remains. Since the purpose of this PR is directly related to that, I'm ok with addressing upper case URIs later.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2098710239
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java:
##########
@@ -18,18 +18,63 @@
*/
package org.apache.polaris.service.storage;
+import java.net.URISyntaxException;
import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.azure.AzureLocation;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
public class StorageLocationTest {
@Test
public void testOfDifferentPrefixes() {
- StorageLocation StandardLocation = StorageLocation.of("file:///path/to/file");
+ StorageLocation standardLocation = StorageLocation.of("file:///path/to/file");
StorageLocation slashLeadingLocation = StorageLocation.of("/path/to/file");
+ StorageLocation manySlashLeadingLocation = StorageLocation.of("////////path/to/file");
StorageLocation fileSingleSlashLocation = StorageLocation.of("file:/path/to/file");
- Assertions.assertThat(slashLeadingLocation.equals(StandardLocation)).isTrue();
- Assertions.assertThat(fileSingleSlashLocation.equals(StandardLocation)).isTrue();
+ StorageLocation fileTooManySlashesLocation = StorageLocation.of("file://///////path/to/file");
+ Assertions.assertThat(slashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(manySlashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileSingleSlashLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileTooManySlashesLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(standardLocation instanceof AzureLocation).isFalse();
Review Comment:
Cf. `aserThat(...).isExactlyInstanceOf(...)`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2098710239
##########
polaris-core/src/test/java/org/apache/polaris/service/storage/StorageLocationTest.java:
##########
@@ -18,18 +18,63 @@
*/
package org.apache.polaris.service.storage;
+import java.net.URISyntaxException;
import org.apache.polaris.core.storage.StorageLocation;
+import org.apache.polaris.core.storage.azure.AzureLocation;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
public class StorageLocationTest {
@Test
public void testOfDifferentPrefixes() {
- StorageLocation StandardLocation = StorageLocation.of("file:///path/to/file");
+ StorageLocation standardLocation = StorageLocation.of("file:///path/to/file");
StorageLocation slashLeadingLocation = StorageLocation.of("/path/to/file");
+ StorageLocation manySlashLeadingLocation = StorageLocation.of("////////path/to/file");
StorageLocation fileSingleSlashLocation = StorageLocation.of("file:/path/to/file");
- Assertions.assertThat(slashLeadingLocation.equals(StandardLocation)).isTrue();
- Assertions.assertThat(fileSingleSlashLocation.equals(StandardLocation)).isTrue();
+ StorageLocation fileTooManySlashesLocation = StorageLocation.of("file://///////path/to/file");
+ Assertions.assertThat(slashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(manySlashLeadingLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileSingleSlashLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(fileTooManySlashesLocation.equals(standardLocation)).isTrue();
+ Assertions.assertThat(standardLocation instanceof AzureLocation).isFalse();
Review Comment:
Cf. `asertThat(...).isExactlyInstanceOf(...)`
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Java URI validations for Blob Storage providers [polaris]
Posted by "adnanhemani (via GitHub)" <gi...@apache.org>.
adnanhemani commented on code in PR #1604:
URL: https://github.com/apache/polaris/pull/1604#discussion_r2096640150
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/StorageLocation.java:
##########
@@ -44,8 +44,10 @@ protected StorageLocation(@Nonnull String location) {
this.location = URI.create(location.replaceFirst("file:/+", LOCAL_PATH_PREFIX)).toString();
} else if (location.startsWith("/")) {
this.location = URI.create(location.replaceFirst("/+", LOCAL_PATH_PREFIX)).toString();
Review Comment:
No, we do not, but am introducing in revision. Thanks!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org