feat: esOptions, track_total_hits, external versioning and $noFetch for documents ES v2

[Jitender-Rathore]

Groundwork in feathers-esx for the documents (invoices) Elasticsearch v2 index. Nothing here changes behaviour for an existing service unless that service opts in.

esOptions vs esParams

esParams is spread verbatim into every Elasticsearch request. The legacy client forwards any key it doesn't recognise as a query-string param, and ES 8 then rejects the request with unrecognized parameter. So the new behaviour flags can't live there.

options.elasticsearch is now split at construction: trackTotalHits and versionField are pulled out into a new esOptions, and everything else continues into esParams unchanged. Existing services see no difference — they pass no such keys.

track_total_hits (lib/core/find.js)

ES stops counting matches at 10,000 by default and reports { value: 10000, relation: 'gte' }, so total is simply wrong on any result set above that. That silently breaks paginated consumers — for us, CSV export pages to Math.ceil(total / 1000), so it would stop at 10k rows.

Opt in per service with elasticsearch: { trackTotalHits: true }. Set in the request body, not as a top-level param, because the legacy client validates top-level params against its own API spec. Untouched when the option is absent.

External versioning on bulk (lib/core/create-bulk/6.0.js)

With elasticsearch: { versionField: 'updatedAt' }, each bulk item is stamped with version and version_type: external_gte. ES then refuses any write carrying a version older than what's stored.

This is what makes a backfill idempotent and order-independent: a row derived from an older snapshot can't clobber a newer live write, and the loader can be re-run freely. Rejected items return 409 version_conflict_engine_exception, which the caller should count and ignore.

Only applied to index — create fails outright if the doc exists, so versioning is meaningless there. Non-parsable or missing version values fall back to an unversioned write rather than failing.

$noFetch (lib/core/create-bulk/core.js)

createBulk always follows the bulk write with an mget to return the created documents. Callers that discard the return value — a background sync writing to a secondary store — pay two round trips for data nobody reads. params.$noFetch returns straight after the bulk.

Also in this branch: parse-query null handling

lib/utils/parse-query.js + tests carry a separate change: { field: null } now becomes must_not: { exists: { field } }. That is the only change to null handling.

An earlier revision of this branch also stripped nulls from arrays and from criteria values. That was reverted: a base-vs-head differential over 207 generated query shapes showed it changed the emitted DSL for 118 of them — dropping $in/$nin elements and whole clauses that master emits today, on a file shared by every ES index. { field: [null] } and { field: { $in: [null] } } therefore still emit term/terms with a null, exactly as they do on master. That is pre-existing behaviour this branch does not change, and it is tracked separately rather than fixed here.

Flagging it explicitly because, unlike everything above, this one is not opt-in — parse-query is shared by every ES-backed service, so it also affects the leads, serials and businesses indices. It is not required by the v2 work: reverting it and re-running the documents differential harness gave 28/28 identical results against Mongo. Happy to split it into its own PR if reviewers would rather it landed separately.

Testing

• test/utils/parse-query.js extended for the null cases.
• Exercised end-to-end against a local corpus of 2,800 documents via the serana esv2diff harness (Mongo vs ES through the same Feathers service): 32/32 on two businesses, plus esv2reconcile 4/4.

🤖 Generated with Claude Code

[Purvi-Gupta]

