[SOLR-10059] Handle appends params in distributed requests

[tboeghk]

Description

In distributed requests, the appends section of a RequestHandler is evaluated not only on the non-distributed coordinator requests but also during the distributed requests. This might lead to parameters (especially harmful for fq params) being attached multiple times.

<requestHandler name="/myHandler" class="solr.SearchHandler">
    <lst name="appends">
        <str name="fq">{!collapse field=routingKey hint=top_fc}</str>
    </lst>
</requestHandler>

The issue exists for some time in the bug tracker: SOLR-10059

Solution

The fix ignores the appends section of the handler definition during distributed requests.

Tests

I added a simple unit test, no cloud setup test though

Checklist

Please review the following and check all that apply:

• [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• [x] I have created a Jira issue and added the issue ID to my pull request title.
• [ ] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
• [x] I have developed this patch against the main branch.
• [x] I have run ./gradlew check.
• [x] I have added tests for my changes.
• [ ] I have added documentation for the Reference Guide

[epugh]

Did you see the precommit failure on missing license header @tboeghk ?

[tboeghk]

Whoopsie, my bad, ran the gradlew check with -Pvalidation.git.failOnModified=false. Added license header in the last commit

[tboeghk]

I came across an edge case that I want to consider: In the distributed environment we should only ignore the appends section if we do not change the handler during the distributed requests. I'll add that as well and write some tests.

[tboeghk]

Hey @epugh any chance to merge this? Or has anybody else directions for improvement?

[epugh]

One more request @tboeghk, it apears from JIRA that your patch would fix https://issues.apache.org/jira/browse/SOLR-14931 ???

[tboeghk]

Yes (I'm 95% sure): The macros don't get expanded on the shard. With the fix above the configuration is not picked up on the shard and the macros should stay expanded. Do we need another test for that?

[epugh]

Does anyone ever say "no, we don't need another test" ;-) I'd love one, partly because this whole space is kind of new for me, so it might help me feel confident of your fix!!!! unit tests are super helpful in understanding the logic of the fix!

[epugh]

@cpoerschke I wanted to get this sorted, any chance you could be another set of eyes????

[cpoerschke]

@cpoerschke I wanted to get this sorted, any chance you could be another set of eyes????

From looking at the changes I'm wondering if RequestUtil might be an alternative place for the logic since the "don't expand macros on the shard" logic already happens there.

From reading about the context of the changes on https://issues.apache.org/jira/browse/SOLR-10059 on 2017-03-01 there's a mention of multi-collection requests, I don't yet fully understand how that fits into the bigger picture.

From running the new tests locally, hmm, they don't pass here at the moment.

[cpoerschke]

... no cloud setup test though ...

#426 is about adding a SearchHandlerAppendsCloudTest test.

[epugh]

@cpoerschke @tboeghk want to pick this one up again....? (Though I know @tboeghk is out on vacation till September!)....

[github-actions[bot]]

This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the [email protected] mailing list. Thank you for your contribution!

[janhoy]

This looks like a good improvement, simple contained fix.

[tboeghk]

I reviewed the fix recently and came up with a smaller fix that isolates the issue better: https://github.com/otto-de/solr/commit/a82b500d3c633621b0062698f540c2974a920fbb

The fix contains a test that breaks without the fix applied (macros in fq parameter)

[janhoy]

I reviewed the fix recently and came up with a smaller fix that isolates the issue better: otto-de@a82b500

The fix contains a test that breaks without the fix applied (macros in fq parameter)

Ok, translate for me what that means. Can this PR or the JIRA be closed? Will there be a new PR?

[tboeghk]

I'll file a new PR 👍

[tboeghk]

I created a new PR https://github.com/apache/solr/pull/2303 that superseeds this PR.