Skip to content

Comments

recursiveGetAttrsetWithJqPrefix: fix top level values#434171

Merged
jian-lin merged 1 commit intoNixOS:masterfrom
Prince213:push-rvnmkknmmznx
Aug 18, 2025
Merged

recursiveGetAttrsetWithJqPrefix: fix top level values#434171
jian-lin merged 1 commit intoNixOS:masterfrom
Prince213:push-rvnmkknmmznx

Conversation

@Prince213
Copy link
Member

@Prince213 Prince213 commented Aug 16, 2025

This PR allows recursiveGetAttrsetWithJqPrefix to properly handle top level value such as dictionaries, which will have a empty name and cause jq to fail, and arrays where [0] would be used instead of the correct .[0].

Example:

let
  nixpkgs = fetchTarball "https://github.com/Prince213/nixpkgs/archive/refs/heads/push-rvnmkknmmznx.tar.gz";
  pkgs = import nixpkgs { };
  utils = import "${nixpkgs}/nixos/lib/utils.nix" {
    inherit pkgs;
    inherit (pkgs) lib;
    config = null;
  };
in
pkgs.runCommand "result.json" { } ''
  ${utils.genJqSecretsReplacementSnippet {
    _secret = pkgs.writeText "secret" (
      builtins.toJSON {
        answer = 42;
      }
    );
    quote = false;
  } "result.json"}
  cp result.json "$out"
''

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels Aug 16, 2025
Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

I guess the intended use case is the same as configFile, which is a bit weird to me. However, the implementation is simple and easy to understand, so I do not mind supporting that use case.

I tested it with NixOS tests for nginx-sso, glance and scrutiny.


I will merge this in a few days if Yinfeng has no other opinion.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 16, 2025
@Prince213
Copy link
Member Author

The idea was more about feature completeness and having less surprise. There's no documentation saying that top level secret is not supported so we should properly implement it. Also the implementation is simple enough that there isn't any reason not to support this use case.

@linyinfeng
Copy link
Member

linyinfeng commented Aug 16, 2025

I think the real problem is that the original recursiveGetAttrsetWithJqPrefix is not correct.

My fix:

diff --git a/nixos/lib/utils.nix b/nixos/lib/utils.nix
index 251e0460dd03..35f5853e3f66 100644
--- a/nixos/lib/utils.nix
+++ b/nixos/lib/utils.nix
@@ -219,14 +219,14 @@ let
               let
                 escapedName = ''"${replaceStrings [ ''"'' "\\" ] [ ''\"'' "\\\\" ] name}"'';
               in
-              recurse (prefix + "." + escapedName) item.${name}
+              recurse (prefix + (if prefix == "." then "" else ".") + escapedName) item.${name}
             ) (attrNames item)
           else if isList item then
             imap0 (index: item: recurse (prefix + "[${toString index}]") item) item
           else
             [ ];
       in
-      listToAttrs (flatten (recurse "" item));
+      listToAttrs (flatten (recurse "." item));
 
     /*
       Takes an attrset and a file path and generates a bash snippet that

Some test cases that will broken under the original implementation:

  1. recursiveGetAttrsetWithJqPrefix [
      {
        example = [
          {
            irrelevant = "not interesting";
          }
          {
            ignored = "ignored attr";
            relevant = {
              secret = {
                _secret = "/path/to/secret";
                quote = true;
              };
            };
          }
        ];
      }
    ] "_secret"
  2. recursiveGetAttrsetWithJqPrefix {
      secret = {
        _secret = "/path/to/secret";
        quote = true;
      };
    } "_secret"

Co-authored-by: linyinfeng <lin.yinfeng@outlook.com>
@Prince213 Prince213 changed the title utils.genJqSecretsReplacementSnippet: support top level secret recursiveGetAttrsetWithJqPrefix: fix top level values Aug 16, 2025
@Prince213 Prince213 requested a review from jian-lin August 16, 2025 15:32
Copy link
Contributor

@jian-lin jian-lin left a comment

Choose a reason for hiding this comment

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

Nice catch! @linyinfeng

Also thanks @Prince213 for this PR.

@jian-lin jian-lin merged commit 319663d into NixOS:master Aug 18, 2025
28 of 30 checks passed
@Prince213 Prince213 deleted the push-rvnmkknmmznx branch November 25, 2025 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants