fix(backend): add merging user as creator of the merged entity (#17913) - #18287
Gabriele Bonadiman (gabrielebonadiman) wants to merge 6 commits into
Conversation
Records the acting user in the merged entity's creator_id list during a merge, following the same convention already used for upsert, and adds an integration test that reproduces the bug and verifies the fix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18287 +/- ##
==========================================
- Coverage 36.37% 28.14% -8.24%
==========================================
Files 3467 3467
Lines 141015 141017 +2
Branches 38072 38073 +1
==========================================
- Hits 51295 39687 -11608
- Misses 89720 101330 +11610
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR records the merging user as a creator of the resulting entity when applicable.
Changes:
- Updates merge creator attribution.
- Excludes internal and
no_creatorsusers. - Adds merge and Elasticsearch test coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Review findings |
|---|---|
opencti-platform/opencti-graphql/src/database/middleware.ts |
Critical (3): duplicate creator_id inputs can overwrite existing creators. Moderate (1): add tests for internal and no_creators users. Critical (1): restore the no-continue lint suppression. |
opencti-platform/opencti-graphql/tests/02-dataInjection/01-dataCount/middleware-test.js |
Moderate (1): clean up both fixtures in finally. Nit (2): clarify the test title. |
opencti-platform/opencti-graphql/tests/03-integration/01-database/elLoadById-rel-payload-test.ts |
Nit (1): remove or split this unrelated test. Critical (1): assert the converted indicates property rather than rel_indicates. |
Suppressed comments (3)
opencti-platform/opencti-graphql/src/database/middleware.ts:1902
- The negative cases in this guard are part of the stated behavior, but the only merge test uses a normal user and exercises neither
INTERNAL_USERSnorno_creators. Add merge tests for an internal actor and a user withno_creators: true, asserting their IDs are not appended; otherwise either exclusion branch can regress unnoticed.
if (!INTERNAL_USERS[user.id] && !user.no_creators) {
updateAttributes.push({ key: 'creator_id', value: [user.id], operation: EditOperation.Add });
opencti-platform/opencti-graphql/tests/02-dataInjection/01-dataCount/middleware-test.js:1158
- If the merge or the subsequent load fails before
loadedThreatis assigned, thisfinallyblock deletes nothing, leaving both newly created fixtures in Elasticsearch. The unique suffix prevents name collisions but does not prevent failed runs from accumulating test data; clean uptargetandsourceas well, tolerating the source already being deleted by a partially completed merge.
if (loadedThreat) {
await deleteElementById(testContext, ADMIN_USER, loadedThreat.id, ENTITY_TYPE_THREAT_ACTOR_GROUP);
opencti-platform/opencti-graphql/tests/03-integration/01-database/elLoadById-rel-payload-test.ts:18
- This entire test exercises
elLoadByIdrelationship-payload sizing and the “SaaS/ES Circuit Breaker,” not merge creator behavior. It adds a 5,000-ID raw ES fixture to the merge PR even though the description says the change is scoped tomergeEntitiesRaw; please remove it from this PR or split it into the dedicated loader/performance change so this fix cannot fail for an unrelated assertion.
describe('elLoadById unbounded rel_* payload vs elFindByIds default exclusion', () => {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ts conflict Resolves a trivial conflict in convertAggregateDistributions caused by an unrelated formatting change on master, keeping the merge-as-creator fix and its test in middleware.ts and middleware-test.js intact.
…eedback Fold the merging user into the existing creator_id merge branch instead of pushing a second overwriting update, remove an unrelated stray test file, and add coverage for the internal-user and no_creators exclusion guards along with clearer test naming and cleanup.
Proposed changes
creator_idlist, matching the same convention already used during upsert (upsert-utils.js)INTERNAL_USERS) and users flagged withno_creatorsare excluded from this, consistent with the upsert behaviorRelated issues
How to test this PR
Covered by a new integration test in
middleware-test.js("should merging user be added as creator of the merged entity"), which creates a target and source entity asADMIN_USER, merges them as a distinct user, and assertscreator_idcontains both the original creator and the merging user.Checklist
Further comments
The fix is scoped to
mergeEntitiesRawinmiddleware.tsand reuses the existingEditOperation.Addpath for multi-valued attributes, socreator_idvalues are deduplicated automatically. No schema or GraphQL contract changes were needed sincecreator_idwas already declared as a multi-valued, updatable attribute.