LatLonPoint.newGeometryQuery with CONTAINS matches nothing instead of throwing for an empty or null-holding geometry array

LatLonPoint.newGeometryQuery(field, queryRelation, latLonGeometries...) and LatLonDocValuesField.newSlowGeometryQuery document @throws IllegalArgumentException when the geometry array "is null, empty or contain a null". For INTERSECTS, WITHIN and DISJOINT the query constructors enforce it. For CONTAINS both factories take a shortcut first that checks only the geometry type (instanceof Point), not null or empty input: an empty array gives ConstantScore() over zero clauses, and a null element gives MatchNoDocsQuery("... with non-point geometries"). Both match no document, so a caller's mistake (an empty list of points, a null slot) becomes an empty result with no error. The doc-values factory still rejects the empty array; the null element is silent in both. Valid input is not affected by this defect. Introduced in 8.8.0 by LUCENE-9641; present in every release through 10.6.0 and on main.

1. The contract. document/LatLonPoint.java:314-326: "@param latLonGeometries array of LatLonGeometries. must not be null or empty." and "@throws IllegalArgumentException if field is null, queryRelation is null, latLonGeometries is null, empty or contain a null" (:323-324). document/LatLonDocValuesField.java:243-246 has the same clause and adds "or line geometry"; that line part is not claimed here, because under CONTAINS a line gets the MatchNoDocsQuery the introducing PR's review settled for non-point geometries (:255-261).
2. The checks live in the query constructors. new LatLonPointQuery(...) (LatLonPoint.java:340) goes through SpatialQuery (field and relation, document/SpatialQuery.java:74-85) and LatLonGeometry.create (geo/LatLonGeometry.java:24-45). new LatLonDocValuesQuery(...) (LatLonDocValuesField.java:263) does its own field and relation checks (document/LatLonDocValuesQuery.java:57-62) and calls LatLonGeometry.create (:87). LatLonGeometry.create rejects an empty array and a null element (:28-40); its null-array check (:25-26) is never reached from LatLonPoint, where a null array fails earlier with NullPointerException (LatLonPoint.java:329, :345 or SpatialQuery.java:83).
3. CONTAINS returns before them. LatLonPoint.java:337-338 sends CONTAINS to makeContainsGeometryQuery (:343-355), which adds one MUST clause per Point and returns MatchNoDocsQuery for anything else (:346-348).
   • Empty array: the loop body never runs, and :354 returns new ConstantScoreQuery(builder.build()) over zero clauses, which matches nothing (it prints as ConstantScore(); at search time the empty BooleanQuery rewrites to MatchNoDocsQuery("empty BooleanQuery"), search/BooleanQuery.java:268-271).
   • Null element: (null instanceof Point) == false is true at :346, so the null takes the non-point branch.
4. The doc-values twin. LatLonDocValuesField.java:255-261 has the same CONTAINS loop and the same null-element outcome; its empty array falls through to LatLonDocValuesQuery and is rejected there.
5. Not claimed. MatchNoDocsQuery for a valid line or polygon under CONTAINS is intended. The null-array NullPointerException fails loudly (only the exception type differs), and a null field with an empty array needs a second invalid argument.

• Who. Code that passes an empty or null-holding array to LatLonPoint.newGeometryQuery (or LatLonDocValuesField.newSlowGeometryQuery, null element) with CONTAINS, for example a list of user-picked points that can be empty. Nothing tells the application its input was invalid.
• Default vs opt-in. Query construction only: no configuration, any index and codec, assertions on and off. CONTAINS on a point field is a niche relation, and no public report was found since 8.8.0 (2021), so the likelihood is rare.
• Needs invalid input. This defect changes nothing for valid geometries (valid multi-point CONTAINS on newSlowGeometryQuery has a separate defect, LFV-701). The documented IndexOrDocValuesQuery pairing still throws for the empty array, because the doc-values half rejects it; the null element is silent in both halves.
• Scale. One call; no index needed; deterministic.

TestLatLonPointContainsNoGeometries (one method, extends LuceneTestCase, package org.apache.lucene.document of the lucene/core tests, Apache license header). It is the hunt's minimal test with its comment shortened to pass checkGoogleJavaFormat. It covers the empty array, the simplest of the two silent cases; the null element follows from the same shortcut and is shown by the diagnostic below. Run with seed DEADBEEF; the hunt's equivalent tests also failed with seeds 4C0A0001, 4C0A0002, 12345678 and CAFEBABE.

