JSON Serialization: Change Libraries #3225

bsardo · 2023-10-16T20:54:56Z

This PR attempts to improve serialization performance by swapping out the standard library encoding/json with the json-iterator library.

General notes about json-iterator:

the library claims 100% compatibility but falls short of this claim
most standard library public functions are provided with the same signature
for now, all that we care about are the marshal and unmarshal functions which have the same signature as the standard library

Notable differences between the standard library and json-iterator marshal/unmarshal functions:

json-iterator functions always return error strings instead of unmarshal, syntax and marshal error types
json-iterator functions do not validate json.RawMessage fields
when unmarshaling bytes to a data type containing a json.RawMessage field, if the field in the bytes has the value "null", the four byte value "null" is not written to the json.RawMessage field like it is in the standard library, but rather the value is considered nil and as a result the json.RawMessage field will be empty
when marshaling, if one of the fields being marshaled is of type json.RawMessage, it is not validated. Syntax errors are not reported like they are with the standard library.

Implementation:

Added the following jsonutil helper function that wrap json-iterator:
- UnmarshalValid (which unmarshals and performs validation)
- Unmarshal (which unmarshals without performing validation)
- Marshal (which marshals without performing validation)
Updated the AMP and auction flows to call the helper functions instead of making calls to the standard library Unmarshal and Marshal functions
Unmarshaling with validation is only performed when PBS core does not have control over the data source (e.g. a stored request has been loaded from a database, a module has manipulated the data by applying changesets).. and in tests.
json-iterator returns untyped string error messages with non-optional "context" for the parse error. We've tried our best to reverse their string formatting to extract a more reasonable error message. Expect this to be improved / fine tuned over time.

…hout main auction flow

…erator marshal call

util/jsonutil/jsonutil.go

hhhjort · 2023-10-17T13:58:06Z

exchange/exchange_test.go

-			expectedErrMessage: "invalid character",
-		},
-		{
-			description:        "Valid extension, empty extBidPrebid and invalid imp ext info",


Why did this test go away?

This test was removed because the json-iterator Marshal function does not validate json.RawMessage fields like the standard library Marshal function so an error in imp.ext will go undetected. In that case I figured we should ensure that whatever is passed into this function is already known to be valid.

Sounds good. These were attempts to test the error handling of Marshal, which may very well be a fools errand.

hhhjort · 2023-10-17T13:59:08Z

exchange/utils_test.go

@@ -403,22 +403,6 @@ func TestCreateSanitizedImpExt(t *testing.T) {
 			},
 			expectedError: "",
 		},
-		{
-			description: "Marshal Error - imp.ext.prebid",


Why did this test case go away?

Same reason as above (#3225 (comment))

hhhjort · 2023-10-17T14:06:19Z

It looks good except I am unclear why 2 test cases went away. Also it seems we did not entirely do away with encoding/json

bsardo · 2023-10-17T16:54:22Z

It looks good except I am unclear why 2 test cases went away. Also it seems we did not entirely do away with encoding/json

Yeah I thought we just wanted to focus on updating the main auction flow for now but an update will be made to account for the other endpoints. The test utils should also be updated and perhaps we can reference json.RawMessage through json-iterator library so encoding/json can be completely removed.

SyntaxNode · 2023-10-18T05:45:14Z

^ These commits extend usage to all other parts of core code and tests. Added error message parsing to attempt to normalize the crazy format of json-iter. Don't want to send this stuff to publishers in a response.

SyntaxNode · 2023-10-18T05:47:22Z

currency/rates_test.go

-						"GBP":0.7662523901
-					},
+						"GBP":0.4815162342
+					}


The behavior of json and json-iter match here. The test was incorrectly setup. It failed because of the trailing comma, nothing to do with the repeated elements. Fixed the test and manually verified behavior matches.

SyntaxNode · 2023-10-18T05:48:02Z

