Replace chained instanceof filter dispatch with the existing FilterVisitor

[vharseko]

Summary

Closes the last remaining CodeQL note-severity category, java/chained-type-tests (4 alerts).

• ObjectNormalizerFacade.normalizeFilter (a 12-way instanceof chain) and AbstractFilterTranslator.createLeafExpression (a 9-way instanceof chain) both dispatch on the framework's own Filter subtypes. The framework already ships a FilterVisitor interface for exactly this purpose (FilteredResultsHandlerVisitor is prior art). Both methods are rewritten as visitor dispatch instead of an instanceof chain.
  • AbstractFilterTranslator's visitor is a private, non-static inner class, so its visitXxxFilter methods can call the enclosing instance's protected createXxxExpression(...) methods. This keeps the point of the class: connector subclasses override those methods.
  • Null safety: the original instanceof chain was implicitly null-safe, because instanceof on null is always false and falls through to the else branch. filter.accept(...) throws NullPointerException on a null filter instead. The fix restores the null check at both entry points, and routes the AND/OR/NOT recursion in ObjectNormalizerFacade back through the null-checked public method rather than calling .accept directly on sub-filters that may be null. A null leaf reaches the translator's guard through translate(new AndFilter(eq, null)) and translate(new NotFilter(null)).
• Behaviour change for third-party filters: a Filter implementation outside the built-in set used to fall through the instanceof chain. The normalizer returned it unchanged and the translator returned null ("everything"). Now the filter's own accept(FilterVisitor, P) picks the branch. Every in-repo custom filter (PassThroughFilter, RangeFilter, TrueFilter, FalseFilter, plus PresenceFilter and ExtendedMatchFilter) dispatches to visitExtendedFilter, which keeps the old fallback. The default search path already evaluated such filters through FilteredResultsHandlerVisitor.
• The other two java/chained-type-tests alerts are dismissed on GitHub as won't-fix. EqualsHashCodeBuilder.append dispatches on primitive array component types, and SQLUtil's JDBC parameter binding dispatches on Integer/Double/Blob/Timestamp/... Both dispatch on final JDK types we don't own, so a fix based on a visitor or on polymorphism is not possible.

Test plan

• New tests in ObjectNormalizerFacadeTests: testPresenceFilterPassedThroughUnchanged, testNullFilterReturnsNull (top-level null), testNullSubFilterIsPassedThrough (a null left or right child of AND and of OR, a null NOT operand, and a one-element AndFilter).
• New test in FilterTranslatorTests: testNullLeafTranslatesToEverything (null leaf under AND and NOT).
• Mutation check: deleting the translator's null guard makes testNullLeafTranslatesToEverything fail with an NPE. In the normalizer, calling child.accept(...) directly at any one of the five composite sites (left or right child of AND, left or right child of OR, the NOT operand), with the other four left intact, makes testNullSubFilterIsPassedThrough fail with an NPE.
• mvn install -DskipITs on connector-framework, connector-framework-internal, OpenICF-ldap-connector, OpenICF-dbcommon and OpenICF-xml-connector, including their existing filter-translator test suites: all green.

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[maximthomas]

praise: Both instanceof chains now go through the framework's own FilterVisitor, and connector extensibility and the old fallbacks are kept.

• LeafExpressionVisitor is a non-static inner class (AbstractFilterTranslator.java:425), so connector overrides of the protected createXxxExpression methods still bind.
• Every in-repo custom Filter (PassThroughFilter, RangeFilter, TrueFilter, FalseFilter) dispatches to visitExtendedFilter, which returns the filter unchanged in the normalizer and null in the translator, the same as the old else branch.
• The implicit null-safety of instanceof was noticed and restored at both entry points (ObjectNormalizerFacade.java:150, AbstractFilterTranslator.java:412).

---

issue (non-blocking): No test pins the null guard restored in AbstractFilterTranslator.createLeafExpression.

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/objects/filter/AbstractFilterTranslator.java:412-414

The description presents testNullFilterReturnsNull as the regression test for a null fix made in both files, but that test only calls ObjectNormalizerFacade.normalizeFilter(null). A null leaf reaches this guard from translate(new AndFilter(eq, null)) (through simplifyAndDistribute, :279) and from translate(new NotFilter(null)) (through normalizeNot → negate(null) → :392). Without the guard, both throw an NPE where the base returned "everything". Measured: with the guard deleted, FilterTranslatorTests stays green (7/7 pass).

@Test
public void testNullLeafTranslatesToEverything() {
    Filter eq = FilterBuilder.equalTo(AttributeBuilder.build("a", "a"));
    assertEquals(new AllFiltersTranslator().translate(new AndFilter(eq, null)),
            new AllFiltersTranslator().translate(eq));
    assertThat(new AllFiltersTranslator().translate(new NotFilter(null))).isEmpty();
}

Pin: once the guard is deleted, both assertions fail with an NPE.

---

