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

Fix TestBufferedCharFilter.Test_Ready failing test, #1102 #1104

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

paulirwin
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Fixes the TestBufferedCharFilter.Test_Ready failing test.

Fixes #1102

Description

Background: CharFilter.IsReady returns true if any calls to Read are guaranteed not to block. Otherwise, it means that a call may or may not block.

This test was failing because StringReader is not a CharFilter and thus it did not have an IsReady property. BufferedCharFilter only is ready if the underlying TextReader is a CharFilter and it returns true for IsReady. So given that BufferedCharFilter already properly cascaded the property, this fixes the failing test by adding a new adapter class to adapt the StringReader to a CharFilter so that it is IsReady-aware. Since StringReaders do not block on calls to Read, this always returns true.

This is perhaps a little bit of a leaky abstraction by having to know these details, but I don't think it really harms much in the design. This property only signals whether Read will block or not, and that's not something .NET TextReader implementations care to surface, nor is it used in Lucene. The only other alternatives here would be to have BufferedCharFilter take a CharFilter instead of a TextReader in its constructor, and that would cascade a bunch of mess to callers further up (if it's even possible to do so) and deviate from upstream, or to implement a Reader abstraction to better match Java and have to adapt all TextReaders to that (and all the extra allocations that would go along with that). I don't think either of those alternatives are worth it just for this property. Also, out of all 8 non-test implementations of CharFilter, only BufferedCharFilter overrides IsReady, and it just delegates it to the inner CharFilter (if it is one) anyways. So while this test ensures that it works correctly for BufferedCharFilter for end user code, it doesn't seem to have much utility in Lucene/Lucene.NET, most likely because these calls would usually block (or at least not be guaranteed to block).

@paulirwin paulirwin added the notes:bug-fix Contains a fix for a bug label Jan 19, 2025
@paulirwin paulirwin requested a review from NightOwl888 January 19, 2025 22:58
Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

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

Yeah, one adapter in a test is a better solution than having several adapters in production code. Looks good.

@paulirwin paulirwin merged commit 013f962 into apache:master Jan 20, 2025
8 checks passed
@paulirwin paulirwin deleted the issue/1102 branch January 20, 2025 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:bug-fix Contains a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing test: TestBufferedCharFilter.Test_Ready
2 participants