Skip to content

Add tests for forbidden file and HTTP locations in CatalogUtils#4638

Open
hemasrishalini wants to merge 2 commits into
apache:mainfrom
hemasrishalini:catalogutils-storage-validation-test
Open

Add tests for forbidden file and HTTP locations in CatalogUtils#4638
hemasrishalini wants to merge 2 commits into
apache:mainfrom
hemasrishalini:catalogutils-storage-validation-test

Conversation

@hemasrishalini

Copy link
Copy Markdown
Contributor

Adds unit tests for CatalogUtils.validateLocationsForTableLike to ensure that:

  • file:/ locations are rejected when FILE storage is not enabled
  • http:/ locations are rejected under the same restriction

This verifies enforcement of storage location validation when SUPPORTED_CATALOG_STORAGE_TYPES does not include FILE.

The tests cover the fallback validation path when no storage restrictions are resolved from entity hierarchy.

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)


Assertions.assertThatThrownBy(
() ->
CatalogUtils.validateLocationsForTableLike(

@dimas-b dimas-b Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a correct test, but I'm not sure it is worth adding, TBH 🤷

In runtime, these locations would be rejected by PolarisServiceImpl.validateStorageConfig() and associated child location checks.

Do you actually have a use case where the exception triggered by this test happens in a deployed Polaris Server?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimas-b Thanks for reviewing.

You're right that these locations would normally be validated earlier through PolarisServiceImpl.validateStorageConfig() and related checks.

My intention was to add coverage for the defensive validation that still exists in CatalogUtils.validateLocationsForTableLike(), since the method explicitly rejects file: and http: locations when FILE storage is not enabled. I thought it would help guard against regressions if the validation flow changes in the future.

That said, I understand the concern about the practical value of this path and am happy to follow your recommendation if you think this test isn't needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what other reviewers think 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants