Issue 114

[askripe]

What

A #/key fragment could only pull string leaf values out of a YAML/JSON
secret. After #934 int and bool work too, but floating-point numbers, other
integer widths, and keys that point at a nested object or array still fail. This
makes all of them work, preserving their native types.

Fixes #114.

Why

The fragment traversal in vals.go decided what to return based on whether the
value was a "terminal" scalar, and isTerminalValue only counted bool, int
and string. As a result:

• a float64 (the usual JSON number) or a large integer fell through to
  unsupported type for key;
• a value that was itself a map or slice was never returned (it fell through to a
  nil / no value found for key), so you could not pull a sub-document out of a
  key.

Changes

All in the shared traversal, so every fragment-capable provider benefits
(s3, gcs, file, vault, ...), not just s3:

• isTerminalValue now accepts every scalar kind (float32/64, all sized
  int/uint), reusing the set already defined for nested refs in
  pkg/expansion/expand_match.go.
• The traversal returns the value at the final fragment key whatever its type
  (scalar, nested map, or slice).
• Descending through an intermediate map[interface{}]interface{} (which some
  providers produce) no longer silently fails (a shadowed-variable bug).
• A genuinely missing key is distinguished from a present null via a comma-ok
  lookup, so FailOnMissingKeyInMap is honored and a present null returns nil.

Example

s3://bucket/obj.json = {"key": 123, "ratio": 1.5, "obj": {"a": 1}, "list": ["x", "y"]}

ref	before	after	notes
...#/key (int)	123	123	already worked (#934)
...#/ratio (float)	unsupported type for key (float64)	1.5	this PR
...#/obj (object)	nil / no value found for key	{a: 1}	this PR
...#/list (array)	unsupported type for key	[x, y]	this PR; the reporter's original goal (#114)
...#/strkey (string)	worked	unchanged	already worked

Behavior change to note

A missing key previously surfaced as a confusing unsupported type for key ... <nil>. It now returns nil, or, with FailOnMissingKeyInMap, the clearer
no value found for key .... Existing string/int/bool extraction is unchanged.

Testing

New table-driven tests use the file provider (it shares s3's
GetStringMap / yaml.Unmarshal), so they need no cloud credentials, plus a
mock-provider test for the map[interface{}]interface{} path, and an extended
TestEvalNodesTypes covering the full CLI path. go test -race ./... is green

[yxxhero]

Review: PR #1197 "Issue 114"

Verdict: Approve. Solid fix for a long-standing bug (#114, open since 2021), with good test coverage and a clear writeup.

What it does

Rewrites the shared #/key fragment traversal in vals.go:481 so a fragment can extract any type — floats, wide ints, nested maps, arrays — not just string/int/bool. Lives in the shared path, so s3, gcs, file, vault, etc. all benefit.

Strengths

• isTerminalValue (vals.go:516) now covers all scalar kinds, matching the set already used in pkg/expansion/expand_match.go. Consistent.
• Genuine bug fixes found along the way (commit 2):
  • vals.go:494 — the old case map[interface{}]interface{} declared obj := inside the case, shadowing the outer variable, so it never actually descended. asStringKeyedMap returns the normalized map and assigns to the outer obj. Real fix.
  • t, found := obj[k] correctly distinguishes an absent key from a present YAML null. Previously a missing key with FailOnMissingKeyInMap=true hit the default branch and produced the confusing unsupported type ... <nil> error; the FailOnMissingKeyInMap check at the end of the loop was effectively unreachable. Now it fires correctly at any depth.
• Tests (vals_fragment_test.go) are table-driven, use the file provider (no AWS creds), and cover: int/float/object extraction, uint64 > MaxInt64, negative floats, nested object/array, intermediate map[interface{}]interface{} via mock provider, and missing-key-vs-null semantics. TestEvalNodesTypes extends the full CLI path. go test -race ./... is green locally.
• README change accurately reflects the new behavior.

Minor observations (non-blocking)

1. vals.go:508 return nil, nil after the loop is now unreachable — the loop always returns on the last index, on !found, or on a non-map intermediate. Harmless (kept the compiler happy), but dead code.
2. No test for a final value that is itself map[interface{}]interface{}. TestFragment_IntermediateInterfaceMap only descends through one to a string leaf. If a provider returns a nested map[interface{}]interface{} and the fragment points directly at it, it's returned as-is (not normalized to map[string]interface{}). This is consistent with the PR's "native type preserved" goal and yaml.Marshal handles it, but a test asserting the returned shape would lock in the contract.
3. Nested map/slice results aren't cached under key (the isTerminalValue guard before r.docCache.Add skips them). This matches the pre-PR behavior and the document is still cached under mapRequestURI, so the only cost is re-traversal — negligible. Noting for completeness.

Behavior change worth signaling to users

A missing key now returns nil (or errors with no value found for key ... under FailOnMissingKeyInMap) instead of the old unsupported type for key ... <nil>. The PR description calls this out. Existing string/int/bool extraction is unchanged.

Recommend merge.