cp $REPRO/TestLatLonPointContainsNoGeometries.java $LUCENE/lucene/core/src/test/org/apache/lucene/document/
cd $LUCENE && ./gradlew :lucene:core:test --tests org.apache.lucene.document.TestLatLonPointContainsNoGeometries -Ptests.seed=DEADBEEF

# gradle, scratch worktree at 5adff78bb168, -Ptests.seed=DEADBEEF (assertions on; -Ptests.asserts=false gives the same)
TestLatLonPointContainsNoGeometries > testContainsWithNoGeometriesThrows FAILED
    junit.framework.AssertionFailedError: Expected exception IllegalArgumentException but no exception was thrown
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:D113C0FEC8040294]:0)
        at app//org.apache.lucene.tests.util.LuceneTestCaseParent.expectThrows(LuceneTestCaseParent.java:1432)
        at app//org.apache.lucene.tests.util.LuceneTestCaseParent.expectThrows(LuceneTestCaseParent.java:1418)
        at app//org.apache.lucene.document.TestLatLonPointContainsNoGeometries.testContainsWithNoGeometriesThrows(TestLatLonPointContainsNoGeometries.java:26)
1 test completed, 1 failed

# with the validate-first fix: 1 test, 0 failures (assertions on and off)

# what the factories return (public-API diagnostic on main, assertions off, one document at (0,0))
P.newGeometryQuery(f, CONTAINS, []) -> ConstantScoreQuery [ConstantScore()] hits=0
P.newGeometryQuery(f, INTERSECTS, []) -> IllegalArgumentException: geometries must not be empty
P.newGeometryQuery(f, WITHIN, []) -> IllegalArgumentException: geometries must not be empty
DV.newSlowGeometryQuery(f, CONTAINS, []) -> IllegalArgumentException: geometries must not be empty
P.newGeometryQuery(f, CONTAINS, [Point(0,0), null]) -> MatchNoDocsQuery [MatchNoDocsQuery("Contains LatLonPoint.newGeometryQuery with non-point geometries")] hits=0
P.newGeometryQuery(f, INTERSECTS, [Point(0,0), null]) -> IllegalArgumentException: geometries[1] must not be null
DV.newSlowGeometryQuery(f, CONTAINS, [null]) -> MatchNoDocsQuery [MatchNoDocsQuery("Contains LatLonDocValuesField.newSlowGeometryQuery with non-point geometries")] hits=0
P.newGeometryQuery(f, CONTAINS, [Point(0,0)]) -> ConstantScoreQuery [ConstantScore(+LatLonPointQuery: field=f:[Point(0.0,0.0),])] hits=1

• The contract is explicit and current. The @throws clause names these inputs for every relation, in both factories. LUCENE-9641 (14009f4424d), which added CONTAINS and the shortcut, rewrote the clause in the same diff and kept "null, empty or contain a null" (git blame).
• The rest of the API enforces it. INTERSECTS, WITHIN, newPolygonQuery and newSlowGeometryQuery(CONTAINS) throw "geometries must not be empty" for the empty array; INTERSECTS throws "geometries[i] must not be null" for a null element (executed). Before LUCENE-9641, on unreleased master, the constructors of the removed LatLonPointInGeometryQuery and LatLonDocValuesPointInGeometryQuery rejected all of these inputs.
• Tests. No upstream test passes an empty or null-holding array with any relation; BaseGeoPointTestCase.testPolygonNullField (lucene/test-framework/.../tests/geo/BaseGeoPointTestCase.java:472-483) treats these @throws clauses as tested contract for the field argument.