Reviewed as part of the REF-25128 set (fence#746, birds#408, serana#4818, this).

The esOptions split is the right fix at the right layer, and I verified backward compatibility by hand — the only other elasticsearch: {...} option bags in the tree (astrid business-items, serana MongoElastic) pass neither trackTotalHits nor versionField, and nothing outside birds reads service.esParams. No existing consumer regresses from that change.

What blocks merge is the parse-query null handling. It silently widens queries for every consumer of this library, which is the same silent-wrong-answer class the v2 project exists to eliminate — reintroduced one layer below where the safety valve can catch it.

I ran parseQuery on both commits rather than reasoning about it:

query	base 3e137dc	head ea48a2d
{status:{$in:[]}}	terms:{status:[]} → matches nothing	null → matches everything
{business:'x',status:{$in:[]}}	both clauses	business filter only
{status:{$ne:null}}	must_not term	null → clause dropped
{status:{$in:['A',null]}}	terms:['A',null]	terms:['A']
{status:{$nin:['A',null]}}	must_not terms:['A',null]	must_not terms:['A']
{status:null}	threw BadRequest	must_not exists ✅

That last row is genuinely new capability and a clean improvement — it used to throw, so no consumer can be depending on the old behaviour. The rest are regressions in the unsafe direction.

Separately, two of the five changes have no live caller anywhere in the tree, so the PR delivers less than the ticket claims. Details inline.

[Purvi-Gupta]

This fires for arrays that were already empty, not just ones emptied by null-filtering.

{status:{$in:[]}}            => null
{business:'x',status:{$in:[]}} => {"filter":[{"term":{"business":"x"}}]}

Before this PR, $in: [] produced terms: {status: []}, which Elasticsearch correctly evaluates as matching nothing. Now the criterion is dropped entirely, so the query matches everything.

An empty $in is not exotic — it's what you get from "filter by this list of ids" whenever the list comes back empty. In the second case above you can see the scoping clause survives while the intended restriction silently vanishes, which is the worst possible shape for this bug.

Fix: never drop a criterion. If the array is empty, emit a clause that matches nothing — push the original empty terms for $in, or filter: [{ bool: { must_not: { match_all: {} } } }]. Only $nin: [] is safely a no-op.

[Jitender-Rathore]

Fixed, though by reverting rather than by adding a match-nothing clause.

Your five examples made me check whether there were more, so I extracted parse-query.js at the base commit 3e137dc and at this head and ran both over 207 query shapes: every operator in queryCriteriaMap crossed with null, [], [null], ['A',null] and scalars, plus $or/$and/$nested wrappers and scoped two-clause queries.

207 cases, 119 divergent — 118 regressions and 1 improvement. It was not confined to $in/$nin/$ne; all 13 operators were affected:

{k:{$gte:null,$lte:5}}
   base: filter[ range gte:null, range lte:5 ]
   head: filter[ range lte:5 ]          <- lower bound silently gone

$nested{k:{$in:[]}}
   base: must[ nested p { terms k:[] } ]
   head: null                            <- entire nested query gone

The differential also showed which null actually needed handling. Bare {field: null} threw BadRequest on base, which is the gap the change was really trying to close. Everything else went in alongside it.

So I kept the bare-null handling and removed the criteria-level and array-level null filtering entirely. Re-running the same 207 shapes now gives 206 identical, 1 divergent, and that one is {k:null} going from BadRequest to must_not exists.

parse-query.js now differs from master by that one block and nothing else.

[Purvi-Gupta]

$ne: null now contributes no clause at all.

{status:{$ne:null}} => null

{field: {$ne: null}} means "field exists and is non-null" and is one of the most common Mongo predicates there is. Dropping it widens the result set to include exactly the documents the caller asked to exclude. The same applies to $gt/$gte/$lt/$lte/$prefix/$regexp: null.

Fix: $ne: null should become must: [{ exists: { field: key } }] — the mirror of the new bare-null handling just above. For range and prefix operators null is genuinely meaningless, so BadRequest is the right answer there rather than a silent drop.

[Jitender-Rathore]

Fixed by the same revert as the $in: [] thread. {k:{$ne:null}} is back to must_not term, identical to master.

I did not add the $ne: null to exists rewrite, or the BadRequest for range and prefix operators. Both are right, but they belong with the follow-up the plan already carved out for this file ("teach parse-query $eq / per-field $exists / $ne:null"), with their own tests, rather than inside a PR whose purpose is the documents index.

[Purvi-Gupta]

Filtering nulls out of arrays changes what the query means, in both directions.

{status:{$in:['A',null]}}  => {"filter":[{"terms":{"status":["A"]}}]}
{status:{$nin:['A',null]}} => {"must_not":[{"terms":{"status":["A"]}}]}

Mongo $in: ['A', null] matches A or null/missing — this now drops every null/missing row (missing rows). Mongo $nin: ['A', null] excludes A and null/missing — this now returns them (extra rows).

Fix, rewriting rather than filtering:

• $in with a null present → should: [{terms:{k:nonNulls}}, {bool:{must_not:{exists:{field:k}}}}] with minimum_should_match: 1
• $nin with a null present → add {exists:{field:k}} to filter alongside the must_not.terms

If that's out of scope here, throw BadRequest instead. Erroring loudly is strictly safer than returning wrong rows, and it's what this code did before the change.

[Jitender-Rathore]

Fixed by the same revert. $in: ['A', null] and $nin: ['A', null] pass through unchanged again, exactly as on master.

Agreed on the ordering you set out: for a library shared by four indices, erroring loudly beats returning wrong rows. Reverting gets that for free here, since base already errored on these shapes. The should / exists rewrite you sketched is the right shape for when this is done properly.

[Purvi-Gupta]

Date.parse on a Date object truncates milliseconds, which defeats the ordering guarantee for exactly the case it's meant to protect.

updatedAt on a Mongoose document is a Date, not a string. Date.parse(dateObj) coerces via Date.prototype.toString(), whose output carries no milliseconds — so the version rounds to whole seconds. Two writes in the same second get identical versions, and external_gte accepts equal, so the older payload can still overwrite the newer one.

Fix: const version = typeof v === 'number' ? v : new Date(v).getTime(); — handles Date, ISO string and numeric timestamp uniformly, preserves ms, and still yields NaN for garbage so the Number.isFinite guard below still holds.

Relatedly, the two silent return {} bail-outs (missing field, unparseable value) are indistinguishable from "versioning not configured", yet they mean the opposite: the service declared versionField and this document is unprotected. Those are precisely the documents a stale backfill row can clobber. Worth a warning or a counter rather than silence.

[Jitender-Rathore]

Fixed:

const version = typeof doc[field] === 'number' ? doc[field] : new Date(doc[field]).getTime();

One correction on the impact, though. It does not currently bite the documents path. buildEsDocumentV2 runs every date field through toDate(), which returns value.toISOString(), and both write paths transform before writing (FlexStoreService.ts:487 for syncUpdate, :529 for syncUpdateBulk). So updatedAt arrives as an ISO string with milliseconds intact. The mechanism is real and the fix is right, because a library cannot assume its callers pre-stringify, but it is hardening rather than an active ordering bug.

There is a regression test for it in the new test/service-options.js. I checked it is meaningful by restoring the old line: it fails with expected 1787565600000 to equal 1787565600123.

On the two silent return {} bail-outs: agreed, and not done. A debug line is easy, but a counter a caller can actually read needs a return-shape change I would rather not make in this PR. Carried to the follow-up.

[Purvi-Gupta]

getVersioning has no live caller — as shipped this feature is unreachable.

Versioning applies only when method === 'index', i.e. only when params.upsert is set. Tracing every write path in the tree:

• birds FlexStoreService._create → ElasticService._create → super._create with no upsert → method === 'create' → versioning = {}
• the only upsert: true in birds is ElasticService.ts:78, the _update fallback — and _update now calls this.Model.index() directly, bypassing this library entirely and using its own copy of the version logic
• the backfill (seeds/essync.mjs) posts raw _bulk over HTTP and never touches feathers-esx

So the ticket's claim that "bulk loads become idempotent" is not delivered by this code path. Either wire a caller (birds' bulk sync should pass upsert), or say plainly in the PR description that this is groundwork.

