structaccess: two ForceSendFields fixes in Set - #6466
Merged
Conversation
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
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
denik
marked this pull request as ready for review
September 1, 2026 19:20
denik
enabled auto-merge
September 1, 2026 19:22
pietern
approved these changes
Sep 2, 2026
| err := updateForceSendFields(parentVal, sf.Name, embeddedIndex, valueVal, sf) | ||
| // 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. |
Contributor
There was a problem hiding this comment.
Comment referring to previous state is irrelevant when you read it post-merge.
Remove the phrase explaining the historical pre-fix state. The "assign first" comment already says what matters.
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.
Two independent bugs in
Set:""recorded it as force-send and then failed."0"was stored as zero and left out of ForceSendFields — marshalled as absent. Latent today, since no value library writes a number as a quoted string.Both tests fail without the fix.
This pull request and its description were written by Isaac.