Skip to content

Conversation

@nfsantos
Copy link
Contributor

The includedPaths and excludedPaths in an index definition must be absolute. If they are not or if they start with a space, then the indexing job may fail or hang. This PR adds a check to the PathFilter constructor to check that all paths are absolute and fail immediately if they are not.

private static void checkPathsAreAbsolute(Iterable<String> paths, String pathType) {
for (String path : paths) {
if (!PathUtils.isAbsolute(path)) {
throw new IllegalStateException("Invalid path in " + pathType + " paths list: " + path + ". Paths must be absolute.");
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be better to throw an IllegalArgumentException instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it would be better, but the PathFilter constructor is already throwing IllegalStateException in a check it does in the constructor, so I prefer to keep it consistent and use the same exception.

@steffenvan
Copy link
Contributor

Should we add a test case where one of the paths start with a space? On another note, I thought we trimmed those string values?

@nfsantos
Copy link
Contributor Author

Should we add a test case where one of the paths start with a space? On another note, I thought we trimmed those string values?

One of the tests is already testing paths that start with space. Let me know if you think we should add more tests.

@steffenvan
Copy link
Contributor

Oh right my eyes must have deceived me. That one space character can be quite subtle, I can understand why someone might miss it

@nfsantos nfsantos merged commit ecfd1f6 into apache:trunk Nov 29, 2024
@nfsantos nfsantos deleted the OAK-11285 branch November 29, 2024 07:39
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.

4 participants