dweiss commented on PR #15469:
URL: https://github.com/apache/lucene/pull/15469#issuecomment-3740242050
Right. I've mentioned it before but the decorator pattern is basically
always prone to braking something. The thing we missed here is indeed the
strong relationship between nextDoc and intoBitSet.
Basically, whenever there is a class that wraps another one, it should
provide these two methods consistently - either it knows how to implement them
in the filter subclass or it shouldn't touch them at all. TestFilterLeafReader
has a nice example where it implements this:
```
private static class TestPositions extends FilterPostingsEnum {
public TestPositions(PostingsEnum in) {
super(in);
}
/** Scan for odd numbered documents. */
@Override
public int nextDoc() throws IOException {
int doc;
while ((doc = in.nextDoc()) != NO_MORE_DOCS) {
if ((doc % 2) == 1) return doc;
}
return NO_MORE_DOCS;
}
}
```
This will clearly result in a different set of bits if intoBitSet stays
delegated.
It is possible to automatically detect and enforce such tightly coupled
overrides - I've written a quick example here using archunit -
https://github.com/apache/lucene/commit/afeda079b4329a9753444eaad1d4f99fda0b7351
this only checks subclasses of FilterPostingsEnum but it immediately fails
with the offender:
```
java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule
'classes that are assignable to
org.apache.lucene.index.FilterLeafReader$FilterPostingsEnum and are not
interfaces should override methods consistently: [nextDoc, intoBitSet]' was
violated (1 times):
Class org.apache.lucene.index.TestFilterLeafReader$TestReader$TestPositions
(TestFilterLeafReader.java:0) must override the following methods consistently:
[nextDoc, intoBitSet] but only overrides: [nextDoc]
at
__randomizedtesting.SeedInfo.seed([77710A3FDD5F4CD4:EB7D1F2EF63D9025]:0)
at
com.tngtech.archunit.lang.ArchRule$Assertions.assertNoViolation(ArchRule.java:94)
at com.tngtech.archunit.lang.ArchRule$Assertions.check(ArchRule.java:86)
at
com.tngtech.archunit.lang.ArchRule$Factory$SimpleArchRule.check(ArchRule.java:165)
at
com.tngtech.archunit.lang.syntax.ObjectsShouldInternal.check(ObjectsShouldInternal.java:81)
at
org.apache.lucene.TestDesignConstraints.testFilterClasses(TestDesignConstraints.java:42)
at
java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:565)
```
There doesn't seem to be the right way to do it or at least I can't think of
any.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