While you're here: birds duplicates this exact typeof x === 'number' ? x : Date.parse(x) + Number.isFinite logic at ElasticService.ts:110-113, with a deliberate wire-key difference (version_type snake_case here for the bulk NDJSON action line, versionType camelCase there for the legacy client's index() API). They agree today, but two copies of a correctness-critical rule will drift and the drift will be invisible. Export it from here and have birds import it.

[Jitender-Rathore]

This one I disagree with. getVersioning has a live caller, in birds#408, which is in the same review set.

birds/src/services/FlexStore/FlexStoreService.ts:547, inside syncUpdateBulk:

._create(chunk, { upsert: true, $noFetch: true })

An array reaches _create, which routes to createBulk (lib/index.js:104-113); params.upsert is truthy so method === 'index', and getVersioning runs. That is on the pushed head origin/REF-25128 at d1e08046, not local-only.

Your point about seeds/essync.mjs is correct: it posts raw _bulk over HTTP and never touches this library. It carries its own copy of the versioning at essync.mjs:206-215.

The duplication point stands, and is now worse than when you wrote it. I fixed the Date.parse truncation here, but birds/src/services/FlexStore/ElasticService.ts:117 still has Date.parse(rawVersion). The two copies now disagree, which is exactly the drift you predicted. I will fix that on birds#408. Exporting a shared helper is the better answer and I would rather do it once both PRs settle, so the helper is written against final behaviour.

[Purvi-Gupta]

