diff --git a/sjsonnet/src/sjsonnet/Evaluator.scala b/sjsonnet/src/sjsonnet/Evaluator.scala index 4ca52e60..489d24a6 100644 --- a/sjsonnet/src/sjsonnet/Evaluator.scala +++ b/sjsonnet/src/sjsonnet/Evaluator.scala @@ -1213,11 +1213,20 @@ class Evaluator( } } + // DO NOT CHANGE: `self` MUST be passed explicitly to `sup.value(...)`. + // WHY: `Val.Obj.value`'s `self` parameter defaults to `this` (= the parent + // `sup`). Omitting `self` here would bind `$`/`self` of the retrieved field + // to the parent in the merge chain, hiding fields contributed by mixins + // applied *after* the parent — see issue #829 (grafana/mimir's + // `overrideSuperIfExists` pattern: `super[name] + override`). This must + // mirror `visitSelectSuper` (`super.name`) so static-key and computed-key + // super lookups agree, matching google/jsonnet, go-jsonnet, and jrsonnet. def visitLookupSuper(e: LookupSuper)(implicit scope: ValScope): Val = { var sup = scope.bindings(e.selfIdx + 1).asInstanceOf[Val.Obj] val key = visitExpr(e.index).cast[Val.Str] - if (sup == null) sup = scope.bindings(e.selfIdx).asInstanceOf[Val.Obj] - sup.value(key.str, e.pos) + val self = scope.bindings(e.selfIdx).asInstanceOf[Val.Obj] + if (sup == null) sup = self + sup.value(key.str, e.pos, self) } def visitImportStr(e: ImportStr): Val.Str = { diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_dollar_binding.jsonnet b/sjsonnet/test/resources/new_test_suite/super_lookup_dollar_binding.jsonnet new file mode 100644 index 00000000..5e82682a --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_dollar_binding.jsonnet @@ -0,0 +1,99 @@ +// Regression test for https://github.com/databricks/sjsonnet/issues/829 +// +// `super[name]` (computed-key super lookup) must bind `$`/`self` of the +// retrieved field to the *outer* self (the merge result), NOT the parent. +// Otherwise fields defined in mixins applied AFTER the parent become +// unreachable from inside fields retrieved through `super[name]`. +// +// This mirrors the semantics of `super.name` (static-key super lookup) and +// matches behavior of google/jsonnet (C++), go-jsonnet, and jrsonnet. +// +// The bug surfaced via grafana/mimir's `overrideSuperIfExists(name, override)` +// helper which uses `super[name] + override` to layer mixins. + +// ============================================================ +// SECTION 1: Static vs dynamic super key parity for $ binding +// ============================================================ + +// 1.1 Computed-key super lookup must see fields from later mixins via $ +local a1 = { + _config+:: { a_enabled: false }, + field: { flag: $._config.b_enabled }, +}; +local b1 = { + _config+:: { b_enabled: false }, + field: super['field'] + { extra: 1 }, +}; +local r1 = (a1 + b1).field; +assert r1 == { flag: false, extra: 1 } : 'super[name] must preserve $: ' + std.toString(r1); + +// 1.2 Static-key super lookup baseline (was already correct in sjsonnet) +local a2 = { + _config+:: { a_enabled: false }, + field: { flag: $._config.b_enabled }, +}; +local b2 = { + _config+:: { b_enabled: false }, + field: super.field + { extra: 1 }, +}; +local r2 = (a2 + b2).field; +assert r2 == r1 : 'super.name and super[name] must agree: ' + std.toString(r2); + +// ============================================================ +// SECTION 2: The grafana/mimir overrideSuperIfExists pattern +// ============================================================ + +local mixinA = { + _config+:: { a_only: 'a' }, + // Field references _config field that only exists after merging mixinB. + alertmanager_statefulset: { + label: $._config.b_only, + }, +}; +local mixinB = { + _config+:: { b_only: 'b' }, + local overrideSuperIfExists(name, override) = + if !(name in super) || super[name] == null || super[name] == {} then null + else super[name] + override, + alertmanager_statefulset: overrideSuperIfExists( + 'alertmanager_statefulset', + { extra: 'mixed-in' }, + ), +}; +local merged = mixinA + mixinB; +assert merged.alertmanager_statefulset == { label: 'b', extra: 'mixed-in' } + : 'overrideSuperIfExists must see merged $._config: ' + + std.toString(merged.alertmanager_statefulset); + +// ============================================================ +// SECTION 3: Edge cases +// ============================================================ + +// 3.1 super[name] with no override (just rebinding) preserves $ +local n1 = { _config+:: { x: 1 }, value: $._config.x }; +local n2 = { value: super['value'] }; +local r5 = (n1 + n2).value; +assert r5 == 1 : 'bare super[name] must preserve binding'; + +// 3.2 Chained mixins: 3 levels deep, $ must reach the outermost +local l1 = { _config+:: { l1: 1 }, item: { sum: $._config.l1 + $._config.l2 + $._config.l3 } }; +local l2 = { _config+:: { l2: 2 }, item: super['item'] + { mid: true } }; +local l3 = { _config+:: { l3: 3 }, item: super['item'] + { top: true } }; +local r6 = (l1 + l2 + l3).item; +assert r6 == { sum: 6, mid: true, top: true } + : 'chained super[name] across 3 mixins: ' + std.toString(r6); + +// 3.3 Computed key from a variable (not a literal string) +local k = 'item'; +local v1 = { _config+:: { x: 'hello' }, item: { val: $._config.x } }; +local v2 = { item: super[k] + { wrapped: true } }; +local r7 = (v1 + v2).item; +assert r7 == { val: 'hello', wrapped: true } + : 'super[var-key] must preserve binding: ' + std.toString(r7); + +// 3.4 Object-extension syntax {a + b} also exercises super lookup +local oa = { _config+:: { y: 'yes' }, label: $._config.y }; +local ob = oa { label: super['label'] }; +assert ob.label == 'yes' : 'super[name] in object-extension must preserve binding'; + +true diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_dollar_binding.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/super_lookup_dollar_binding.jsonnet.golden new file mode 100644 index 00000000..7eead1ee --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_dollar_binding.jsonnet.golden @@ -0,0 +1,2 @@ +true + diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_autoscaling.libsonnet b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_autoscaling.libsonnet new file mode 100644 index 00000000..f2abc322 --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_autoscaling.libsonnet @@ -0,0 +1,9 @@ +// Mimics grafana/mimir's ingest-storage-ingester-autoscaling.libsonnet — +// adds the `_config` field that the rollout mixin's fields reference via $. +// The whole point of the bug: this mixin is applied LAST, so super[name] +// inside memberlist must still see this field through $._config. +{ + _config+:: { + ingest_storage_ingester_autoscaling_enabled: false, + }, +} diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_memberlist.libsonnet b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_memberlist.libsonnet new file mode 100644 index 00000000..8174996a --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_memberlist.libsonnet @@ -0,0 +1,12 @@ +// Mimics grafana/mimir's memberlist.libsonnet — uses the +// `super[name] + override` pattern through the local helper. This is the +// site that triggered issue #829. +{ + // Verbatim pattern from operations/mimir/memberlist.libsonnet:171. + local overrideSuperIfExists(name, override) = + if !(name in super) || super[name] == null || super[name] == {} then null + else super[name] + override, + + alertmanager_statefulset: + overrideSuperIfExists('alertmanager_statefulset', { gossip: true }), +} diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_pattern.jsonnet b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_pattern.jsonnet new file mode 100644 index 00000000..e259fa3d --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_pattern.jsonnet @@ -0,0 +1,29 @@ +// Regression test for https://github.com/databricks/sjsonnet/issues/829 +// +// End-to-end multi-file reproduction of the grafana/mimir failure: +// +// sjsonnet.Error: Field does not exist: ingest_storage_ingester_autoscaling_enabled +// at [overrideSuperIfExists].(mimir/rollout-operator.libsonnet:18:64) +// at [].(mimir/memberlist.libsonnet:106:50) +// +// Faithfully mirrors the structure: three libsonnet mixins merged with `+`, +// where the last mixin's `_config` adds the field that an earlier mixin's +// field references through `super[name] + override`. + +local rollout = import 'super_lookup_mimir_rollout.libsonnet'; +local memberlist = import 'super_lookup_mimir_memberlist.libsonnet'; +local autoscaling = import 'super_lookup_mimir_autoscaling.libsonnet'; + +local merged = rollout + memberlist + autoscaling; + +// Without the fix this throws: +// Field does not exist: ingest_storage_ingester_autoscaling_enabled +local rt = merged.rollout_operator_replica_template_access_enabled; +assert rt == false : 'rollout enabled flag must evaluate via $._config across mixins'; + +// memberlist's overrideSuperIfExists must surface the rebuilt statefulset +local sts = merged.alertmanager_statefulset; +assert sts.label == 'autoscaling-disabled' : 'super[name] must rebind $: ' + std.toString(sts); +assert sts.gossip == true : 'override must merge on top of super[name]: ' + std.toString(sts); + +true diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_pattern.jsonnet.golden b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_pattern.jsonnet.golden new file mode 100644 index 00000000..7eead1ee --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_pattern.jsonnet.golden @@ -0,0 +1,2 @@ +true + diff --git a/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_rollout.libsonnet b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_rollout.libsonnet new file mode 100644 index 00000000..de1a54dd --- /dev/null +++ b/sjsonnet/test/resources/new_test_suite/super_lookup_mimir_rollout.libsonnet @@ -0,0 +1,22 @@ +// Mimics grafana/mimir's rollout-operator.libsonnet — defines a field that +// references _config keys defined in a later mixin. +{ + _config+:: { + multi_zone_ingester_enabled: false, + multi_zone_store_gateway_enabled: false, + cortex_compactor_concurrent_rollout_enabled: false, + }, + + // Line equivalent to rollout-operator.libsonnet:18 — references + // ingest_storage_ingester_autoscaling_enabled which is added by the + // autoscaling mixin appended LATER in the merge chain. + rollout_operator_replica_template_access_enabled: + $._config.ingest_storage_ingester_autoscaling_enabled, + + alertmanager_statefulset: { + label: + if $._config.ingest_storage_ingester_autoscaling_enabled + then 'autoscaling-enabled' + else 'autoscaling-disabled', + }, +}