Skip to content

Add message schedule header constants#939

Open
caspervonb wants to merge 1 commit into
mainfrom
backport-message-schedule-headers
Open

Add message schedule header constants#939
caspervonb wants to merge 1 commit into
mainfrom
backport-message-schedule-headers

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

Backport of nats-jetstream PR #923. Adds ADR-51 schedule header constants to the legacy JetStream module so existing nats-py users can set scheduled-publish headers (Nats-Schedule, Nats-Schedule-Target, etc.) without hard-coding strings. The allow_msg_schedules StreamConfig flag was already present.

@caspervonb caspervonb added the nats-legacy Legacy asyncio NATS client label May 15, 2026
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 15, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 1m 36s —— View job


PR Review: Add message schedule header constants

  • Gather context (PR diff, changed files)
  • Review nats/src/nats/js/api.py changes
  • Review nats/tests/test_js.py changes
  • Post review summary

Overall: The header string values are all correct — verified against nats-jetstream/src/nats/jetstream/headers.py. The schedule preset constants (@yearly, @monthly, etc.) are also accurate, and test coverage is solid.

One gap vs. the nats-jetstream source:

The nats-jetstream package exports NATS_SCHEDULE_ROLLUP_SUB: Final[str] = "sub", which is the only valid value for the Nats-Schedule-Rollup header. This backport adds Header.SCHEDULE_ROLLUP to the enum but omits the companion value constant. Without it, users must hardcode "sub" when setting that header. An inline comment has been left on the relevant line.

Minor observation (no action needed):

Header.SCHEDULER (Nats-Scheduler) and Header.SCHEDULE_NEXT (Nats-Schedule-Next) are server-set, read-only headers — the nats-jetstream source explicitly notes they should not be set by a publishing client. The Header enum already contains CONSUMER_STALLED under the same pattern (server-set, no comment), so this is consistent with the existing convention in the legacy package.

Comment thread nats/src/nats/js/api.py
@caspervonb

Copy link
Copy Markdown
Collaborator Author

@claude please review again

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 27, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 2m 5s —— View job


PR Review: Add message schedule header constants

  • Gather context (PR diff, changed files)
  • Review nats/src/nats/js/api.py changes
  • Review nats/tests/test_js.py changes
  • Compare with nats-jetstream source for parity
  • Post review summary

Overall: LGTM. All header string values are correct and the tests are complete. The deferred SCHEDULE_ROLLUP_SUB is a reasonable call given the minimal-backport intent.

nats/src/nats/js/api.py

Header enum — all 8 new members verified against nats-jetstream/src/nats/jetstream/headers.py:

Enum member Wire value Correct
Header.SCHEDULE "Nats-Schedule"
Header.SCHEDULE_TARGET "Nats-Schedule-Target"
Header.SCHEDULE_SOURCE "Nats-Schedule-Source"
Header.SCHEDULE_TTL "Nats-Schedule-TTL"
Header.SCHEDULE_TIME_ZONE "Nats-Schedule-Time-Zone"
Header.SCHEDULE_ROLLUP "Nats-Schedule-Rollup"
Header.SCHEDULER "Nats-Scheduler"
Header.SCHEDULE_NEXT "Nats-Schedule-Next"

Preset constants — all 5 verified:

Constant Value Correct
SCHEDULE_YEARLY "@yearly"
SCHEDULE_MONTHLY "@monthly"
SCHEDULE_WEEKLY "@weekly"
SCHEDULE_DAILY "@daily"
SCHEDULE_HOURLY "@hourly"

Alphabetical ordering in the enum is maintained correctly — new members fit cleanly between ROLLUP and STATUS.

Deferred SCHEDULE_ROLLUP_SUB — acknowledged. Worth noting for context: the existing Header.ROLLUP (Nats-Rollup) uses the same "sub" value, defined as MSG_ROLLUP_SUBJECT = "sub" in kv.py (used by KV and object store). The precedent in this codebase is that rollup value constants live alongside their consumers, not in api.py. Deferring SCHEDULE_ROLLUP_SUB is consistent with that pattern.

nats/tests/test_js.py

Both new test methods cover exactly what was added:

  • test_schedule_header_values: all 8 enum members
  • test_schedule_preset_constants: all 5 preset constants

No gaps in test coverage for the additions.

Summary

The changes are correct, minimal, and consistent with the rest of the nats.js.api module's conventions. The previous review's one concern (SCHEDULE_ROLLUP_SUB) has been addressed by the author with a clear rationale, and the existing rollup pattern in this codebase supports that decision.

@caspervonb caspervonb marked this pull request as ready for review May 29, 2026 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nats-legacy Legacy asyncio NATS client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant