Add additional options for catalog test implementations #4906
Conversation
| return false; | ||
| } | ||
|
|
||
| protected boolean supportsNamesWithSlashes() { |
There was a problem hiding this comment.
Why is this not supported? Slashes should be properly escaped in paths, which is why we have this test.
There was a problem hiding this comment.
For REST based catalogs, some servers don't allow escaped slashes in the path by default, like Tomcat. There are workarounds but it can open up potential security issues, so some may choose not to support this.
There was a problem hiding this comment.
Is that for escaped slashes in the typical way or for the REST catalogs escaped slashes that use %00 (Null byte)?
I’m good with not requiring that server implementations require this (I’m almost certain it’s already not required). But would want to ensure the spec is up to date on that re: having to support it or not. But if the null-byte can open up security problems (which in general it can) then I can understand people deciding tables / namespaces really don’t benefit from having slashes in them.
There was a problem hiding this comment.
The null byte is used as a delimiter AFAIK, the slash is just URL encoded, i.e. %2F
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
Outdated
Show resolved
Hide resolved
|
Thanks, @bryanck! |
This PR adds a couple of options to the catalog tests so it is more flexible for different catalog implementations. The first option is to support testing against catalogs that manage the table location. The second option is to support testing against catalogs that do not support slashes in the namespace or table name.