[HDPark95]

What

FTSearchParams.params(Map<String, Object> paramValues) is meant to merge the
given values into any parameters already set. The merge branch copies the field
onto itself instead of the argument:

public FTSearchParams params(Map<String, Object> paramValues) {
  if (this.params == null) {
    this.params = new HashMap<>(paramValues);
  } else {
    this.params.putAll(params);   // uses the field, not the argument
  }
  return this;
}

params here resolves to the field, so this.params.putAll(params) is
this.params.putAll(this.params) — a no-op, and the supplied paramValues are
dropped.

Impact

Whenever parameters are already present — e.g. addParam(...) followed by
params(map), or two params(map) calls — the second map is silently
discarded. Those values never reach the wire (the PARAMS clause omits them),
so an FT.SEARCH query that references the corresponding $name placeholders
does not get them substituted. When the parameter map starts empty the first
branch is taken and the value is preserved, which is why this went unnoticed.

Fix

Use the paramValues argument in the merge branch.

Test

Added FTSearchParamsTest.paramsMapMergesInsteadOfDroppingValues, a server-free
unit test that sets one parameter via addParam, then merges a second via
params(Map), and asserts both are emitted in the built command arguments. It
fails on the current code and passes with the fix.

---

Note

Low Risk
Small, localized fix to RediSearch query parameter assembly with dedicated unit and integration tests; no auth or data-path changes beyond correct PARAMS emission.

Overview
Fixes FTSearchParams.params(Map) so a second map actually merges when parameters already exist (e.g. after addParam or an earlier params call). The merge branch incorrectly called putAll on the internal map with itself, so extra paramValues were dropped and never appeared in the PARAMS clause for FT.SEARCH.

Documents merge/overwrite behavior on addParam and params. Adds unit tests in FTSearchParamsTest and an integration test testQueryParamsMixedAddParamAndMap for mixed addParam + params usage.

^{Reviewed by Cursor Bugbot for commit 7500187. Bugbot is set up for automated code reviews on this repo. Configure here.}

[kdelay]

The one-line fix looks correct: in the merge branch params resolves to the field, so this.params.putAll(params) was a self-copy and the argument was discarded. I grepped the rest of src/main/java for the same shape and this is the only occurrence — the other putAll hits are the JedisByteMap/JedisByteHashMap overrides and the executor map aggregators, so it is a one-off rather than a pattern.

Two notes on the test. I ran mvn -B -Dwith-param-names=true -Dtest=FTSearchParamsTest -DfailIfNoTests=false test on a clean checkout of master with this PR applied (Corretto 17): 1 test, 0 failures. Reverting only the production line reproduces the failure, with the emitted args being [FT.SEARCH, PARAMS, 2, existing, 1].

1. Notice that the expected 2 in that assertion is satisfied by the PARAMS arity token, not by the value of added — containsArguments is presence-only and position-agnostic, so the only thing actually catching the regression is the added key. That also means the arity, which is the part the server parses, is never asserted. To check how much that costs, I mutated line 146 to args.add(PARAMS).add(params.size()) (arity 2 instead of 4 for two parameters, which the server would reject) and re-ran the same command: still 1 test, 0 failures. An assertThat(args, hasArgument(2, RawableFactory.from(4))) alongside the existing assertion would pin the pair count and make the test fail on that class of change too.

2. The description names two ways into the merge branch — addParam then params(map), and two params(map) calls — but only the first is covered. The second one is a couple of extra lines in the same test and is the path a caller building a query from several maps would hit.

Unrelated to this PR, but adjacent enough to be worth a separate issue: AggregationBuilder.params(Map) appends the PARAMS clause into aggrArgs eagerly instead of accumulating into a map, so a second call emits a second clause rather than merging. Built with the same style of unit check, new AggregationBuilder("*").params({one=1}).params({two=2}) produces FT.AGGREGATE * PARAMS 2 one 1 PARAMS 2 two 2. Different failure mode from this one (duplicate clause instead of dropped values), same API shape.

One practical note if you touch the test: formatter-maven-plugin:validate runs before test, so an unformatted new file fails the build before any test executes.

[HDPark95]

Thanks, both are good catches. Done in ac375f7.

1. Pinned the arity. Added assertThat(args, hasArgument(2, RawableFactory.from(4))) next to the presence check. I reproduced the mutation to confirm it bites: changing line 146 to args.add(PARAMS).add(params.size()) now fails only that assertion (argument at index 2 was 2 instead of 4) while containsArguments still passes, so the pair count is pinned independently of the names.

2. Added the second entry into the merge branch as paramsMapMergesAcrossTwoMapCalls (params(first).params(second)), alongside the existing addParam-then-params(map) case.

Verification (Docker, temurin-17, -Dtest=FTSearchParamsTest): 2 tests, 0 failures. Reverting the production line back to putAll(params) fails both tests with the emitted args [FT.SEARCH, PARAMS, 2, existing, 1] and [FT.SEARCH, PARAMS, 2, first, 1]. formatter:validate on the new file is clean.

On AggregationBuilder.params(Map): agreed, that is a separate defect (duplicate PARAMS clause rather than dropped values) and worth its own issue, so I have kept this PR scoped to FTSearchParams.

[ggivo]

@HDPark95 Good catch — the merge branch has been a self-putAll no-op since the method was introduced. LGTM.

To get this release-ready in one pass, I pushed a few small commits to your branch:

• stripped the explanatory comments from the tests;
• added the missing javadoc for addParam/params(Map)
• added an integration test covering the mixed addParam + params(Map) path — the merge branch this PR fixes previously had no live-server coverage.

One ask for future contributions: when fixing a bug like this, please start with an actual usage pattern that demonstrates the issue on a real server — ideally, a small integration test (they are fast with our dockerized setup; see docs/integration-testing.md). Wire-level unit tests like yours are a valuable complement, but the integration test is what proves the user-visible behavior end to end.

Thanks again!
Closes: #4679.

[HDPark95]

Thanks for the cleanups and the javadoc. The restructured test reads better, and the integration test on the mixed addParam + params(Map) path covers exactly the wire behavior that had no live-server coverage before.

Point taken on leading with an integration test. I went through docs/integration-testing.md (the make start + mvn verify topology) and I'll start from a real-server reproduction for changes like this going forward. Thanks for the review.