From 87769011699976e788e56acd09cc7be988778fe6 Mon Sep 17 00:00:00 2001 From: Sheridan C Rawlins Date: Thu, 21 Nov 2024 15:59:38 -0800 Subject: [PATCH 1/6] Older devices are sending "gpc": 1 and this can't marshal. --- openrtb_ext/request_wrapper.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/openrtb_ext/request_wrapper.go b/openrtb_ext/request_wrapper.go index cb60948768..c593038dfd 100644 --- a/openrtb_ext/request_wrapper.go +++ b/openrtb_ext/request_wrapper.go @@ -9,6 +9,7 @@ import ( "github.com/prebid/openrtb/v20/openrtb2" "github.com/prebid/prebid-server/v3/util/jsonutil" "github.com/prebid/prebid-server/v3/util/ptrutil" + "github.com/tidwall/gjson" ) // RequestWrapper wraps the OpenRTB request to provide a storage location for unmarshalled ext fields, so they @@ -1278,8 +1279,10 @@ func (re *RegExt) unmarshal(extJson json.RawMessage) error { gpcJson, hasGPC := re.ext[gpcKey] if hasGPC && gpcJson != nil { - if err := jsonutil.Unmarshal(gpcJson, &re.gpc); err != nil { - return err + gpcResult := gjson.ParseBytes(gpcJson) + if gpcResult.Exists() { + gpc := gpcResult.String() + re.gpc = &gpc } } From 639a1cf5311163bf5fce68d98b4ce24d9b642e41 Mon Sep 17 00:00:00 2001 From: Sheridan C Rawlins Date: Thu, 21 Nov 2024 16:07:51 -0800 Subject: [PATCH 2/6] Added test cases. --- openrtb_ext/request_wrapper_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/openrtb_ext/request_wrapper_test.go b/openrtb_ext/request_wrapper_test.go index 73851238d0..f574ee24e8 100644 --- a/openrtb_ext/request_wrapper_test.go +++ b/openrtb_ext/request_wrapper_test.go @@ -2344,6 +2344,20 @@ func TestRegExtUnmarshal(t *testing.T) { expectGPC: ptrutil.ToPtr("some_value"), expectError: false, }, + { + name: `valid_gpc_json "1"`, + regExt: &RegExt{}, + extJson: json.RawMessage(`{"gpc": "1"}`), + expectGPC: ptrutil.ToPtr("1"), + expectError: false, + }, + { + name: `valid_gpc_json 1`, + regExt: &RegExt{}, + extJson: json.RawMessage(`{"gpc": 1}`), + expectGPC: ptrutil.ToPtr("1"), + expectError: false, + }, { name: "malformed_gpc_json", regExt: &RegExt{}, From 4af0b375ff9329d44b4371cb37cc213e189dca38 Mon Sep 17 00:00:00 2001 From: Sheridan C Rawlins Date: Fri, 22 Nov 2024 08:39:44 -0800 Subject: [PATCH 3/6] Address SyntaxNode comments. --- openrtb_ext/request_wrapper_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openrtb_ext/request_wrapper_test.go b/openrtb_ext/request_wrapper_test.go index f574ee24e8..e4c14b6031 100644 --- a/openrtb_ext/request_wrapper_test.go +++ b/openrtb_ext/request_wrapper_test.go @@ -2345,14 +2345,14 @@ func TestRegExtUnmarshal(t *testing.T) { expectError: false, }, { - name: `valid_gpc_json "1"`, + name: `valid_gpc_json_"1"`, regExt: &RegExt{}, extJson: json.RawMessage(`{"gpc": "1"}`), expectGPC: ptrutil.ToPtr("1"), expectError: false, }, { - name: `valid_gpc_json 1`, + name: `valid_gpc_json_1`, regExt: &RegExt{}, extJson: json.RawMessage(`{"gpc": 1}`), expectGPC: ptrutil.ToPtr("1"), @@ -2391,6 +2391,7 @@ func TestRegExtUnmarshal(t *testing.T) { assert.Equal(t, tt.expectDSA, tt.regExt.dsa) assert.Equal(t, tt.expectGDPR, tt.regExt.gdpr) assert.Equal(t, tt.expectUSPrivacy, tt.regExt.usPrivacy) + assert.Equal(t, tt.expectGPC, tt.regExt.gpc) }) } } From 5c8d751d6f3e91f76b799a58b26218d065e7d203 Mon Sep 17 00:00:00 2001 From: Sheridan C Rawlins Date: Mon, 2 Dec 2024 08:40:44 -0800 Subject: [PATCH 4/6] Move use of gjson into jsonutil. --- go.mod | 2 +- openrtb_ext/request_wrapper.go | 7 +--- util/jsonutil/forcestring.go | 15 ++++++++ util/jsonutil/forcestring_test.go | 57 +++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 util/jsonutil/forcestring.go create mode 100644 util/jsonutil/forcestring_test.go diff --git a/go.mod b/go.mod index e844fe5bf2..7f672c0f20 100644 --- a/go.mod +++ b/go.mod @@ -34,6 +34,7 @@ require ( github.com/rs/cors v1.11.0 github.com/spf13/viper v1.12.0 github.com/stretchr/testify v1.8.1 + github.com/tidwall/gjson v1.17.1 github.com/vrischmann/go-metrics-influxdb v0.1.1 github.com/xeipuuv/gojsonschema v1.2.0 github.com/yudai/gojsondiff v1.0.0 @@ -70,7 +71,6 @@ require ( github.com/spf13/pflag v1.0.5 // indirect github.com/stretchr/objx v0.5.0 // indirect github.com/subosito/gotenv v1.3.0 // indirect - github.com/tidwall/gjson v1.17.1 // indirect github.com/tidwall/match v1.1.1 // indirect github.com/tidwall/pretty v1.2.0 // indirect github.com/tidwall/sjson v1.2.5 // indirect diff --git a/openrtb_ext/request_wrapper.go b/openrtb_ext/request_wrapper.go index c593038dfd..4a38691c23 100644 --- a/openrtb_ext/request_wrapper.go +++ b/openrtb_ext/request_wrapper.go @@ -9,7 +9,6 @@ import ( "github.com/prebid/openrtb/v20/openrtb2" "github.com/prebid/prebid-server/v3/util/jsonutil" "github.com/prebid/prebid-server/v3/util/ptrutil" - "github.com/tidwall/gjson" ) // RequestWrapper wraps the OpenRTB request to provide a storage location for unmarshalled ext fields, so they @@ -1279,11 +1278,7 @@ func (re *RegExt) unmarshal(extJson json.RawMessage) error { gpcJson, hasGPC := re.ext[gpcKey] if hasGPC && gpcJson != nil { - gpcResult := gjson.ParseBytes(gpcJson) - if gpcResult.Exists() { - gpc := gpcResult.String() - re.gpc = &gpc - } + jsonutil.ParseIntoString(gpcJson, &re.gpc) } return nil diff --git a/util/jsonutil/forcestring.go b/util/jsonutil/forcestring.go new file mode 100644 index 0000000000..88bdc53627 --- /dev/null +++ b/util/jsonutil/forcestring.go @@ -0,0 +1,15 @@ +package jsonutil + +import "github.com/tidwall/gjson" + +// ParseIntoString Parse json bytes into a string pointer +func ParseIntoString(b []byte, ppString **string) { + if ppString == nil { + panic("ppString is nil") + } + result := gjson.ParseBytes(b) + if result.Exists() && result.Raw != `null` { + *ppString = new(string) + **ppString = result.String() + } +} diff --git a/util/jsonutil/forcestring_test.go b/util/jsonutil/forcestring_test.go new file mode 100644 index 0000000000..c0bccf8dd6 --- /dev/null +++ b/util/jsonutil/forcestring_test.go @@ -0,0 +1,57 @@ +package jsonutil + +import ( + "testing" + + "github.com/prebid/prebid-server/v3/util/ptrutil" + "github.com/stretchr/testify/assert" +) + +func Test_ParseIntoString(t *testing.T) { + tests := []struct { + name string + b []byte + want *string + }{ + { + name: "empty", + }, + { + name: "quoted_1", + b: []byte(`"1"`), + want: ptrutil.ToPtr("1"), + }, + { + name: "unquoted_1", + b: []byte(`1`), + want: ptrutil.ToPtr("1"), + }, + { + name: "null", + b: []byte(`null`), + }, + { + name: "quoted_null", + b: []byte(`"null"`), + want: ptrutil.ToPtr("null"), + }, + { + name: "quoted_hello", + b: []byte(`"hello"`), + want: ptrutil.ToPtr("hello"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got *string + ParseIntoString(tt.b, &got) + assert.Equal(t, tt.want, got) + }) + } +} + +func Test_ParseIntoStringPanic(t *testing.T) { + assert.Panics(t, func() { + ParseIntoString([]byte(`"123"`), nil) + }) +} From 61c786d18ec54b77bcfa154ca66133c8ceeadbe7 Mon Sep 17 00:00:00 2001 From: Sheridan C Rawlins Date: Mon, 2 Dec 2024 08:43:53 -0800 Subject: [PATCH 5/6] Use ptrutil in jsonutil too. --- util/jsonutil/forcestring.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/util/jsonutil/forcestring.go b/util/jsonutil/forcestring.go index 88bdc53627..db5739a7b9 100644 --- a/util/jsonutil/forcestring.go +++ b/util/jsonutil/forcestring.go @@ -1,6 +1,9 @@ package jsonutil -import "github.com/tidwall/gjson" +import ( + "github.com/prebid/prebid-server/v3/util/ptrutil" + "github.com/tidwall/gjson" +) // ParseIntoString Parse json bytes into a string pointer func ParseIntoString(b []byte, ppString **string) { @@ -9,7 +12,6 @@ func ParseIntoString(b []byte, ppString **string) { } result := gjson.ParseBytes(b) if result.Exists() && result.Raw != `null` { - *ppString = new(string) - **ppString = result.String() + *ppString = ptrutil.ToPtr(result.String()) } } From 8444c5c1c626cd95d6ec228028e1ca4e9f6eef9e Mon Sep 17 00:00:00 2001 From: Sheridan C Rawlins Date: Wed, 11 Dec 2024 15:32:44 -0800 Subject: [PATCH 6/6] Don't panic; return an error. --- openrtb_ext/request_wrapper.go | 2 +- util/jsonutil/forcestring.go | 7 +++++-- util/jsonutil/forcestring_test.go | 20 ++++++++++++-------- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/openrtb_ext/request_wrapper.go b/openrtb_ext/request_wrapper.go index 4a38691c23..7b73d31867 100644 --- a/openrtb_ext/request_wrapper.go +++ b/openrtb_ext/request_wrapper.go @@ -1278,7 +1278,7 @@ func (re *RegExt) unmarshal(extJson json.RawMessage) error { gpcJson, hasGPC := re.ext[gpcKey] if hasGPC && gpcJson != nil { - jsonutil.ParseIntoString(gpcJson, &re.gpc) + return jsonutil.ParseIntoString(gpcJson, &re.gpc) } return nil diff --git a/util/jsonutil/forcestring.go b/util/jsonutil/forcestring.go index db5739a7b9..64ce2a6795 100644 --- a/util/jsonutil/forcestring.go +++ b/util/jsonutil/forcestring.go @@ -1,17 +1,20 @@ package jsonutil import ( + "errors" + "github.com/prebid/prebid-server/v3/util/ptrutil" "github.com/tidwall/gjson" ) // ParseIntoString Parse json bytes into a string pointer -func ParseIntoString(b []byte, ppString **string) { +func ParseIntoString(b []byte, ppString **string) error { if ppString == nil { - panic("ppString is nil") + return errors.New("ppString is nil") } result := gjson.ParseBytes(b) if result.Exists() && result.Raw != `null` { *ppString = ptrutil.ToPtr(result.String()) } + return nil } diff --git a/util/jsonutil/forcestring_test.go b/util/jsonutil/forcestring_test.go index c0bccf8dd6..60f87fafa4 100644 --- a/util/jsonutil/forcestring_test.go +++ b/util/jsonutil/forcestring_test.go @@ -9,9 +9,10 @@ import ( func Test_ParseIntoString(t *testing.T) { tests := []struct { - name string - b []byte - want *string + name string + b []byte + want *string + wantErr bool }{ { name: "empty", @@ -44,14 +45,17 @@ func Test_ParseIntoString(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var got *string - ParseIntoString(tt.b, &got) + err := ParseIntoString(tt.b, &got) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) assert.Equal(t, tt.want, got) }) } } -func Test_ParseIntoStringPanic(t *testing.T) { - assert.Panics(t, func() { - ParseIntoString([]byte(`"123"`), nil) - }) +func Test_ParseIntoNilStringError(t *testing.T) { + assert.Error(t, ParseIntoString([]byte(`"123"`), nil)) }