Skip to content

feat(jetstream): add consumers.createOrUpdate#412

Open
styler wants to merge 2 commits into
nats-io:mainfrom
styler:consumer-create-or-update
Open

feat(jetstream): add consumers.createOrUpdate#412
styler wants to merge 2 commits into
nats-io:mainfrom
styler:consumer-create-or-update

Conversation

@styler
Copy link
Copy Markdown

@styler styler commented May 9, 2026

Mirrors CreateOrUpdateConsumer in the Go client — useful for KV/object-store init paths and any caller that wants idempotent setup without doing the info-then-decide dance themselves.

Also a small fix in addUpdate: the action default used ||, which coerced the empty-string CreateOrUpdate value back to "create". Changed to ?? so the action survives. Today this only matters for the new createOrUpdate path (the existing add/update callers always pass a truthy action), but worth fixing in the same PR since the new method depends on it.

Mirrors CreateOrUpdateConsumer in the Go client.

Also switches addUpdate's action default from || to ?? so the empty-string
CreateOrUpdate action isn't silently coerced back to "create".

Signed-off-by: Styler <styler@streamelements.com>
@synadia-claude-reviewer
Copy link
Copy Markdown
Contributor

synadia-claude-reviewer Bot commented May 9, 2026

Claude finished @styler's task in 2m 4s —— View job


PR Review: feat(jetstream): add consumers.createOrUpdate

  • Gather PR diff and context
  • Read source files for context
  • Analyze for bugs, security, performance, TypeScript correctness
  • Post inline comments on specific issues
  • Post overall review summary

Overall Assessment

The change is small, correct, and follows existing conventions. Two observations below.

||?? fix (jsmconsumer_api.ts:99) — Correct and necessary. ConsumerApiAction.CreateOrUpdate is "" (empty string), which is falsy, so || would silently coerce it to "create". ?? only falls back on null/undefined, which is the right semantics here.

createOrUpdate implementation — Correct. The spread {...(opts ?? {}), action: ConsumerApiAction.CreateOrUpdate} always forces the action to CreateOrUpdate regardless of what opts provides, which is the right contract for this method. Since ConsumerCreateOptions doesn't include action, callers can't accidentally override it through the typed API.

Two things worth addressing before merge:

  1. Test gap — The added test only exercises the update path (consumer exists, config differs → should update). There's no assertion for the create path (consumer does not exist → should create). That's half the contract left untested. See inline comment →

  2. Interface/implementation type mismatchtypes.ts declares cfg: Partial<ConsumerConfig>, but jsmconsumer_api.ts implements it as cfg: ConsumerConfig. This follows the same existing pattern as add, so it isn't a regression introduced here — but it does mean callers relying on the interface type could pass a genuinely partial config that the implementation cannot handle. Worth tracking as a follow-up if not addressed here. See inline comment →

No security or async issues found. Cross-runtime compatibility is unaffected (no runtime-specific code paths).


// createOrUpdate must accept the modified config and apply it
const ci = await jsm.consumers.createOrUpdate("stream", config);
assertEquals(ci.config.inactive_threshold, nanos(60 * 1000));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test only covers the update path (existing consumer, modified config). It would be good to also assert the create path — e.g. call createOrUpdate for a consumer name that doesn't exist yet and verify it succeeds without error. That's the other half of "create or update".

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added — covers a fresh durable name in the same test.

return this.addUpdate(stream, cfg, cco);
}

createOrUpdate(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The interface in types.ts declares cfg: Partial<ConsumerConfig>, but this implementation uses cfg: ConsumerConfig. This follows the same pattern as add, so it's consistent with the existing convention — just flagging it in case the intent is to accept genuinely partial configs (the implementation and the underlying addUpdate both require a full ConsumerConfig).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Matching add's signature on purpose — the impl/interface mismatch predates this PR and applies to add too. Would rather fix both in a follow-up than diverge here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor nit - there's no create in the ConsumerAPI, so addOrUpdate possibly is more consistent? wdyt?

Signed-off-by: Styler <styler@streamelements.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants