From a53636bcf619b7b56cf2886229661918a7f94eda Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:10:20 +0200 Subject: [PATCH 1/3] structaccess: assign before updating ForceSendFields Set updated ForceSendFields before assigning, so a value that could not be converted left the field's send-behaviour changed even though the Set failed: setting an omitempty numeric field to "" recorded the field as force-send and then failed the conversion. The assignment goes first. Co-authored-by: Isaac --- libs/structs/structaccess/set.go | 10 ++++++---- libs/structs/structaccess/set_test.go | 9 +++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/libs/structs/structaccess/set.go b/libs/structs/structaccess/set.go index 2285d470fa9..05eb864d50d 100644 --- a/libs/structs/structaccess/set.go +++ b/libs/structs/structaccess/set.go @@ -140,13 +140,15 @@ func setStructField(parentVal reflect.Value, fieldName string, valueVal reflect. return fmt.Errorf("field %q cannot be set", sf.Name) } - // Handle ForceSendFields: remove if setting nil, add if setting empty value - err := updateForceSendFields(parentVal, sf.Name, embeddedIndex, valueVal, sf) - if err != nil { + // Assign first: a value that cannot be converted must leave the struct exactly as it was, + // and updating ForceSendFields before the assignment left the field's send-behaviour + // changed after a failed Set. + if err := assignValue(fv, valueVal); err != nil { return err } - return assignValue(fv, valueVal) + // Handle ForceSendFields: remove if setting nil, add if setting empty value + return updateForceSendFields(parentVal, sf.Name, embeddedIndex, valueVal, sf) } // setMapValue sets a value in a map diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 1b294494eca..433dd54f156 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -791,3 +791,12 @@ func TestSet_MixedForceSendFields(t *testing.T) { assert.Equal(t, []string{"SecondFieldOmit"}, obj.Second.ForceSendFields) // no duplicates }) } + +// A value that cannot be converted must leave the struct untouched, ForceSendFields included. +func TestSet_FailedConversionLeavesForceSendFields(t *testing.T) { + job := &jobs.JobSettings{Name: "n"} //exhaustruct:ignore + + require.Error(t, structaccess.SetByString(job, "max_concurrent_runs", "")) + assert.Empty(t, job.ForceSendFields) + assert.Equal(t, "n", job.Name) +} From 3fb73a732239e418954cb3e1512542c84080082c Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Tue, 1 Sep 2026 17:11:19 +0200 Subject: [PATCH 2/3] structaccess: decide ForceSendFields from the value actually stored Set converted the caller's value on the way in but judged emptiness from the value it was handed, so an omitempty numeric or bool field set from a string that converts to zero was stored as zero and then left out of ForceSendFields -- and marshalled as absent. Setting max_concurrent_runs to "0" silently meant unset. Latent today, since no value library writes a number as a quoted string, but it contradicts the contract callers rely on: a scalar is left to structaccess to convert between the numeric and string kinds. Co-authored-by: Isaac --- libs/structs/structaccess/set.go | 17 ++++++++++++++--- libs/structs/structaccess/set_test.go | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/libs/structs/structaccess/set.go b/libs/structs/structaccess/set.go index 05eb864d50d..ef5e2a0546d 100644 --- a/libs/structs/structaccess/set.go +++ b/libs/structs/structaccess/set.go @@ -143,12 +143,23 @@ func setStructField(parentVal reflect.Value, fieldName string, valueVal reflect. // Assign first: a value that cannot be converted must leave the struct exactly as it was, // and updating ForceSendFields before the assignment left the field's send-behaviour // changed after a failed Set. - if err := assignValue(fv, valueVal); err != nil { + converted, err := convertValue(valueVal, fv.Type()) + if err != nil { + return err + } + if err := assignValue(fv, converted); err != nil { return err } - // Handle ForceSendFields: remove if setting nil, add if setting empty value - return updateForceSendFields(parentVal, sf.Name, embeddedIndex, valueVal, sf) + // ForceSendFields is decided from the converted value, not the caller's: setting an + // omitempty int64 from the string "0" stores 0, which is empty and must be forced, while + // the string "0" is not empty and would have left the field to be omitted. + if !valueVal.IsValid() { + // Setting nil: the field is being made absent, which convertValue renders as the zero + // value. Pass the invalid value through so it is removed from ForceSendFields. + return updateForceSendFields(parentVal, sf.Name, embeddedIndex, valueVal, sf) + } + return updateForceSendFields(parentVal, sf.Name, embeddedIndex, converted, sf) } // setMapValue sets a value in a map diff --git a/libs/structs/structaccess/set_test.go b/libs/structs/structaccess/set_test.go index 433dd54f156..9144c61c3cd 100644 --- a/libs/structs/structaccess/set_test.go +++ b/libs/structs/structaccess/set_test.go @@ -1,6 +1,7 @@ package structaccess_test import ( + "encoding/json" "testing" "github.com/databricks/cli/libs/structs/structaccess" @@ -800,3 +801,18 @@ func TestSet_FailedConversionLeavesForceSendFields(t *testing.T) { assert.Empty(t, job.ForceSendFields) assert.Equal(t, "n", job.Name) } + +// ForceSendFields is decided from the value actually stored, not the one the caller passed: +// setting an omitempty numeric field from the string "0" stores zero, which has to be forced +// or the field marshals as absent. +func TestSet_StringZeroIntoOmitemptyNumberIsForced(t *testing.T) { + job := &jobs.JobSettings{Name: "n"} //exhaustruct:ignore + + require.NoError(t, structaccess.SetByString(job, "max_concurrent_runs", "0")) + assert.Equal(t, 0, job.MaxConcurrentRuns) + assert.Contains(t, job.ForceSendFields, "MaxConcurrentRuns") + + blob, err := json.Marshal(job) + require.NoError(t, err) + assert.Contains(t, string(blob), `"max_concurrent_runs":0`) +} From 11c6c58135ff4c49bbdb8ff545e69b43555035d2 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Wed, 2 Sep 2026 08:11:42 +0200 Subject: [PATCH 3/3] structaccess: trim redundant comment Remove the phrase explaining the historical pre-fix state. The "assign first" comment already says what matters. --- libs/structs/structaccess/set.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libs/structs/structaccess/set.go b/libs/structs/structaccess/set.go index ef5e2a0546d..e6b84ad78a5 100644 --- a/libs/structs/structaccess/set.go +++ b/libs/structs/structaccess/set.go @@ -140,9 +140,7 @@ func setStructField(parentVal reflect.Value, fieldName string, valueVal reflect. return fmt.Errorf("field %q cannot be set", sf.Name) } - // Assign first: a value that cannot be converted must leave the struct exactly as it was, - // and updating ForceSendFields before the assignment left the field's send-behaviour - // changed after a failed Set. + // Assign first: a value that cannot be converted must leave the struct exactly as it was. converted, err := convertValue(valueVal, fv.Type()) if err != nil { return err