$noFetch has zero consumers — I grepped birds, serana and seeds and there is no occurrence of noFetch anywhere.

On the leak question, it's safe: params is never spread into a request (getBulkCreateParams spreads only service.esParams), and it sits on root params rather than params.query, so filterQuery's whitelist never sees it. No repeat of the versionField problem.

But this is also the only path in the library that returns a differently-shaped array — mapBulk metadata records rather than fetched source documents — and that contract change is documented in an inline comment only, not in README.md or types/index.d.ts. Either land the caller with it or drop it from this PR.

[Jitender-Rathore]

Same as the versioning thread: there is a consumer, at birds/src/services/FlexStore/FlexStoreService.ts:547, on the pushed head of birds#408.

._create(chunk, { upsert: true, $noFetch: true })

The documentation point is correct and I have fixed it. types/index.d.ts now carries an ElasticsearchCreateParams interface documenting both upsert and $noFetch, including the return-shape change you flagged (mapBulk metadata records rather than fetched source documents), and _create takes it.

It also has executed coverage now, in test/service-options.js: the test asserts mget is never called and that the returned element carries _meta.

[Purvi-Gupta]

The split itself is correct — I verified esParams really is spread verbatim into every request (find.js:14, get-bulk.js:9, create/6.0.js:17, update/6.0.js:12, create-bulk/6.0.js:57, patch-bulk.js:35), so the constructor is the right layer to intercept.

Two requests:

1. Make the stale-install failure loud. The canary trap documented on the ticket (npm resolving plain 1.0.2 as a nested copy under birds, producing unrecognized parameter: [versionField] on every document search) took out staging and qa01. The durable fix belongs in birds, but this constructor is where it can be made self-describing: if esRequestParams contains a key outside a known-safe allowlist, throw here with the key name. A wrong config then fails at boot with a clear message instead of on every user query, and it future-proofs the next behaviour option someone adds. A peerDependency won't help — the failure was a nested install, which peer resolution permits.

2. Add a regression test asserting versionField and trackTotalHits do not appear in service.esParams. That single assertion is the test for the outage that actually happened.

