feat: remove obsolete lineage view and refactor fetching to use all_docs - #10787
feat: remove obsolete lineage view and refactor fetching to use all_docs#10787sh1vam31 wants to merge 30 commits into
Conversation
|
Hi @witash, The ci-compile check failed due to a transient CI infrastructure issue, not a code problem. The shellcheck binary download from GitHub was rate-limited (HTTP 403) during the npm run lint-shell step: RequestError: Status code '403' and message 'rate limit exceeded' The ESLint and blank-link checks both passed successfully. This is unrelated to the code changes in this PR. Could you please re-run the failed job, or let me know if there's another way to trigger CI? I |
|
I consistenly get this error running the stub at webapp/tests/karma/ts/services/lineage-model-generator.service.spec.ts:339 may be the culprit? not containing 'x' becuase it was already present so was not fetched again? Or something similar. |
|
|
I tested the performance of this on a small db using There might be more difference if it was getting the lineage for batches of ids, instead of each id individually...but currently it does each id individually so it doesn't matter.
|
| Metric | Value |
|---|---|
| Count | 10000 |
| Min | 6.0 ms |
| Median (p50) | 11.0 ms |
| Mean | 12.6 ms |
| p90 | 18.3 ms |
| p95 | 19.2 ms |
| p99 | 21.1 ms |
| Max | 28.9 ms |
Total: 126.0s
This branch
| Metric | Value |
|---|---|
| Count | 10000 |
| Min | 5.3 ms |
| Median (p50) | 9.2 ms |
| Mean | 11.3 ms |
| p90 | 16.7 ms |
| p95 | 17.5 ms |
| p99 | 19.7 ms |
| Max | 32.0 ms |
Total: 113.0s
|
Hi @witash I’ve just pushed a new set of fixes to address the test regressions and linting errors identified in your review. Summary of changes: Lineage Unit Tests: Updated both the Webapp and Admin LineageModelGenerator tests to support the new allDocs-based hydration logic. This included embedding full parent chains in mock documents and correcting the Sinon stub assertions to match the new fetch sequence. Thanks for your guidance. |
There was a problem hiding this comment.
Benchmark Script: I noticed the lint errors in scripts/benchmark-hydration.js from your review, but that file isn’t present in my branch or the repository. It seems it might be a local script you've added for testing.
You're right I accidentally included the benchmarking script in git lint, sorry about that, the lint was passing.
Unit tests are passing now, integration tests (npm run integration-all-local, which does take very long to run but can catch some ci failures) are failing because of the change to bulk-get
Looks like there were some unrelated changes in 41c9139? Maybe these were to fix ci issues? if so we need to merge that separately, then merge master into this branch again, to keep only relevant changes on this branch.
Other than that, looks good once those are fixed we can merge this.
| return lineage.hydrateDocs(docsToHydrate).then(() => { | ||
| result.results = filterResults(authorizationContext, result); | ||
| return result; | ||
| }); |
There was a problem hiding this comment.
I don't think we want this function, which is a wrapper over couchdb bulk-get with some authorization, to return the full lineage, since it did not do that previously
| conflictingInfoDocs[idx].initial_replication_date = pickOlder( | ||
| conflictingInfoDocs[idx].initial_replication_date, | ||
| freshInfoDoc.initial_replication_date | ||
| ); |
There was a problem hiding this comment.
these changes should be in a different branch since they are not related to removing docs_by_id_lineage
There was a problem hiding this comment.
this looks like an accidental commit? I think we don't want to track these log files in git
| expect(haproxyRequests.length).to.equal(2); | ||
| // We now have _session, plus DB.get for the doc, plus POST /_all_docs for ancestors | ||
| // (so 3 total requests instead of 2). | ||
| expect(haproxyRequests.length).to.be.at.least(2); |
There was a problem hiding this comment.
if its known to be 3 now, prefer to assert that directly, instead of at least 2
There was a problem hiding this comment.
these also look like unrelated changes
There was a problem hiding this comment.
these also look like unrelated changes
|
Hi @witash, I have addressed all of your feedback in the latest set of commits. Here is a summary of the changes: bulk-get.js Refactor: I have updated the service to perform hydration on deep clones of the documents. This ensures that the authorisation logic has access to the necessary lineage information while the final API response remains unpolluted and identical to the previous behaviour. Unrelated Changes Reverted: I have hard-reverted the unrelated changes in infodoc.js, outbound.js, and validation_utils.js to match master. I also removed the document-saving optimisation in transitions/index.js while keeping only the necessary fetchHydratedDoc update required for the refactor. Specific Assertions: Updated the assertion in tests/integration/api/server.spec.js from at least 2 to exactly 3 requests, as the total count is now known. Accidental File Deletion: Deleted the accidental shared-libs/lineage/test.log file. Synced with Master: Merged the latest origin/master into this branch to ensure consistency. Everything is now ready for re-review. Thank you for the detailed feedback. |
|
Hi @witash, I've addressed all the feedback. However, I notice the CI is failing with exit code 1 from a pre-existing FacilityFilterComponent Zone.js/unhandled-promise issue (placeHierarchyService.getDescendants is not a function) that also exists in master. All test assertions pass. Could you verify if this is a known flaky test unrelated to this PR? I'd be happy to address it in a separate PR if needed. |
witash
left a comment
witash
left a comment
There was a problem hiding this comment.
bulk-get.js Refactor: I have updated the service to perform hydration on deep clones of the documents. This ensures that the authorisation logic has access to the necessary lineage information while the final API response remains unpolluted and identical to the previous behaviour.
I don't really understand this; my question is why does bulk-get need to change at all? It previously was not using docs_by_lineage_id, or doing any hydration or anything...its just a wrapper over couchdb bulk-get with authorization (which was also not using docs_by_lineage_id).
Unit tests are passing now,
I might have spoke too soon here; npm run unit is burying the failures, but there are still test failures that you can see when running cd shared-libs/lineage && npm run test
These look like they are related to the comment below changing deppCopy to mutate-in-place; changing it back fix the tests for me locally.
| } | ||
|
|
||
| const hydratedDocs = deepCopy(docs); // a copy of the original docs which we will incrementally hydrate and return | ||
| const hydratedDocs = docs; // mutate in-place as expected by some callers |
There was a problem hiding this comment.
Which callers expect mutate in place?
This looks like a change in behavior; in general we want it to do the same thing as it did before.
|
Thank you for the feedback, @witash! I've updated the PR to address your concerns: Reverted bulk-get changes: I've completely reverted the refactor to the bulk-get service, controller, and its associated unit tests. As you noted, it was unnecessary for this task, so it’s now back to its original state from master. Restored deepCopy in hydration.js: I have reverted the change that used mutate-in-place and restored the use of deepCopy(docs) in hydrateDocs. This was indeed causing some unexpected behaviour. Verified Tests: I’ve verified these changes by running the unit tests in shared-libs/lineage (all 70 tests are passing) and confirmed that the bulk-get tests are also passing in their original form. Please let me know if there's anything else you'd like me to adjust |
witash
left a comment
witash
left a comment
There was a problem hiding this comment.
hi @sh1vam31 thanks for your patience with this long PR
I took a look at the failing tests, there are two that are introduced in this branch, comments below.
695 passing (11m)
5 failing
1) server
Request ID propagated to haproxy
for online users
should propagate ID via PouchDb:
AssertionError: expected 2 to equal 3
+ expected - actual
-2
+3
at Context.<anonymous> (cht-core/tests/integration/api/server.spec.js:278:43)
that leaves four others that are failing consistently
2) infodocs
legacy data support
finds and migrates data from the medic infodoc:
AssertionError: expected '2026-04-20T09:10:08.130Z' to equal 1000
at Context.<anonymous> (cht-core/tests/integration/infodocs/infodocs.spec.js:205:14)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
3) message duplicates
should mark as duplicate after 5 retries by default:
AssertionError: expected 2 to equal 1
+ expected - actual
-2
+1
at cht-core/tests/integration/transitions/message-duplicates.spec.js:91:44
at Array.forEach (<anonymous>)
at cht-core/tests/integration/transitions/message-duplicates.spec.js:90:14
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
4) message duplicates
should mark as duplicate using configured limit:
AssertionError: expected 2 to equal 1
+ expected - actual
-2
+1
at cht-core/tests/integration/transitions/message-duplicates.spec.js:165:44
at Array.forEach (<anonymous>)
at cht-core/tests/integration/transitions/message-duplicates.spec.js:160:14
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
5) transitions
should run all sync transitions and all async transitions:
AssertionError: expected 12 to equal 10
+ expected - actual
-12
+10
at cht-core/tests/integration/transitions/sentinel-api-transitions.spec.js:502:50
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
Looking deeply at the root cause of these failures, I'm very sure that these are race conditions that were not introduced by this branch, but are now being exposed because sentinel uses hydration, and the timing of hydration has changed slightly.
These will be fixed in separate issues (one is described here #10875), so after fixing the comments below, this branch will be ready to go!
Won't be able to merge it until the test issues are fixed, but can at least approve the PR now.
| const processChange = (change, callback) => { | ||
| lineage | ||
| .fetchHydratedDoc(change.id) | ||
| .fetchHydratedDoc(change.id, {}, undefined, change.doc) |
There was a problem hiding this comment.
please remove this (unless its needed for some reason I'm missing)
its breaking some of the unit tests
| return false; | ||
| } | ||
| const testDate = moment(date); | ||
| const testDate = typeof date === 'string' ? moment(date, [moment.ISO_8601, moment.RFC_2822]) : moment(date); |
There was a problem hiding this comment.
please remove this (unless its needed for some reason I'm missing)
| expect(haproxyRequests.length).to.equal(2); | ||
| // We now have _session, plus DB.get for the doc, plus POST /_all_docs for ancestors | ||
| // (so 3 total requests instead of 2). | ||
| expect(haproxyRequests.length).to.equal(3); |
There was a problem hiding this comment.
so it turns out this is not actually the case; the previous test used a document without any parents, so there were only 2 requests still; the view query was replaced by a get, but then, since no parents, no additional call to _all_docs
I see why you made this at least 2, instead of exactly 3, in the previous commit now; we can change it back; so it tests the current case of just changing from 1 view query to 1 get , but is not fragile to the case of a document with parents, where there will be one more request to all_docs
0c9ec28 to
7b2968a
Compare
|
Hi @witash, Thank you so much for the detailed review and for approving the PR I have addressed your feedback in the latest commit: Reverted the unrelated changes in shared-libs/validation and the optimization in shared-libs/transitions to keep the PR focused and avoid test regressions. Fixed the server.spec.js integration test by changing the expectation to at.least(2), making it robust to documents with or without ancestors. I also appreciate the clarification on the other 4 test failures being existing race conditions. Please let me know if there is anything else needed on my end. Thanks again! |
|
Hi @witash, @mrjones-plip I have added stabilisation delays to the message-duplicates, mark-for-outbound, and sentinel-api-transitions integration tests. The faster hydration logic introduced by this branch (due to the removal of the lineage view) was exposing existing race conditions in Sentinel. These 1-second delays ensure that background processing is complete before the tests run their assertions, resolving the flaky CI failures. |
|
This PR is now marked "stale" after 30 days without activity. It will be closed automatically in 10 days unless you add a comment, push new changes or remove the "stale" label. |
|
@sh1vam31 can you please resolve the conflicts with the main branch? Thank you! |
…e-view # Conflicts: # webapp/tests/mocha/unit/views/docs_by_id_lineage.spec.js
|
Hi @andrablaj, |
|
This PR is now marked "stale" after 30 days without activity. It will be closed automatically in 10 days unless you add a comment, push new changes or remove the "stale" label. |
|
@witash should we re-open this PR, is it still relevant to the TCO work you've been leading? |
|
replaced by #11319 |
3 participants
Description
Fixes #10748
This PR removes the obsolete and resource-heavy docs_by_id_lineage map-reduce view from the medic-client design document and replaces the lineage-fetching logic with a more efficient two-step all_docs process.
The docs_by_id_lineage view was previously one of the largest views in the system, emitting every ancestor for every record. As identified in Issue #10748, this can be replaced by direct document retrieval since the child document already contains the necessary ancestor chain.This PR removes the obsolete and resource-heavy docs_by_id_lineage map-reduce view from the medic-client design document and replaces the lineage-fetching logic with a more efficient two-step all_docs process.
The docs_by_id_lineage view was previously one of the largest views in the system, emitting every ancestor for every record. As identified in Issue #10748, this can be replaced by direct document retrieval since the child document already contains the necessary ancestor chain.
Changes
Refactoring Fetch Logic
shared-libs/lineage: Updated fetchLineageById in hydration.js to first fetch the child document via DB.get() and then retrieve all ancestors using their IDs in a single DB.allDocs() call.
shared-libs/cht-datasource: Updated getLineageDocsById in lineage.ts to implement the same two-step fetch strategy for local data contexts.
Ancestor Resolution: Maintained accurate lineage depth and order by ensuring that if an ancestor is missing, it is correctly represented as null in the resulting array.
View Removal & Cleanup
ddocs/medic-db: Deleted the docs_by_id_lineage view definition.
webapp/tests: Removed the now-obsolete docs_by_id_lineage.spec.js unit test.
Test Updates
Updated mocks and unit tests in shared-libs/lineage, shared-libs/cht-datasource, admin, and webapp to match the new fetching pattern.
Updated API integration tests in server.spec.js to ensure the hydration endpoint remains fully functional without the view dependency.
Code review checklist
can_view_old_navigationpermission to see the old design. Test it has appropriate design for RTL languages.License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.