Fix: check for slashless metadata paths #968
Conversation
RichardLiu2001
force-pushed
the
check-slashless-metadata-paths
branch
from
8461a5d to
edb49b3
Compare
| int lastSlashIndex = metadataFileLocation.lastIndexOf("/"); | ||
| Preconditions.checkArgument( | ||
| lastSlashIndex != -1, | ||
| "Invalid metadata file location: %s, must be a full path to a file", |
There was a problem hiding this comment.
The presence of a slash does not mean the path is full, e.g. ../relative
There was a problem hiding this comment.
Right, but this indicates the absence of a slash, which means the path is not full.
There was a problem hiding this comment.
Yeah, but it is kind of misleading to check for condition X and then print an error message saying condition (X + Y) is not met.
How about...
Invalid metadata file location; metadata file location should be absolute and must contain `/`: %s
There was a problem hiding this comment.
To be clear, I don't think making this check for an absolute path is strictly a blocker here; we can file an issue to track that if need be. But let's please try to swiftly get rid of the 500
| // Throw an exception if this table already exists in the catalog. | ||
| if (tableExists(identifier)) { | ||
| throw new AlreadyExistsException("Table already exists: %s", identifier); | ||
| } | ||
|
|
||
| String locationDir = metadataFileLocation.substring(0, metadataFileLocation.lastIndexOf("/")); | ||
| String locationDir = metadataFileLocation.substring(0, lastSlashIndex); |
There was a problem hiding this comment.
What if metadataFileLocation is s3://my-bucket?
There was a problem hiding this comment.
Since there is no .metadata.json in the file name, that would fail the file name format checks in https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TableMetadataParser.java#L68 when we try to read the metadata file, giving a 400.
RichardLiu2001
requested review from
adutra,
ashvina,
dennishuo,
eric-maynard,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain and
ebyhr
as code owners
RichardLiu2001
changed the title
Polaris currently returns a 500 error for the
registerTablecatalog API if the provided metadata file path does not contain any slashes.lastIndexOf("/")will return-1, causing anRange [0, -1) out of boundserror when callingsubstring().