Skip to content

fix(core): dedupe integration on re-authorize#572

Open
d-klotz wants to merge 1 commit intonextfrom
fix/dedupe-integration-on-reauth
Open

fix(core): dedupe integration on re-authorize#572
d-klotz wants to merge 1 commit intonextfrom
fix/dedupe-integration-on-reauth

Conversation

@d-klotz
Copy link
Copy Markdown
Contributor

@d-klotz d-klotz commented Apr 13, 2026

Summary

  • CreateIntegration now looks up an existing integration by (userId, config.type, entityIds) before creating a new one. If found, the existing record is reused and testAuth() runs so stale credentials surface via the usual status/messages pipeline — no ON_CREATE re-send, no config overwrite.
  • Entity identity already encodes the external account because ProcessAuthorizationCallback.findOrCreateEntity dedupes entities by (userId, moduleName, externalId). Re-auth returns the same entity.id, so the integration record was the only place duplicates still leaked in.
  • Entity comparison is order-insensitive (it's a set).

Files changed

  • integrations/use-cases/create-integration.js — dedup logic + _buildInstance extraction.
  • integrations/repositories/integration-repository-interface.js — new abstract findIntegrationByUserIdTypeAndEntities.
  • integrations/repositories/integration-repository-{mongo,postgres,documentdb}.js — implementations. Filter by userId + config.type at the DB layer, then order-insensitive set equality on entity IDs in JS.
  • integrations/tests/doubles/test-integration-repository.js — in-memory version.
  • integrations/tests/doubles/dummy-integration-class.js — adds a testAuth() spy (no other tests reference it).
  • integrations/tests/use-cases/create-integration.test.js — 6 new cases.

Test plan

  • npm test -- --testPathPattern="create-integration" — 13/13 pass (6 new).
  • npm test -- --testPathPattern="integrations/tests" — 129/129 pass.
  • Lint: no new errors. Pre-existing abstract-method arg warnings only.

Notes

  • Behaviour change only on the "re-auth" path — fresh creations unchanged.
  • The testAuth() on reuse writes to messages.errors and flips status to ERROR on failure via UpdateIntegrationStatus / UpdateIntegrationMessages, so the API response naturally reflects credential validity.

CreateIntegration previously called integrationRepository.createIntegration
unconditionally, so every re-auth POST /api/integrations created a fresh
Integration row for the same user + type + entity set. Entities are already
deduped in ProcessAuthorizationCallback by (userId, moduleName, externalId),
so re-auth returns the same entity.id — the integration record is where the
duplicate leaks in.

Now CreateIntegration looks up an existing integration keyed by
(userId, config.type, entityIds). If found, it loads the integration
instance and runs testAuth() so stale credentials surface via the normal
status/messages pipeline instead of silently succeeding; no ON_CREATE is
re-sent and config is not overwritten. Entity comparison is
order-insensitive since entityIds is a set.

Adds findIntegrationByUserIdTypeAndEntities to the repository interface and
implementations for Mongo, Postgres, and DocumentDB, plus the in-memory
test double. Extends create-integration.test.js with cases covering reuse,
testAuth invocation, order-insensitive matching, and the discriminators
(type, entity set, userId) that should still produce fresh records.
@d-klotz d-klotz added release Create a release when this pr is merged prerelease This change is available in a prerelease. labels Apr 13, 2026
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd85498e4a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +53 to +54
await instance.testAuth();
return mapIntegrationClassToIntegrationDTO(instance);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return refreshed state after re-auth testAuth

When an existing integration is reused, instance.testAuth() persists status/message changes through repository use-cases, but those writes do not mutate the in-memory instance object; immediately mapping instance to DTO can therefore return stale status/messages even when auth just failed. This affects re-authorize requests that depend on the response payload to reflect credential validity, so the flow should reload the integration (or update the instance fields) before returning.

Useful? React with 👍 / 👎.

@d-klotz
Copy link
Copy Markdown
Contributor Author

d-klotz commented Apr 13, 2026

Code review

Found 1 issue:

  1. On the reuse path, await instance.testAuth() persists status/messages to the DB via updateIntegrationStatus.execute(...) / updateIntegrationMessages.execute(...) but does not mutate this.status / this.messages on the in-memory instance. The next line returns mapIntegrationClassToIntegrationDTO(instance), which reads directly from the instance — so when auth has just failed, the DB row is ERROR but the HTTP response still shows the pre-testAuth status (typically ENABLED) with no errors. This contradicts this PR's own JSDoc promising stale credentials "surface as an ERROR status / message".

);
if (existing) {
const instance = await this._buildInstance(existing);
await instance.testAuth();
return mapIntegrationClassToIntegrationDTO(instance);
}
const integrationRecord =

Fix options: re-read the record from the repository after testAuth() and map that, or have testAuth() also mutate the instance's status / messages when it updates the DB.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

prerelease This change is available in a prerelease. release Create a release when this pr is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant