Honor ForceSendFields for query parameters#1728
Open
renaudhartert-db wants to merge 1 commit into
Open
Conversation
4a08060 to
da251bd
Compare
da251bd to
c59ade0
Compare
Query parameters are serialized by go-querystring via the `url` struct tag, which applies `omitempty` but has no knowledge of ForceSendFields. As a result a zero-valued scalar field listed in ForceSendFields (for example a false `cascade` bool on the pipelines Delete request) was dropped instead of being sent on the wire, even though the same field would be honored in a JSON body. Re-add force-sent fields in makeQueryString after go-querystring runs, mirroring the basic-type-only ForceSendFields semantics already used by the marshal package for JSON bodies. The walk recurses into nested structs and non-nil pointers to structs so that each struct level honors its own ForceSendFields, using the same `parent[child]` key notation go-querystring emits. Pointer fields are otherwise left untouched (a non-nil pointer is already serialized and a nil pointer means "not set"), and values match go-querystring's formatting via fmt.Sprint. Co-authored-by: Isaac Signed-off-by: Ubuntu <renaud.hartert@databricks.com>
c59ade0 to
8cf3121
Compare
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
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
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.
Summary
Query parameters listed in
ForceSendFieldsare now sent on the wire even when they hold a zero value, matching the behavior that already exists for JSON request bodies. This fixes cases like the pipelines Delete API, where settingCascadetofalseand adding it toForceSendFieldsdid not actually sendcascade=falseon the wire.Why
Query parameters are serialized from the request struct's
urltag by the go-querystring library. That tag usesomitempty, so a zero-valued scalar field (for example afalsebool, a0int, or an empty string) is dropped from the URL.ForceSendFieldswas designed to opt out of exactly this behavior, but it was only ever wired into themarshalpackage, which handles JSON bodies. go-querystring has no knowledge ofForceSendFields, so adding a query field to it had no effect.(Path parameters are not affected: they are tagged
url:"-"and substituted directly into the URL path, so they never flow through this serialization.)A user reported this for the pipelines Delete API:
Cascadedefaults totrueserver-side, so a caller who wants to delete a pipeline without cascading must sendcascade=falseexplicitly. AddingCascadetoForceSendFieldslooked like the right way to do that (it is how the same situation is handled for JSON bodies), but the parameter was silently dropped and the server applied its default. This affected every query parameter across all services, not justcascade, including fields nested inside request sub-structs.How is this tested?
Added two table-driven tests in
httpclient/request_test.go.TestMakeQueryStringForceSendFieldscovers the top-level cases: zero values dropped withoutForceSendFields, afalsebool sent when forced, non-zero values still sent without forcing, multiple forced fields, forcing not overriding an explicitly set value, and unknown forced field names being ignored.TestMakeQueryStringForceSendFieldsNestedcovers nested sub-structs: a nested struct honoring its ownForceSendFields, non-nil and nil pointers to nested structs, and doubly nested fields. Also verified end-to-end by calling the real pipelines Delete andiam.CheckPolicyAPIs through a fake HTTP transport and confirming the forced zero values appear in the request URL. The fullhttpclienttest suite passes.