Skip to content

Defer optional nkeys import into auth setup - #979

Draft
caspervonb wants to merge 2 commits into
mainfrom
defer-nkeys-import
Draft

Defer optional nkeys import into auth setup#979
caspervonb wants to merge 2 commits into
mainfrom
defer-nkeys-import

Conversation

@caspervonb

Copy link
Copy Markdown
Collaborator

A top-level import nkeys broke import nats.client on a plain pip install nats-core: nkeys ships only as the [nkeys] extra, so the unconditional import raised ImportError for everyone not using nkey/JWT auth.

The import now lives in the two auth-setup helpers (its only consumers), after their early returns. The base package loads without the extra, pre-built handler tuples and JWT parsing stay nkeys-free, and deriving a signer from a seed raises a clear Install nats-core[nkeys]. hint — mirroring how the websocket extra is guarded in connection.py.

A top-level `import nkeys` made `import nats.client` fail with
ImportError on a plain `pip install nats-core`, since nkeys ships only
as the `[nkeys]` extra. Move it into the nkey/JWT auth setup helpers —
its only consumers — after their early returns, so the base package
loads without the extra, pre-built handlers and JWT parsing stay
nkeys-free, and deriving a signer from a seed raises a clear hint.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 22, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context (PR diff, changed file)
  • Examine surrounding code for correctness
  • Check for security/correctness issues
  • Post inline comments if warranted
  • Post review summary

The fix is correct and necessary. The top-level import nkeys at module load time was a real regression for anyone using nats-core without the [nkeys] extra, and deferring it into the two helpers that actually need it is the right approach.

A few notes:

Correctness — The deferred-import helper (_import_nkeys) is structurally identical to the websockets guard already in connection.py (lines 318–321), so the pattern is established and consistent. _setup_nkey_auth calls it after the pre-built-tuple early return, before any file I/O. ✓

_setup_jwt_auth placement (see inline comment) — Here the import happens after creds files are already read, which is inconsistent with the nkey path and wastes I/O on misconfigured environments. Not a bug, but worth tidying.

Performance — Python caches modules in sys.modules, so repeated calls to _import_nkeys() are a dict lookup; no concern there.

Pre-built handler paths — Both functions correctly short-circuit before touching nkeys when the caller already supplies handler tuples, so users providing custom closures are unaffected even without the extra installed.

Overall: one minor suggestion left as an inline comment; the core fix is sound.

Comment thread nats-core/src/nats/client/__init__.py Outdated
msg = f"Invalid jwt argument: {jwt!r}"
raise TypeError(msg)

nkeys = _import_nkeys()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: _import_nkeys() is placed after all the file I/O (.read_text() / .read_bytes()), so a user without nkeys installed who passes a .creds or seed file will have that file read before the clear ImportError is raised. This is inconsistent with _setup_nkey_auth, where _import_nkeys() is called before the seed file is read.

Consider moving the call to just after the early-return guard, mirroring _setup_nkey_auth. Not a bug — the error is still clear and actionable — but the current placement does unnecessary file I/O before failing.

Matches _setup_nkey_auth, so a missing nkeys package raises its
ImportError before any JWT or seed file is read.
@caspervonb

Copy link
Copy Markdown
Collaborator Author

Review comments addressed, PTAL @claude

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @caspervonb's task in 59s —— View job


Re-review of addressed comments

  • Gather context
  • Read current state of nats-core/src/nats/client/__init__.py
  • Verify _import_nkeys() placement in _setup_jwt_auth
  • Post review summary

The inline comment has been fully addressed in commit 5ed1da9 ("Import nkeys before reading credential files").

What changed_import_nkeys() was moved from the bottom of _setup_jwt_auth (after all file I/O) to immediately after the early-return guard, before any file reads:

def _setup_jwt_auth(jwt):
    if isinstance(jwt, tuple) and callable(jwt[0]):
        return jwt                   # pre-built handlers: no nkeys needed

    nkeys = _import_nkeys()          # ← now here, before any I/O

    if isinstance(jwt, Path):
        creds_content = jwt.read_text()   # file I/O only reached with nkeys present
    ...

This exactly mirrors _setup_nkey_auth, which already had this ordering. A user without nkeys installed who passes a .creds file or a seed path now gets the clear ImportError immediately, with no wasted file reads.

Overall — both helpers are now consistent, the fix is correct, and no issues remain. LGTM.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant