-
Notifications
You must be signed in to change notification settings - Fork 1
feat: esOptions, track_total_hits, external versioning and $noFetch for documents ES v2 #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,14 @@ function createBulkCore(service, data, params, { getBulkCreateParams }) { | |
|
|
||
| return service.Model.bulk(bulkCreateParams).then((results) => { | ||
| const created = mapBulk(results.items, service.id, service.meta, service.join); | ||
|
|
||
| // Callers that discard the return value (e.g. a background sync writing to a secondary | ||
| // store) can skip the follow-up mget entirely. Without this a 500-document bulk costs | ||
| // two round trips instead of one, for data nobody reads. | ||
| if (params.$noFetch) { | ||
| return created; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On the leak question, it's safe: But this is also the only path in the library that returns a differently-shaped array —
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the versioning thread: there is a consumer, at ._create(chunk, { upsert: true, $noFetch: true })The documentation point is correct and I have fixed it. It also has executed coverage now, in |
||
| } | ||
|
|
||
| // We are fetching only items which have been correctly created. | ||
| const docs = created | ||
| .map((item, index) => ({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,18 @@ function find(service, params) { | |
| const findParams = { | ||
| _source: filters.$select, | ||
| from: filters.$skip, | ||
| size: filters.$limit, | ||
| // `paginate: false` with no `$limit` leaves size undefined, so ES applies its default of 10 | ||
| // and a full export silently returns 10 rows. | ||
| size: paginate === false ? filters.$limit || 10000 : filters.$limit, | ||
| sort: filters.$sort, | ||
| body: { | ||
| query: esQuery ? { bool: esQuery } : undefined, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 High — Line 9 sets size: paginate === false ? filters.$limit || 10000 : filters.$limit, // Default max size to 10k if paginate is false
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Restored the fallback on line 9: size: paginate === false ? filters.$limit || 10000 : filters.$limit,Confirmed the mechanism before changing it — |
||
| // Elasticsearch stops counting matches at 10,000 by default and reports | ||
| // { value: 10000, relation: 'gte' }, so any paginated result set larger than that | ||
| // reports a wrong `total`. Opt in per service via `elasticsearch.trackTotalHits`. | ||
| ...(service.esOptions && service.esOptions.trackTotalHits !== undefined | ||
| ? { track_total_hits: service.esOptions.trackTotalHits } | ||
| : {}), | ||
| }, | ||
| ...service.esParams, | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,12 +17,20 @@ class Service extends AdapterService { | |
| throw new Error('Elasticsearch `Model` (client) needs to be provided'); | ||
| } | ||
|
|
||
| // `esParams` is spread verbatim into EVERY Elasticsearch request (search, bulk, index, | ||
| // get, ...). The legacy client passes any key it does not recognise straight through as | ||
| // a query-string param, and Elasticsearch 8 then rejects the request with | ||
| // `unrecognized parameter`. So behaviour options must be separated out here and kept | ||
| // out of esParams - they are exposed as `esOptions` instead. | ||
| const { trackTotalHits, versionField, ...esRequestParams } = options.elasticsearch || {}; | ||
|
|
||
| super({ | ||
| id: '_id', | ||
| parent: '_parent', | ||
| routing: '_routing', | ||
| meta: '_meta', | ||
| esParams: { refresh: false, ...options.elasticsearch }, | ||
| esParams: { refresh: false, ...esRequestParams }, | ||
| esOptions: { trackTotalHits, versionField }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The split itself is correct — I verified Two requests:
Note
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regression test added, in the new expect(service.esParams).to.not.have.property('trackTotalHits');
expect(service.esParams).to.not.have.property('versionField');alongside one asserting both land on 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 On coverage: the four changes are no longer untested. |
||
| whitelist: [ | ||
| '$prefix', | ||
| '$wildcard', | ||
|
|
@@ -48,12 +56,13 @@ class Service extends AdapterService { | |
| }); | ||
|
|
||
| // Alias getters for options | ||
| ['Model', 'parent', 'routing', 'meta', 'join', 'esVersion', 'esParams'].forEach((name) => | ||
| Object.defineProperty(this, name, { | ||
| get() { | ||
| return this.options[name]; | ||
| }, | ||
| }), | ||
| ['Model', 'parent', 'routing', 'meta', 'join', 'esVersion', 'esParams', 'esOptions'].forEach( | ||
| (name) => | ||
| Object.defineProperty(this, name, { | ||
| get() { | ||
| return this.options[name]; | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| this.core = core(options.esVersion); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -218,7 +218,23 @@ function parseQuery(query, idProp) { | |
| return specialQueryHandlers[key](value, result, idProp); | ||
| } | ||
|
|
||
| validateType(value, key, ['number', 'string', 'boolean', 'undefined', 'object', 'array']); | ||
| validateType(value, key, [ | ||
| 'number', | ||
| 'string', | ||
| 'boolean', | ||
| 'undefined', | ||
| 'object', | ||
| 'array', | ||
| 'null', | ||
| ]); | ||
|
|
||
| // Handle null values - convert to "must_not exists" query to search for null fields | ||
| if (value === null) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — null only handled at the top level; arrays/criteria still emit This handles
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: But this is not introduced here. Same two shapes against Byte-identical for the array/criteria cases. The only behavioural difference this branch makes is that bare 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 ( 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? |
||
| result.must_not = result.must_not || []; | ||
| result.must_not.push({ exists: { field: key } }); | ||
| return result; | ||
| } | ||
|
|
||
| // The value is not an object, which means it's supposed to be a primitive or an array. | ||
| // We need add simple filter[{term: {}}] query. | ||
| if (type !== 'object') { | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "name": "@refrens/feathers-esx", | ||
| "version": "1.0.1", | ||
| "version": "1.0.2-documents-esx.0", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This version goes backwards relative to master. Good news on the thing the ticket worried about: I test-merged Suggested: merge master in first, release as
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. The merge check is useful too, I had assumed Plan: merge |
||
| "description": "Refrens fork of feathers-elasticsearch with fixes and extensions", | ||
| "main": "lib/", | ||
| "types": "types", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| /* eslint-disable no-underscore-dangle */ | ||
| const { expect } = require('chai'); | ||
| const { Service } = require('../lib'); | ||
|
|
||
| /** | ||
| * These run without an Elasticsearch server: the constructor makes no requests, and the bulk | ||
| * tests below drive a stub client. They cover the `esParams` / `esOptions` split and the | ||
| * external versioning stamped onto bulk actions. | ||
| */ | ||
| describe('Elasticsearch service options', () => { | ||
| const makeService = (elasticsearch) => | ||
| new Service({ Model: {}, esVersion: '6.0', elasticsearch }); | ||
|
|
||
| describe('esParams / esOptions split', () => { | ||
| // The regression test for the staging + qa01 outage: a behaviour option that leaks into | ||
| // esParams is spread into every request, and Elasticsearch 8 rejects the whole search with | ||
| // `unrecognized parameter: [versionField]`. | ||
| it('keeps behaviour options out of esParams', () => { | ||
| const service = makeService({ | ||
| index: 'refrens_documents', | ||
| trackTotalHits: true, | ||
| versionField: 'updatedAt', | ||
| }); | ||
|
|
||
| expect(service.esParams).to.not.have.property('trackTotalHits'); | ||
| expect(service.esParams).to.not.have.property('versionField'); | ||
| }); | ||
|
|
||
| it('exposes behaviour options on esOptions', () => { | ||
| const service = makeService({ trackTotalHits: true, versionField: 'updatedAt' }); | ||
|
|
||
| expect(service.esOptions.trackTotalHits).to.equal(true); | ||
| expect(service.esOptions.versionField).to.equal('updatedAt'); | ||
| }); | ||
|
|
||
| it('passes every other elasticsearch option through to esParams unchanged', () => { | ||
| const service = makeService({ index: 'people', type: 'doc', refresh: true }); | ||
|
|
||
| expect(service.esParams).to.deep.equal({ index: 'people', type: 'doc', refresh: true }); | ||
| }); | ||
|
|
||
| it('defaults esParams.refresh to false and esOptions to undefined when not configured', () => { | ||
| const service = makeService({ index: 'people' }); | ||
|
|
||
| expect(service.esParams).to.deep.equal({ index: 'people', refresh: false }); | ||
| expect(service.esOptions.trackTotalHits).to.equal(undefined); | ||
| expect(service.esOptions.versionField).to.equal(undefined); | ||
| }); | ||
| }); | ||
|
|
||
| describe('external versioning on bulk create', () => { | ||
| // Returns the action descriptors (every other element) of the bulk body the service sent. | ||
| const bulkActionsFor = async (elasticsearch, docs, params) => { | ||
| let body; | ||
| const service = new Service({ | ||
| Model: { | ||
| bulk: (sent) => { | ||
| body = sent.body; | ||
| return Promise.resolve({ items: [] }); | ||
| }, | ||
| }, | ||
| esVersion: '6.0', | ||
| elasticsearch, | ||
| }); | ||
|
|
||
| await service._create(docs, params); | ||
|
|
||
| return body.filter((_, index) => index % 2 === 0); | ||
| }; | ||
|
|
||
| it('stamps external_gte from the configured versionField', async () => { | ||
| const actions = await bulkActionsFor( | ||
| { index: 'people', versionField: 'updatedAt' }, | ||
| [{ _id: '1', updatedAt: '2026-08-24T10:00:00.123Z' }], | ||
| { upsert: true }, | ||
| ); | ||
|
|
||
| expect(actions[0].index.version).to.equal(Date.parse('2026-08-24T10:00:00.123Z')); | ||
| expect(actions[0].index.version_type).to.equal('external_gte'); | ||
| }); | ||
|
|
||
| // Date.parse() on a Date object goes through toString(), which drops milliseconds. Two | ||
| // writes in the same second would then tie, and external_gte accepts a tie. | ||
| it('preserves milliseconds when the version field is a Date object', async () => { | ||
| const updatedAt = new Date('2026-08-24T10:00:00.123Z'); | ||
| const actions = await bulkActionsFor( | ||
| { index: 'people', versionField: 'updatedAt' }, | ||
| [{ _id: '1', updatedAt }], | ||
| { upsert: true }, | ||
| ); | ||
|
|
||
| expect(actions[0].index.version).to.equal(updatedAt.getTime()); | ||
| }); | ||
|
|
||
| it('accepts an epoch number as the version', async () => { | ||
| const actions = await bulkActionsFor( | ||
| { index: 'people', versionField: 'updatedAt' }, | ||
| [{ _id: '1', updatedAt: 1787565600123 }], | ||
| { upsert: true }, | ||
| ); | ||
|
|
||
| expect(actions[0].index.version).to.equal(1787565600123); | ||
| }); | ||
|
|
||
| it('writes unversioned when the field is missing or unparsable', async () => { | ||
| const actions = await bulkActionsFor( | ||
| { index: 'people', versionField: 'updatedAt' }, | ||
| [{ _id: '1' }, { _id: '2', updatedAt: 'not a date' }], | ||
| { upsert: true }, | ||
| ); | ||
|
|
||
| actions.forEach((action) => { | ||
| expect(action.index).to.not.have.property('version'); | ||
| expect(action.index).to.not.have.property('version_type'); | ||
| }); | ||
| }); | ||
|
|
||
| it('does not version the create action, which cannot overwrite anyway', async () => { | ||
| const actions = await bulkActionsFor( | ||
| { index: 'people', versionField: 'updatedAt' }, | ||
| [{ _id: '1', updatedAt: '2026-08-24T10:00:00.123Z' }], | ||
| {}, | ||
| ); | ||
|
|
||
| expect(actions[0]).to.have.property('create'); | ||
| expect(actions[0].create).to.not.have.property('version'); | ||
| }); | ||
|
|
||
| it('writes unversioned when no versionField is configured', async () => { | ||
| const actions = await bulkActionsFor( | ||
| { index: 'people' }, | ||
| [{ _id: '1', updatedAt: '2026-08-24T10:00:00.123Z' }], | ||
| { upsert: true }, | ||
| ); | ||
|
|
||
| expect(actions[0].index).to.not.have.property('version'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('$noFetch', () => { | ||
| it('returns the bulk metadata without a follow-up mget', async () => { | ||
| let mgetCalled = false; | ||
| const service = new Service({ | ||
| Model: { | ||
| bulk: () => Promise.resolve({ items: [{ index: { _id: '1', status: 201 } }] }), | ||
| mget: () => { | ||
| mgetCalled = true; | ||
| return Promise.resolve({ docs: [] }); | ||
| }, | ||
| }, | ||
| esVersion: '6.0', | ||
| elasticsearch: { index: 'people' }, | ||
| }); | ||
|
|
||
| const result = await service._create([{ _id: '1' }], { upsert: true, $noFetch: true }); | ||
|
|
||
| expect(mgetCalled).to.equal(false); | ||
| expect(result[0]._meta).to.deep.equal({ _id: '1', status: 201 }); | ||
| }); | ||
| }); | ||
|
|
||
| /** | ||
| * `paginate: false` with no `$limit` leaves `size` undefined, so Elasticsearch applies its | ||
| * own default of 10 and a full export silently returns 10 rows. master fixed this in | ||
| * `653ff91` (REF-19943); this branch forked before that commit and dropped it, which is how | ||
| * it came back. Driven through a stub `Model.search` so no server is needed. | ||
| */ | ||
| describe('find: size when paginate is false', () => { | ||
| const captureSearch = async (params) => { | ||
| let sent; | ||
| const service = new Service({ | ||
| Model: { | ||
| search: (findParams) => { | ||
| sent = findParams; | ||
| return Promise.resolve({ hits: { hits: [], total: 0 } }); | ||
| }, | ||
| }, | ||
| esVersion: '6.0', | ||
| elasticsearch: { index: 'people' }, | ||
| paginate: { default: 10, max: 50 }, | ||
| }); | ||
| await service.find(params); | ||
| return sent; | ||
| }; | ||
|
|
||
| it('falls back to 10000 when paginate is false and no $limit is given', async () => { | ||
| const sent = await captureSearch({ paginate: false, query: {} }); | ||
| expect(sent.size).to.equal(10000); | ||
| }); | ||
|
|
||
| it('still honours an explicit $limit when paginate is false', async () => { | ||
| const sent = await captureSearch({ paginate: false, query: { $limit: 25 } }); | ||
| expect(sent.size).to.equal(25); | ||
| }); | ||
|
|
||
| it('leaves the paginated path alone', async () => { | ||
| const sent = await captureSearch({ query: { $limit: 5 } }); | ||
| expect(sent.size).to.equal(5); | ||
| }); | ||
| }); | ||
| }); | ||
| /* eslint-enable no-underscore-dangle */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getVersioninghas no live caller — as shipped this feature is unreachable.Versioning applies only when
method === 'index', i.e. only whenparams.upsertis set. Tracing every write path in the tree:FlexStoreService._create→ElasticService._create→super._createwith noupsert→method === 'create'→versioning = {}upsert: truein birds isElasticService.ts:78, the_updatefallback — and_updatenow callsthis.Model.index()directly, bypassing this library entirely and using its own copy of the version logicseeds/essync.mjs) posts raw_bulkover HTTP and never touches feathers-esxSo 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.isFinitelogic atElasticService.ts:110-113, with a deliberate wire-key difference (version_typesnake_case here for the bulk NDJSON action line,versionTypecamelCase there for the legacy client'sindex()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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I disagree with.
getVersioninghas a live caller, in birds#408, which is in the same review set.birds/src/services/FlexStore/FlexStoreService.ts:547, insidesyncUpdateBulk:An array reaches
_create, which routes tocreateBulk(lib/index.js:104-113);params.upsertis truthy somethod === 'index', andgetVersioningruns. That is on the pushed headorigin/REF-25128atd1e08046, not local-only.Your point about
seeds/essync.mjsis correct: it posts raw_bulkover HTTP and never touches this library. It carries its own copy of the versioning atessync.mjs:206-215.The duplication point stands, and is now worse than when you wrote it. I fixed the
Date.parsetruncation here, butbirds/src/services/FlexStore/ElasticService.ts:117still hasDate.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.