currency/rates_test.go

 		},
 	}

 	for _, tc := range testCases {
 		// Execute:
 		updatedRates := Rates{}
-		err := json.Unmarshal([]byte(tc.ratesJSON), &updatedRates)
+		err := jsonutil.UnmarshalValid([]byte(tc.ratesJSON), &updatedRates)


Updated all tests to use UnmarshalValid because it seemed like we want to validate in tests. Does that check out?

SyntaxNode · 2023-10-18T05:48:27Z

currency/rates_test.go


 		// Verify:
 		assert.Equal(t, err != nil, tc.expectsError, tc.desc)
 		if tc.expectsError {
-			assert.Equal(t, err.Error(), tc.expectedError.Error(), tc.desc)
+			assert.Equal(t, tc.expectedError.Error(), err.Error(), tc.desc)


Expected and actual orders were reversed, leading to confusing error messages. Fixed.

SyntaxNode · 2023-10-18T05:51:05Z

exchange/exchange_test.go

-			expectedErrMessage: "invalid character",
-		},
-		{
-			description:        "Valid extension, empty extBidPrebid and invalid imp ext info",


Sounds good. These were attempts to test the error handling of Marshal, which may very well be a fools errand.

SyntaxNode · 2023-10-18T05:55:19Z

util/jsonutil/jsonutil.go

+
+	msgStartIndex := strings.Index(msg, ": ")
+	if msgStartIndex == -1 {
+		return msg


Fallback to message as-is if markers aren't found.

hhhjort

Looks good once the merge conflict is resolved

bsardo · 2023-10-18T15:32:22Z

LGTM

bsardo

LGTM

bsardo added 5 commits October 16, 2023 12:54

Add json-iterator lib with unmarshal helper functions and call throug…

859c3a4

…hout main auction flow

Add marshal error type and marshal helper function that wraps json-it…

0fcd2f8

…erator marshal call

Call marshal helper function throughout main auction flow

1090b32

Only unmarshal in request wrapper if body is not null

31d7abe

Merge branch 'master' into use-json-iterator-for-serialization

2b28db6

bsardo added do not port PBS 2.0 labels Oct 16, 2023

bsardo assigned hhhjort and guscarreon Oct 16, 2023

SyntaxNode reviewed Oct 16, 2023

View reviewed changes

util/jsonutil/jsonutil.go Outdated Show resolved Hide resolved

Move json-iterator initialization to private package variables

1505461

hhhjort reviewed Oct 17, 2023

View reviewed changes

Cookie Sync

138909a

SyntaxNode added 3 commits October 17, 2023 13:13

Remaining JSON Un/Marshal For Non-Adapter Non-Test

230efba

Update Cookie To UnmarshalValid

72092f2

Extended To Tests

5a2f3b1

SyntaxNode reviewed Oct 18, 2023

View reviewed changes

hhhjort previously approved these changes Oct 18, 2023

View reviewed changes

SyntaxNode added 2 commits October 18, 2023 17:04

Merge branch 'master' into use-json-iterator-for-serialization

334a2a5

Fix Merge Conflicts

240a7ce

SyntaxNode dismissed hhhjort’s stale review via 240a7ce October 18, 2023 21:04

bsardo commented Oct 18, 2023

View reviewed changes

SyntaxNode changed the title ~~JSON Serialization: Use Different Library in Main Auction Flow~~ JSON Serialization: Change Libraries Oct 19, 2023

hhhjort approved these changes Oct 19, 2023

View reviewed changes

SyntaxNode approved these changes Oct 19, 2023

View reviewed changes

bsardo merged commit 6f630fd into prebid:master Oct 19, 2023
5 checks passed

svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023

JSON Serialization: Change Libraries (prebid#3225)

5ded3f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON Serialization: Change Libraries #3225

JSON Serialization: Change Libraries #3225

bsardo commented Oct 16, 2023 •

edited

Loading

hhhjort Oct 17, 2023

bsardo Oct 17, 2023

SyntaxNode Oct 18, 2023

hhhjort Oct 17, 2023

bsardo Oct 17, 2023

hhhjort commented Oct 17, 2023

bsardo commented Oct 17, 2023

SyntaxNode commented Oct 18, 2023

SyntaxNode Oct 18, 2023

SyntaxNode Oct 18, 2023

SyntaxNode Oct 18, 2023

SyntaxNode Oct 18, 2023

SyntaxNode Oct 18, 2023

hhhjort left a comment

bsardo commented Oct 18, 2023

bsardo left a comment

JSON Serialization: Change Libraries #3225

JSON Serialization: Change Libraries #3225

Conversation

bsardo commented Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort commented Oct 17, 2023

bsardo commented Oct 17, 2023

SyntaxNode commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

bsardo commented Oct 18, 2023

bsardo left a comment

Choose a reason for hiding this comment

bsardo commented Oct 16, 2023 •

edited

Loading