Skip to content

fix(auth): deserialize listed secrets when recursive is enabled - #12810

Merged
davidsbatista merged 2 commits into
deepset-ai:mainfrom
Lesereingrape:fix/deserialize-secrets-recursive
Sep 23, 2026
Merged

davidsbatista merged 2 commits into
deepset-ai:mainfrom
Lesereingrape:fix/deserialize-secrets-recursive

Conversation

@Lesereingrape

@Lesereingrape Lesereingrape commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Related Issues

Proposed Changes:

deserialize_secrets_inplace checked "is this value a dict and is recursion on?" before "is this key one of the secrets I was asked to restore?". Since a serialized Secret is itself a dict ({'type': 'env_var', 'env_vars': [...], 'strict': ...}), every listed key was captured by the recursion branch with recursive=True, so the Secret.from_dict branch was unreachable for precisely the inputs it exists to serve — enabling recursion made the helper restore fewer secrets than disabling it. The recursive call also omitted the recursive argument, so the traversal stopped one level below the top.

The change reorders the two conditions inside the same loop and forwards the flag:

  • listed keys are always deserialized, recursion on or off;
  • unlisted dict values are descended into, at any depth, when recursive=True;
  • recursive=False behavior is byte-for-byte what it was (the first branch required recursive anyway).

Impact for users: haystack.utils.deserialize_secrets_inplace is the helper the docs recommend for rebuilding secrets in a custom component's from_dict; with recursive=True callers kept plain dicts where a Secret was expected, and the failure surfaced later as AttributeError: 'dict' object has no attribute 'resolve_value'.

How did you test it?

Added 4 unit tests to test/utils/test_auth.py (that file had no coverage of this helper before):

test on main on this branch
..._deserializes_listed_keys (control, recursive=False) pass pass
..._leaves_non_secret_nested_dicts_untouched (control) pass pass
..._deserializes_listed_keys_when_recursive fail (dict) pass
..._recurses_into_nested_dicts (depth 3) fail (dict) pass
# branch
pytest test/utils/test_auth.py -q  ->  7 passed

# same test file, source file reverted to b717d00
pytest test/utils/test_auth.py -q  ->  2 failed, 5 passed

Environment note, stated plainly: hatch is not available on my machine, so I did not run hatch run test:unit / hatch run test:types. I ran against an editable install of this checkout (haystack 3.2.0-rc0, import haystack resolving to the clone, Python 3.13) with pytest directly. For lint I ran the repository's pinned ruff (v0.16.0, the rev in .pre-commit-config.yaml) on the changed files — All checks passed! / 2 files already formatted — plus the repo's own scripts/release_note_backticks.py on the new release-note file (exit 0). mypy was not run. Pre-commit hooks are not installed in my clone, so the individual hook tools above were run directly rather than through pre-commit run --all-files; codespell was not run. CI will be the first full check.

Notes for the reviewer

  • Only haystack/utils/auth.py:234-238 changes; the release note is in releasenotes/notes/fix-deserialize-secrets-recursive-388f1fab3ec25639.yaml.
  • Deliberate behavior difference to look at: an unlisted key whose dict value is a serialized secret is still not deserialized (the key must be named in keys), and a listed key whose dict value is not a secret dict will now reach Secret.from_dict while recursive=True where before it was skipped. Secret.from_dict raises on a missing/unknown type, so that case becomes a clear error instead of a silent pass-through — I judged that preferable to guessing, and the None case stays untouched.
  • I noticed fix: treat blank env vars as unset in Secret.from_env_var #12723 touches the same file for an unrelated reason (blank env vars in Secret.from_env_var); its hunks are at :63, :183, :211 and do not overlap this loop, so the two changes should rebase cleanly in either order.
  • This contribution was made with an AI coding assistant: it located the asymmetry, wrote the reproduction and the tests, and measured the before/after numbers quoted above. This PR was opened during an unattended contribution run, so the account owner has not read the diff yet — please review it with that in mind, and it will be amended or closed on request.

Checklist

  • I have read the contributors guidelines and the code of conduct.
  • I have updated the related issue with new insights and changes.
  • I have added unit tests and updated the docstrings. (Tests added; the recursive docstring already described the fixed behavior, so no docstring change was needed.)
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test: and added ! in case the PR includes breaking changes.
  • I have documented my code.
  • I have added a release note file, following the contributors guidelines.
  • I ran the tools behind the pre-commit hooks that apply to my files (repo-pinned ruff 0.16.0 check + format, release_note_backticks.py) and they are clean; I could not run hatch, mypy or the full hook suite locally — see "How did you test it?" for the exact scope.

@Lesereingrape
Lesereingrape requested a review from a team as a code owner September 19, 2026 08:47
@Lesereingrape
Lesereingrape requested review from davidsbatista and removed request for a team September 19, 2026 08:47
@vercel

vercel Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

@Lesereingrape is attempting to deploy a commit to the deepset Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions Bot added the type:documentation Improvements on the docs label Sep 23, 2026
@vercel

vercel Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
haystack-docs Ready Ready Preview Sep 23, 2026 9:44am UTC

Request Review

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  haystack/utils
  auth.py
Project Total  

This report was generated by python-coverage-comment-action

@davidsbatista
davidsbatista enabled auto-merge (squash) September 23, 2026 09:49
@HaystackBot

Copy link
Copy Markdown
Contributor

Hi @Lesereingrape, thanks a lot for your contribution! 🙏

We noticed that the Contributor License Agreement (CLA) check (license/cla) hasn't passed yet, so we've temporarily moved this PR to draft and paused the review assignment.

To get your PR reviewed, please sign the CLA via the link in the license/cla check below (or in the CLA bot comment). As soon as the check turns green, this PR will automatically be marked ready for review again and a reviewer will be re-assigned.

@HaystackBot HaystackBot added the cla-pending PR is in draft until the contributor signs the CLA label Sep 23, 2026
@HaystackBot
HaystackBot marked this pull request as draft September 23, 2026 09:52
auto-merge was automatically disabled September 23, 2026 09:52

Pull request was converted to draft

@davidsbatista

Copy link
Copy Markdown
Contributor

@Lesereingrape you need to sign the license agreement in the next few days otherwise the PR will be closed

@HaystackBot
HaystackBot marked this pull request as ready for review September 23, 2026 10:29
@HaystackBot HaystackBot removed the cla-pending PR is in draft until the contributor signs the CLA label Sep 23, 2026
@HaystackBot

Copy link
Copy Markdown
Contributor

Thanks for signing the CLA, @Lesereingrape! 🎉 This PR is now ready for review again and the reviewer has been re-assigned.

@davidsbatista
davidsbatista merged commit 9678a74 into deepset-ai:main Sep 23, 2026
34 checks passed

This branch was successfully deployed

1 active deployment
Preview — 39b8ee7b Deployed Sep 23, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic:tests type:documentation Improvements on the docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deserialize_secrets_inplace with recursive=True stops restoring the keys you listed

3 participants