issue (non-blocking): No test pins routing AND/OR/NOT sub-filters through the null-checked normalizeFilter.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/operations/ObjectNormalizerFacade.java:168, :208, :212

testNullFilterReturnsNull passes null only at the top level (:150), and testAnd/testOr/testNot use only non-null children. new NotFilter(null) and new AndFilter(x, null) are legal, and so is a one-element AndFilter(Collection), whose getRight() is null. Measured: with the three visit methods changed to call child.accept(this, p) directly, ObjectNormalizerFacadeTests stays green (16/16 pass, testNullFilterReturnsNull included), even though normalizeFilter(new NotFilter(null)) then throws an NPE.

@Test
public void testNullSubFilterIsPassedThrough() {
    ObjectNormalizerFacade normalizer = createTestNormalizer();
    assertNull(((NotFilter) normalizer.normalizeFilter(new NotFilter(null))).getFilter());
    assertNull(((AndFilter) normalizer.normalizeFilter(
            new AndFilter(FilterBuilder.contains(createTestAttribute()), null))).getRight());
    assertNull(((OrFilter) normalizer.normalizeFilter(
            new OrFilter(FilterBuilder.contains(createTestAttribute()), null))).getRight());
}

Pin: this needs imports for AndFilter, NotFilter, OrFilter and the static org.testng.Assert.assertNull. Each assertion fails with an NPE under that mutant.

---

nitpick (non-blocking): The NormalizingFilterVisitor Javadoc says extended filters carry no attribute, but ExtendedMatchFilter does.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/operations/ObjectNormalizerFacade.java:157-160, :185-187

ExtendedMatchFilter extends AttributeFilter and dispatches to visitExtendedFilter, which returns it as is, so its attribute value is never normalized. That behaviour is unchanged from the base; the comment just gives the wrong reason for it.

/**
 * Applies {@link #normalizeAttribute(Attribute)} to every attribute
 * referenced by a filter, recursing through composite (AND/OR/NOT)
 * filters. Filters dispatched to visitExtendedFilter (presence,
 * extended-match, pass-through and custom filters) are returned
 * unchanged; an ExtendedMatchFilter's attribute is not normalized.
 */

---

note (non-blocking): A behaviour change the description does not mention.

• ObjectNormalizerFacade.java:153, AbstractFilterTranslator.java:415: a third-party Filter outside the built-in set used to fall through the instanceof chain (returned unchanged, or null). Now its own accept(FilterVisitor, P) picks the branch. No in-repo implementation is affected, and the default search path already evaluated such filters through FilteredResultsHandlerVisitor, so this was judged not a defect.

[vharseko]

Addressed in the new commit, rebased onto current master:

• Null guard in AbstractFilterTranslator.createLeafExpression: added FilterTranslatorTests.testNullLeafTranslatesToEverything, the test you proposed (AndFilter(eq, null) and NotFilter(null)). With the guard deleted, it fails with an NPE.
• AND/OR/NOT recursion through the null-checked normalizeFilter: added ObjectNormalizerFacadeTests.testNullSubFilterIsPassedThrough, covering NotFilter(null), AndFilter(x, null), OrFilter(x, null) and a one-element AndFilter(Collection). When the visit methods call child.accept(this, p) directly, it fails with an NPE.
• NormalizingFilterVisitor Javadoc: it now says that filters dispatched to visitExtendedFilter (presence, extended-match, pass-through, custom) are returned unchanged, and that an ExtendedMatchFilter's attribute is not normalized.
• Third-party Filter dispatch: the PR description now notes the behaviour change. The code is unchanged.

Also in this commit: @Override on all 26 visitXxx methods (this clears the java/missing-override-annotation alerts CodeQL raised on this PR), and a malformed copyright line in ObjectNormalizerFacadeTests is fixed.

[maximthomas]

praise: both of the new null-safety tests do their job, and the downstream suites listed in the test plan run and pass in CI.

• Mutation claims checked on this head. Deleting the guard at AbstractFilterTranslator.java:412-414 makes FilterTranslatorTests.testNullLeafTranslatesToEverything fail with an NPE (:499). Calling child.accept(this, p) at every composite site makes ObjectNormalizerFacadeTests.testNullSubFilterIsPassedThrough fail (:187).
• NormalizingFilterVisitor is a stateless singleton that receives the facade as P (ObjectNormalizerFacade.java:163-166), so dispatch creates no visitor objects.
• OpenICF-ldap-connector, OpenICF-dbcommon and OpenICF-xml-connector are modules of the root reactor, and build-maven passes at dcc2e099 on all 9 OS/JDK cells.

---

issue (non-blocking): testNullSubFilterIsPassedThrough never builds a null left child, so no test covers the null-safe routing of getLeft() in visitAndFilter/visitOrFilter.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/operations/ObjectNormalizerFacade.java:170, :224; OpenICF-java-framework/connector-framework-internal/src/test/java/org/identityconnectors/framework/impl/api/local/operations/ObjectNormalizerFacadeTests.java:184-195

