Conversation
Collaborator
Integration test reportCommit: ee2d517
Top 3 slowest tests (at least 2 minutes):
|
Keyed slices are matched by key value alone (the key field is only used to render the path). A diff on a matched element's own key field therefore means the two sides carry the same identity under a different field — e.g. a permission declared under user_name that the Permissions API stores and returns as service_principal_name — which is not a real change. Drop those field diffs; non-key fields still diff normally. This fixes a perpetual no-op "update" of dashboard/job/etc permissions when a service principal is declared under user_name, generically for any keyed slice, without per-resource logic. Alternative to the resource-level fix in #6710. The testserver models the backend's user_name(UUID) -> service_principal_name readback so the case reproduces locally. Co-authored-by: Isaac <no-reply@databricks.com>
Keyed-slice elements are matched by value, so encoding the key field in the path is redundant — and for a permission it is actively wrong, because the backend may return a principal under a different field (a user_name holding a service principal's application ID comes back as service_principal_name), producing a perpetual no-op "update". Introduce libs/structs/registry: a declarative, type-keyed record of which fields identify a slice element (registered from init() next to each type). structdiff keys off it instead of per-call KeyFunc maps and addresses elements as [='value'] (key field omitted); a diff on a matched element's own key field is dropped, so the field difference is no longer a change. structaccess and configsync resolve [='value'] back to an element through the registry (the type for structaccess, the element's members for configsync's dynamic values). This removes the KeyFunc machinery and the adapter's KeyedSlices method, and collapses the job path patterns into nine type registrations. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
denik
force-pushed
the
denik/keyed-path-drop-field
branch
from
September 21, 2026 08:26
f8c7ac1 to
6e06655
Compare
Address review findings on the field-agnostic ([='value']) keyed paths: - configsync resolved a keyed element's key field from a global set of key-field names, which is ambiguous — a job task carries both task_key and a job_cluster_key reference, so the name-based guess picked the wrong field and broke selector resolution and rename detection. Resolve the element's key fields from the sequence's Go type instead (structaccess.TypeAtPath + registry.KeyFields), which is exact. - structaccess now resolves [='value'] via registry.ElementKey, the same identity the diff uses, instead of matching any key field (which could pick a different element). - registry: resolve a key field's index breadth-first so an outer field shadowing an embedded one wins (matching encoding/json); guard nil pointer embeds and nil elements against a FieldByIndex panic; drop the now-unused IsKeyField/ElementHasValue. Co-authored-by: Isaac <no-reply@databricks.com>
…key field Register now dereferences pointer types before storing, so Register[*T] matches lookups by T. stringFieldIndex resolves the dominant JSON field (the one encoding/json serializes) first and requires it to be a string, so a non-string outer field that shadows an embedded string is correctly rejected rather than silently keying on the hidden field. Co-authored-by: Isaac <no-reply@databricks.com>
A later review kept surfacing edge cases in the registry's hand-rolled JSON field resolver (implicit names, same-depth ambiguity, unexported anonymous embeds, embed cycles). Rather than reimplement encoding/json field resolution, reduce the registry to what it uniquely knows — the key field names per type — and read a key field's value through structaccess (ElementKeyValue), the codebase's canonical resolver used everywhere else. structdiff and structaccess resolve an element's key value through it, so behavior matches encoding/json and stays consistent, and the registry has no reflection edge cases of its own. Co-authored-by: Isaac <no-reply@databricks.com>
A keyed-slice element with no key field set now resolves to the empty key ("")
rather than being unaddressable: ElementKeyValue and dynElementKey return ok for
any resolvable element and "" when no key field is set, so an emitted [=''] path
resolves back to that element (matching the old [field=''] behavior) instead of
failing with "no array element found".
Co-authored-by: Isaac <no-reply@databricks.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Keyed-slice elements (job tasks, permissions, grants, secret ACLs, …) are matched by value, so encoding the key field in the change path is redundant. For permissions it is also wrong: the Permissions API may return a principal under a different field than configured (a
user_nameholding a service principal's application ID comes back asservice_principal_name), which surfaced as a perpetual no-opupdateof the ACL on every plan/deploy.Approach
Introduce
libs/structs/registry— a declarative, type-keyed record of which fields identify a keyed-slice element, registered frominit()next to each type:structdiffkeys off the registry by element type (no more per-callKeyFuncmaps) and addresses an element as[='value']— the key field is omitted. A diff on a matched element's own key field is dropped, so a principal carried under a different field is no longer reported as a change.structaccessresolves[='value']back to an element via the registry (it has the Go type).configsyncresolves it for dynamic values by recognising the element's key field withregistry.IsKeyField, so remote-drift write-back stays correct.KeyFuncmachinery and the adapter'sKeyedSlicesmethod; the job path-pattern map collapses to nine type registrations.This is the generic-diff line of the permissions false-positive fix (cf. #6710 hook / #6732 side-key merge): here the key field simply leaves the path, and the resolvers recover it from type/registry.
Validation
New
registryunit tests;structpathround-trips[='value']. All keyed-slice acceptance goldens regenerated ([task_key='x']→[='x']). Unit tests acrosslibs/structs,bundle/direct,configsync,terraform_dabs_map; acceptanceinvariantno_drift and fullconfig-remote-syncpass — the config-remote-sync write-back edits the correct element. gofmt + lint clean.This pull request and its description were written by Isaac.