Note npm test on this head: 89 unit tests pass, and the entire Elasticsearch Service integration suite fails in its before all hook (mapper_parsing_exception — the ES5-era fixtures don't load on an ES8 server). That suite is the only thing that would exercise create-bulk, find, $noFetch and versioning, so four of the five changes in this PR have no executed coverage at all.

[Jitender-Rathore]

Regression test added, in the new test/service-options.js:

expect(service.esParams).to.not.have.property('trackTotalHits');
expect(service.esParams).to.not.have.property('versionField');

alongside one asserting both land on esOptions, and one asserting every other elasticsearch key still flows through to esParams unchanged.

I would rather not add the throw. In the failure it is meant to catch, the stale nested copy is the code that runs: npm resolved plain 1.0.2 under birds, so 1.0.2 is what was loaded, and a guard added in a later version never executes. It would also need an allowlist of every legitimate ES request param, which moves between client versions, so a false positive fails a service at boot over a param that was fine. The durable fix is the pin inside birds, as you said.

On coverage: the four changes are no longer untested. test/service-options.js runs without an Elasticsearch server, since the constructor issues no requests and the bulk tests drive a stub client, and gives 11 executed tests across the split, the versioning and $noFetch. The ES5-era fixtures failing against ES8 in test/index.js is a real problem and worth its own ticket, but it no longer leaves this PR uncovered.

[Purvi-Gupta]

This version goes backwards relative to master. origin/master is already at 1.0.2 (commit 34705f5), and in semver 1.0.2-documents-esx.0 < 1.0.2, so merging as-is regresses the published version and the CHANGELOG loses master's 1.0.2 entry.

Good news on the thing the ticket worried about: I test-merged origin/master into this head and lib/core/find.js auto-merges cleanly, keeping master's paginate: false fix. There's no risk of reverting it through the code path — the conflicts are confined to CHANGELOG.md, package.json and package-lock.json.

Suggested: merge master in first, release as 1.0.3 (or 1.1.0, since this adds features), and drop the -documents-esx.0 canary entry from the CHANGELOG — it's a throwaway build and doesn't belong in mainline history.

[Jitender-Rathore]

Agreed. The merge check is useful too, I had assumed find.js would conflict.

Plan: merge origin/master in first, release as 1.1.0 since this adds features, and drop the 1.0.2-documents-esx.0 entry from the CHANGELOG. That canary existed only to unblock staging and qa01 and does not belong in mainline history.

[naw7az]

Review of the esOptions/esParams split, track_total_hits, external versioning, $noFetch and the null parse-query change.

The crux of the PR is solid: the destructure in lib/index.js correctly pulls trackTotalHits/versionField out of esParams into esOptions, track_total_hits lands in the search body (not a top-level param), external versioning is index-only and uses new Date().getTime() (not Date.parse) to keep milliseconds, and $noFetch short-circuits the follow-up mget. Tests cover all of these well. No behaviour option leaks into the params sent to ES.

Two issues below. The first is the important one: this branch is 5 commits behind master and does not carry the paginate:false size fallback, which is a silent truncation bug for exactly the CSV-export path this v2 work depends on.

Low (not blocking): the PR description claims parse-query "strips nulls from arrays and from criteria values (skipping the clause entirely if an array empties out)", but the diff only adds the top-level value === null case — the array branch (value.forEach(...term...)) and the criteria branch are unchanged. See the inline note.

[naw7az]

🔴 High — paginate:false truncates to 10 rows (missing size fallback)

Line 9 sets size: filters.$limit. When paginate is false and no $limit is supplied, filters.$limit is undefined, so ES falls back to its default size of 10 — a find({ paginate: false }) returns only 10 documents. master fixed this (REF-19943) with size: paginate === false ? filters.$limit || 10000 : filters.$limit, but this branch forked before that commit and drops it. This is the CSV-export path the v2 dashboard relies on, so it silently truncates. Restore the fallback on line 9:

    size: paginate === false ? filters.$limit || 10000 : filters.$limit, // Default max size to 10k if paginate is false

[Jitender-Rathore]

Fixed. Restored the fallback on line 9:

size: paginate === false ? filters.$limit || 10000 : filters.$limit,

Confirmed the mechanism before changing it — git merge-base --is-ancestor origin/master HEAD reports master is not merged into this branch, and git log -1 origin/master -- lib/core/find.js is 653ff91 refactor: added max size incase of paginate:false. So this branch forked before that commit and silently dropped it, exactly as you describe.

[naw7az]

🟡 Medium — null only handled at the top level; arrays/criteria still emit term: null

This handles { field: null }, but a null inside an array ({ field: [null] }) or a criterion value ({ field: { $in: [null] } }) still reaches the unchanged branches below and produces { term: { field: null } } / terms with a null element. A term query with a null value throws No value specified for term query in ES, failing the whole search (and this file is shared by every ES index, not opt-in). The PR description states these nulls are stripped, but the diff doesn't do it — either strip nulls in the array/criteria branches too, or drop the description's claim.

[Jitender-Rathore]

Half of this is right and I've fixed that half; pushing back on the other half.

Right: the PR description was wrong. It claimed nulls are stripped from arrays and criteria; the diff doesn't do that. Description corrected.

Your DSL reading is also exactly right — verified by running the function:

{ field: [null] }        => {"filter":[{"term":{"field":null}}]}
{ field: {$in:[null]} }  => {"filter":[{"terms":{"field":[null]}}]}

But this is not introduced here. Same two shapes against origin/master:

--- MASTER (what runs in prod today) ---
{ field: [null] }        => {"filter":[{"term":{"field":null}}]}
{ field: {$in:[null]} }  => {"filter":[{"terms":{"field":[null]}}]}
{ field: null }          THREW: field should be one of number, string, boolean, undefined, object, array

Byte-identical for the array/criteria cases. The only behavioural difference this branch makes is that bare { field: null } stops throwing and becomes must_not exists. Net: 1 shape fixed, 0 regressed.

On stripping them anyway: an earlier revision of this branch did exactly that, and it was reverted after measuring it. I built a base-vs-head differential over 207 generated query shapes (parseQuery is pure, so no ES needed) and the stripping version changed the emitted DSL for 118 of them — silently dropping $in/$nin elements and, where an array emptied, whole clauses that master emits today. On a file shared by every ES index, that trades a narrow pre-existing fault for a broad new one. The origin was cb44a5b ("Skip null values in criteria to avoid Elasticsearch errors"), which is where the 118 came from.

So: description fixed, behaviour deliberately left at master parity. Happy to take the array/criteria fix as its own PR with the 207-shape differential as its gate — that's where it can be shown safe. Want me to raise it?