The case for intended (the devil's-advocate verifier and the cross-model refute lens): (a) @param says the array "must not be null or empty", so the caller misuses the API; (b) in the review of lucene-solr#2155 a reviewer (nknize) suggested throwing for non-point geometries under CONTAINS, and the author (iverase) answered "Still I return a MatchNoDocs query for the Query builders"; (c) LatLonShape.newGeometryQuery(f, CONTAINS) with an empty array also returns ConstantScore(); (d) an empty set matching nothing is common in Lucene.

Why it fails. (a) The javadoc does not stop at the precondition: it promises the exception for exactly these inputs. (b) The review covered valid non-point geometries, a relation points can never satisfy, not invalid arrays. (c) LatLonShape's empty case appears to be a historical consequence of LUCENE-9225 (286d22717b2, 8.5.0), which dropped a CONTAINS && length > 1 guard that XYShape.newGeometryQuery still has (XYShape throws), and for a null element LatLonShape throws "geometries[0] must not be null" while LatLonPoint returns MatchNoDocsQuery (executed). (d) Here the API documents the exception and enforces it on every other path. The cross-model reviewer rated the misuse defence weak against the explicit @throws; it does hold for impact, hence Low.

Fix A was built and checked through gradle on 5adff78bb168 in a scratch worktree: the repro with assertions on and off, then the repro with every org.apache.lucene.document.TestLatLon* and org.apache.lucene.search.TestLatLon* suite, seed DEADBEEF, and checkGoogleJavaFormat. Option B was not run. The regression test is the one repro test, unchanged; it covers only the empty LatLonPoint case, while the null element and the doc-values factory, which fix A also changes, are shown by the diagnostic in the repro section, not by a test.

Add LatLonPoint.checkGeometryQueryArguments(field, queryRelation, latLonGeometries) (package-private) and call it as the first statement of newGeometryQuery (LatLonPoint.java:327-328) and of newSlowGeometryQuery (LatLonDocValuesField.java:248-249); +27/-0, messages as in LatLonGeometry.create.

  static void checkGeometryQueryArguments(
      String field, ShapeField.QueryRelation queryRelation, LatLonGeometry... latLonGeometries) {
    if (field == null) {
      throw new IllegalArgumentException("field must not be null");
    }
    if (queryRelation == null) {
      throw new IllegalArgumentException("queryRelation must not be null");
    }
    if (latLonGeometries == null) {
      throw new IllegalArgumentException("geometries must not be null");
    }
    if (latLonGeometries.length == 0) {
      throw new IllegalArgumentException("geometries must not be empty");
    }
    for (int i = 0; i < latLonGeometries.length; i++) {
      if (latLonGeometries[i] == null) {
        throw new IllegalArgumentException("geometries[" + i + "] must not be null");
      }
    }
  }

Trade-offs

Implements the null, empty and null-element part of both @throws clauses in one place, before every shortcut; the doc-values clause's "or line geometry" is left as it is. Side effects, all towards the documented behaviour: a null array gives IllegalArgumentException instead of NullPointerException (as newPolygonQuery did in 8.7.0), and so does a null field with an empty CONTAINS array. Callers that pass an empty CONTAINS array today get an exception instead of zero hits: a behaviour change, so consider a CHANGES entry. Cost: one linear null scan per query construction, run first on every path (the single-geometry INTERSECTS shortcuts included); the paths that reach LatLonGeometry.create scan the array again there.

What the checks validated

The repro passes with assertions on and off; the repro plus every document.TestLatLon* and search.TestLatLon* suite (22 classes): 236 tests, 0 failures, 19 skipped; :lucene:core:checkGoogleJavaFormat passes with the fix and the test in place. The hunt validated the LatLonPoint half alone the same way (repro passes; 41 TestLatLonPointQueries tests pass).

Throw for length == 0 before the loop in makeContainsGeometryQuery, and for geometry == null inside both CONTAINS loops (LatLonPoint.java:345-346, LatLonDocValuesField.java:256-257).

Trade-offs

Smallest change, leaves the other paths untouched. Keeps the undocumented NullPointerException for a null array and a second copy of the checks.

What the checks validated

Not run.

Change the javadoc so that an empty or null-holding array under CONTAINS is documented to match nothing.

Trade-offs

Keeps the empty array inconsistent with the other relations and with the doc-values twin, and keeps hiding the caller's mistake.

What the checks validated

Not run.

Method: semi-formal (a proof-obligation audit of a class's contract, witnessed by an executable repro).

• The audit checked the paths of the two factories (LatLonPoint.java:314-355, LatLonDocValuesField.java:232-264) against that @throws clause. The INTERSECTS, WITHIN and DISJOINT paths reach the checks in the query constructors and LatLonGeometry.create; the CONTAINS shortcut returns before them.
• The finder then wrote a one-test public-API JUnit that fails on main with assertions on and off and on 10.6.0 and 9.12.3, and a probe for the null element in both factories.
• An independent reproducer, an executed cross-model review, a devil's-advocate verifier and an adjudication reproduced it ("Prior reports, history and checks").