Skip to content

SOLR-12505: fetch() returns the fetched fields when the target handler's default query parser is not lucene - #5169

Open
nick-boss-tech wants to merge 12 commits into
apache:mainfrom
nick-boss-tech:solr-12505-submit
Open

nick-boss-tech wants to merge 12 commits into
apache:mainfrom
nick-boss-tech:solr-12505-submit

Conversation

@nick-boss-tech

@nick-boss-tech nick-boss-tech commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖 (posted on behalf of Nick Shanin)

https://issues.apache.org/jira/browse/SOLR-12505

What happens today

fetch() returns the tuples without the fetched fields when the target handler's default query parser is not lucene.

fetch() sends its batch as {! df=<join field> q.op=OR cache=false } followed by the escaped key values. It sends no defType, so the handler's default parser reads the batch. Solr reads the {! prefix only when that default is lucene or func (QParser.java lines 387 to 396, merge-base). With another default, the batch is read as plain query text and the fetched fields are missing. The ticket's assignee, Eric Pugh, found that removing defType=edismax from the /select handler fixed it.

The batch is built at FetchStream.java lines 239 to 246 (merge-base; current main has the same lines).

What this change does

fetch() sends defType=lucene with every batch, so the prefix is read whatever the handler's default is.

FetchStream.java lines 273 to 276 add the parameter. Behavior change: a defType default on the handler no longer applies to fetch batches. A handler with the lucene default, which is Solr's default and the test setup, sends the same batch as before.

fetch() also rejects a join field name that would break its batch query.

A join field name that would break the batch query now fails when the expression is built, and the error names the field (FetchStream.java lines 153 to 187). The batch sends the name as an unquoted value, which the local-params reader ends at whitespace or }, so both are rejected anywhere in the name. A leading quote would switch the reader to a quoted string, and a leading $ would be read as a reference to a request parameter, so both are rejected at the start of the name. The whitespace in that check is Character.isWhitespace, the same definition the local-params reader splits on, so Unicode spaces are rejected as well as ASCII ones. Before, a name with whitespace or } ended the df= value early in the batch query and corrupted the fetched-field list, and a name starting with $ sent the batch looking up the wrong field. The name is rejected rather than escaped: escaping would let the batch accept a name that is not in the schema, while the check fails with a message that says what is wrong.

Proof

Each new test fails without its fix and passes with it.

testFetchStreamWithNonLuceneDefaultDefType sets the test collection's /select default to edismax. On the merge-base FetchStream, with the test as it stands at 5f20e11, it fails with ComparisonFailure: expected:<alpha> but was:<null>. The test collection is created from a dedicated copy of the configset: the Config API overlay is stored with the configset, so setting the /select default on the shared configset would outlive the test and change /select for the other tests in the class.

With this change, StreamDecoratorTest runs 60 tests, 0 skipped, 0 failures at head cc3b9d681e9d3caa63bb405baf5b19c66e64cd04, verified 2026-10-16. That includes the two existing fetch tests, which use the default parser and do not change.

testFetchStreamRejectsJoinFieldNameThatBreaksLocalParams sends the join field name a i. On the parent commit 5f20e11, which has no join check, the drain fails with the server's parser error, invalid query parser 'i', which does not name the join field. With this change it fails at construction with the message on the cause chain (see Limits).

testFetchStreamRejectsJoinFieldNameWithUnicodeSpace sends the join field name a\u2003i, with an em space (U+2003) instead of an ASCII space; the local-params reader splits on that character too. On the merge-base code, which has no join check, nothing rejects the name and no error is thrown. The ASCII-only pattern the check used before did not catch it either. With this change it fails at construction with the same message.

testFetchStreamRejectsJoinFieldNameStartingWithDollarSign sends the join field name $a_i. With this change the expression fails as it is built. On the head before the leading-$ rejection, nothing rejects the name: the batch is sent, $a_i is read as a parameter reference, and the server answers that parameter a_i is missing.

testFetchStreamRejectsJoinFieldNameWithClosingBraceOrLeadingQuote covers the remaining rejected shapes: a}i, 'a_i and "a_i. Each fails at construction with the same message.

A choice to check

The batch sets defType=lucene. The ticket thread also named the terms query parser.

Joel Bernstein proposed setting the parameter, and David Smiley agreed (his comment). Smiley also suggested the terms parser (plugin name, merge-base, comma separator, merge-base). That alternative sets defType=terms with the keys as the query. It changes the query text of every batch, and the keys would need the terms separator instead of the escaping the batch uses now.

Was setting defType on the batch the right call, or should the batch use the terms parser?

One difference matters at scale: the batch ORs one clause per key value, so a batch holding more key values than the handler's maxBooleanClauses (1024 by default) would fail under lucene. The terms parser has no such limit.

Limits

The batch no longer depends on a handler's default defType. The test uses edismax only. Not covered: the realtime get endpoint, or a handler that pins defType under invariants.

  • Only the batch request and the join field check change. The search side of the expression is untouched.
  • StreamFactory reports only a generic message for a constructor failure, so the check's message sits on the cause chain. Fixing that is outside this change; a follow-up ticket and PR can be opened on request.
  • The rejection tests cover every rejected shape: an ASCII space, an em space, a name containing }, and names starting with a single quote, a double quote, or $. Names containing {, ! or a backslash are not rejected, because the reader treats them as ordinary characters.
  • A handler that pins defType under invariants still overrides the batch's defType=lucene, because request parameters do not override invariants. The new test covers a handler defaults entry; the invariants configuration is not covered.
  • The realtime get endpoint, which the ticket raised, works only when the join field is the uniqueKey. It is not used here.
  • Base: this change is written against merge-base 14c7aac, which is 77 commits behind upstream main at d38e0f1, as counted on 2026-10-13. It has not been rebased.

Changelog: changelog/unreleased/SOLR-12505-fetch-stream-deftype.yml

AI assistance

AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.

nick-boss-tech and others added 10 commits October 3, 2026 23:56
The batch query uses local params that only the lucene parser honors. Send defType=lucene so a handler default such as edismax does not defeat fetch(). Adds an unrun, hypothetical regression test.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The subjects indexed by testFetchStreamWithNonLuceneDefaultDefType ended
in the join keys (0, 1, 2) and are copyFielded into text, the handler
default field, so the default edismax parser found the documents by
accident and the test passed without the fix. The subjects are now
alpha, bravo and charlie, which share no token with the keys; on base
code the test fails with the fetched subject null, the ticket symptom.
Also drops the stale javadoc note claiming the test was never run.
The new test built its StreamContext with a fresh SolrClientCache and
never closed it, leaking the cloud client at class teardown. The cache
is now closed in the finally block.
…nd keep assertThrows lambdas to one statement
…espace and name the check in the changelog title
… break the local-params value, and reject a leading '$'
@nick-boss-tech
nick-boss-tech marked this pull request as ready for review October 16, 2026 16:00
…solate the edismax test's Config API overlay in a dedicated configset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant