Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LUCENE-10526: add single method to mockfile to wrap a Path #822

Merged
merged 3 commits into from
Apr 20, 2022

Conversation

rmuir
Copy link
Member

@rmuir rmuir commented Apr 20, 2022

Currently "new FilterPath" is called from everywhere, making it impossible for a mockfilesystem to use a custom subclass.

See JIRA for full description.

The use case here is to e.g. allow WindowsFS to wrap with a custom subclass that checks for special characters that windows doesn't allow. But today it can't do that since there is code everywhere doing the wrapping.

This PR adds a single method wrapPath() to FilterFileSystemProvider to do this wrapping, and refactors everything to use it.

Currently "new FilterPath" is called from everywhere, making it
impossible for a mockfilesystem to use a custom subclass.
Copy link
Member

@mikemccand mikemccand left a comment

Choose a reason for hiding this comment

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

This looks awesome @rmuir -- now we consistently call provider.wrapPath instead of new FilterPath allowing us to make improvements like LUCENE-10525 (properly blocking illegal windows-only filesystem characters) far easier. Thanks!!

Copy link
Contributor

@gautamworah96 gautamworah96 left a comment

Choose a reason for hiding this comment

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

I left just one comment from my limited understanding of the mock FS APIs.
The plan described in the LUCENE-10525 JIRA does make sense. This opens up avenues to improve other FS systems with specific customizations as well (going well beyond the original motivation!).

@rmuir
Copy link
Member Author

rmuir commented Apr 20, 2022

@gautamworah96 care to take another look? I think fixing the tiny nit was helpful to our tests. now it is easier for tests to wrap a path with one of these mock filesystems explicitly, as they don't have to deal with URI class or pass around FileSystem objects. thanks!

Copy link
Contributor

@gautamworah96 gautamworah96 left a comment

Choose a reason for hiding this comment

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

LGTM. I mocked a bit of the WindowsPath locally and placing the filename checks there make sense.

@rmuir
Copy link
Member Author

rmuir commented Apr 20, 2022

Thanks for reviewing, and good luck improving the act-like-Windows!

@rmuir rmuir merged commit 844bd88 into apache:main Apr 20, 2022
asfgit pushed a commit that referenced this pull request Apr 20, 2022
Currently "new FilterPath" is called from everywhere, making it impossible for a mockfilesystem to use a custom subclass.
Add FilterFileSystemProvider.wrapPath(path), which subclasses can override. Fix tests to use it instead of juggling URI objects and passing FileSystems around.
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.

None yet

3 participants