You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "HonahX (via GitHub)" <gi...@apache.org> on 2026/08/26 21:47:38 UTC
[PR] Fix: StorageUtil.removeRedundantLocations incorrectly drop all equivalent locations [polaris]
HonahX opened a new pull request, #5390:
URL: https://github.com/apache/polaris/pull/5390
## Problem
`removeRedundantLocations` drops a location if it's a "child" of another location in the same set:
```java
for (String potentialParent : locationStrings) {
StorageLocation potentialParentLocation = StorageLocation.of(potentialParent);
for (String potentialChild : locationStrings) {
if (!potentialParent.equals(potentialChild)) {
StorageLocation potentialChildLocation = StorageLocation.of(potentialChild);
if (potentialChildLocation.isChildOf(potentialParentLocation)) {
result.remove(potentialChild);
}
}
}
}
```
The self-comparison guard (`!potentialParent.equals(potentialChild)`) compares **raw strings**, but the actual containment check (`StorageLocation.isChildOf`) compares **normalized** strings: it always adds a trailing slash, and `S3Location`/`AzureLocation` also ignore the scheme.
So two raw strings that look different but denote the same location skip the guard, then test as children of *each other*, and both get removed instead of one.
Example — `location` uses the secure Azure scheme, `write.data.path` uses the non-secure one, for the same path:
```
location: abfss://acct@host.dfs.core.windows.net/mytable
write.data.path: abfs://acct@host.dfs.core.windows.net/mytable
```
Walking the loop with these two strings as `locationStrings`:
- `potentialParent = location`, `potentialChild = write.data.path`: raw strings differ → guard passes → `isChildOf` ignores scheme, so `write.data.path` is a child of `location` → **`write.data.path` removed**.
- `potentialParent = write.data.path`, `potentialChild = location`: same reasoning, symmetrically → **`location` removed**.
Both entries are gone. With only these two in the set, `getLocationsUsedByTable` returns an **empty set**, which fails credential vending (no allowed read/write locations). The same thing happens with a trailing-slash-only mismatch, e.g. `s3://bucket/table` vs `s3://bucket/table/`.
## Fix
Check `isChildOf` in both directions. If it's true both ways, the two strings are the same location (not one containing the other), so keep exactly one instead of removing both:
```java
boolean childIsChildOfParent = potentialChildLocation.isChildOf(potentialParentLocation);
boolean parentIsChildOfChild = potentialParentLocation.isChildOf(potentialChildLocation);
if (childIsChildOfParent && parentIsChildOfChild) {
String toRemove =
potentialParent.compareTo(potentialChild) > 0 ? potentialParent : potentialChild;
result.remove(toRemove);
} else if (childIsChildOfParent) {
result.remove(potentialChild);
}
```
For the example above, `isChildOf` is now true both ways, so exactly one of `location`/`write.data.path` is removed instead of both. Which one doesn't matter: the AWS/Azure credential integrations strip the scheme before building the actual IAM policy/SAS scope either way.
## Testing
Added two tests to `StorageUtilTest`: one for a scheme-only mismatch, one for a trailing-slash-only mismatch. Both reproduce an empty set on the pre-fix code and pass with the fix.
<!--
📝 Describe what changes you're proposing, especially breaking or user-facing changes.
📖 See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [ ] 🛡️ Don't disclose security issues! (contact security@apache.org)
- [ ] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] 💡 Added comments for complex logic
- [ ] 🧾 Updated `CHANGELOG.md` (if needed)
- [ ] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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] Fix: StorageUtil.removeRedundantLocations incorrectly drop all equivalent locations when only scheme differed [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5390: Fix: StorageUtil.removeRedundantLocations incorrectly drop all equivalent locations when only scheme differed
URL: https://github.com/apache/polaris/pull/5390
--
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] Fix: StorageUtil.removeRedundantLocations incorrectly drop all equivalent locations when only scheme differed [polaris]
Posted by "ayushtkn (via GitHub)" <gi...@apache.org>.
ayushtkn commented on code in PR #5390:
URL: https://github.com/apache/polaris/pull/5390#discussion_r3930931939
##########
polaris-core/src/test/java/org/apache/polaris/core/storage/StorageUtilTest.java:
##########
@@ -107,4 +107,35 @@ public void getLocationsUsedByTable() {
IcebergTableLikeEntity.USER_SPECIFIED_WRITE_METADATA_LOCATION_KEY, "/3/")))
.contains("/1/", "/2/", "/3/");
}
+
+ @Test
+ public void getLocationsUsedByTable_trailingSlashOnlyMismatchIsDeduped() {
Review Comment:
The test name uses underscore, the code explicitly discourage this
https://github.com/apache/polaris/blob/c75cafcbaeaabe970eaa93c56ca7d7e2636a5e1f/AGENTS.md?plain=1#L164-L173
--
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] Fix: StorageUtil.removeRedundantLocations incorrectly drop all equivalent locations when only scheme differed [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5390:
URL: https://github.com/apache/polaris/pull/5390#issuecomment-6072668776
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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] Fix: StorageUtil.removeRedundantLocations incorrectly drop all equivalent locations when only scheme differed [polaris]
Posted by "hreddygayam (via GitHub)" <gi...@apache.org>.
hreddygayam commented on PR #5390:
URL: https://github.com/apache/polaris/pull/5390#issuecomment-6192548871
I'll be working on it
--
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