In the test, every null child is either on the right or inside a NOT. I replaced p.normalizeFilter(filter.getLeft()) with filter.getLeft().accept(this, p) at :170 and :224 only, and ObjectNormalizerFacadeTests still passes (17 run, 0 failed). Both new AndFilter(null, eq) and new OrFilter(null, eq) are legal, because CollectionUtil.newList keeps a null left. With that mutant they throw an NPE, where this head returns the null left unchanged. The mutation check in the description is therefore true only when all sites are mutated together.

        assertNull(((AndFilter) normalizer.normalizeFilter(new AndFilter(null, contains)))
                .getLeft());
        assertNull(((OrFilter) normalizer.normalizeFilter(new OrFilter(null, contains)))
                .getLeft());

Pin: add these two asserts to testNullSubFilterIsPassedThrough. Both fail with an NPE under the left-only mutant and pass at this head.

---

issue (non-blocking): normalizeFilter now uses three stack frames for each AND/OR level; before this PR it used one.

OpenICF-java-framework/connector-framework-internal/src/main/java/org/identityconnectors/framework/impl/api/local/operations/ObjectNormalizerFacade.java:153, :168-172, :222-226

The recursion is now normalizeFilter -> Filter.accept -> visitAndFilter/visitOrFilter -> normalizeFilter. I measured it under -Xint on a thread with a 1M stack, running the normalizer and then the translator, in the same order as SearchImpl. A right-nested OR or AND of N EqualsFilters passes at the base (e6bad438) for every N up to 2500. At this head, normalizeFilter throws StackOverflowError above N = 2042. Two repeats gave the same numbers. With a warm JIT I could not reproduce the difference. Only filters more than 2000 levels deep running in interpreted mode are affected. I am not asking for a change: this is the price of visitor dispatch.

---

issue (non-blocking): This bug predates the PR: OrFilter.getRight() returns an AndFilter when there are three or more sub-filters, so the rewritten visitOrFilter turns FilterBuilder.or(a, b, c) into OR(a, AND(b, c)).

OpenICF-java-framework/connector-framework/src/main/java/org/identityconnectors/framework/common/objects/filter/OrFilter.java:77-87, ObjectNormalizerFacade.java:224-225, AbstractFilterTranslator.java:138

FilterBuilder.or(Collection) and or(Filter...) build a flat OrFilter (FilterBuilder.java:456-479). For a list longer than two, getRight() copies everything after the first element into new AndFilter(right). visitOrFilter then builds new OrFilter(norm(getLeft()), norm(getRight())). As a result, an object that matches only b, or only c, no longer matches the filter that SearchImpl passes to the connector. AbstractFilterTranslator.translate() -> normalizeNot() (:111, :138) walks the same getLeft()/getRight() pair. I traced this by reading the code and did not run it. The base behaves the same way, so this belongs in a separate issue. The fix is one line in OrFilter, and it corrects every walker that uses getLeft()/getRight():

            right.removeFirst();
            return new OrFilter(right);

Pin: build a, b and c as EqualsFilters on different values. normalizeFilter(FilterBuilder.or(a, b, c)).accept(obj) should be true for an obj that matches only c; at this head it is false.

[vharseko]

Addressed in the new commit, rebased onto current master:

• Null left child of AND/OR: added the two asserts you proposed (AndFilter(null, contains) and OrFilter(null, contains)) to testNullSubFilterIsPassedThrough. I re-ran the mutation check one site at a time. Calling child.accept(this, p) directly at any one of the five composite sites (left or right child of AND, left or right child of OR, the NOT operand), with the other four left intact, makes the test fail with an NPE (:187–:194). The "Mutation check" line in the description now says exactly that.
• Three stack frames per AND/OR level: left as is, as you suggested. This is the price of visitor dispatch, and the difference only shows for filters more than 2000 levels deep in interpreted mode.
• OrFilter.getRight() returning an AndFilter: confirmed. It is out of scope for this PR, so I opened OrFilter.getRight() returns an AndFilter for three or more sub-filters #150. It reaches further than the normalizer and the translator: FilterHandlers.CompositeFilterHandler serializes getLeft()/getRight(), so a remote connector server receives or(a, b, c) as OR(a, AND(b, c)), and the groovy MapFilterVisitor hands scripts the same tree.

[maximthomas]

praise: the new asserts close the last gap in the null-safety tests. Each child site in the visitor now has its own test.

• ObjectNormalizerFacadeTests.java:192-195 adds new AndFilter(null, contains) and new OrFilter(null, contains). I mutated one site at a time, replacing p.normalizeFilter(...) with child.accept(this, p) in ObjectNormalizerFacade.java. Each of the five mutants makes testNullSubFilterIsPassedThrough fail with an NPE on its own line: NOT :187, AND-right :188, OR-right :190, AND-left :192, OR-left :194. The unmutated head passes, 17 run / 0 failed.
• The change touches only tests. Rebased onto the new base, the two existing commits are unchanged, and build-maven passes at 3514d47e on all 9 OS/JDK cells (JDK 11-26).