Use Json compacter in the bidders/params endpoint #3395

guscarreon · 2024-01-10T17:39:07Z

To not go back to using the default json library in the bidders/params endpoint, this pull requests modifies the handler function to use of the tidwall/pretty library instead.

bsardo · 2024-01-11T03:29:12Z

What are your thoughts on using encoding/json#HTMLEscape followed by encoding/json#Compact instead of this new library prior to calling jsonutil.Marshal which uses json-iter under the hood?
The HTMLEscape function will convert certain characters to unicode (e.g. < to /u003c) if that's needed and Compact will remove unnecessary white space.

SyntaxNode · 2024-01-10T19:07:27Z

router/router.go

@@ -44,6 +44,7 @@ import (
 	"github.com/julienschmidt/httprouter"
 	_ "github.com/lib/pq"
 	"github.com/rs/cors"
+	"github.com/tidwall/pretty"


Curious, why did you choose the pretty package over the built-in json.Compact?

SyntaxNode · 2024-01-10T19:12:29Z

router/router.go

@@ -76,12 +77,12 @@ func newJsonDirectoryServer(schemaDirectory string, validator openrtb_ext.Bidder
 		if !isValid {
 			glog.Fatalf("Schema exists for an unknown bidder: %s", bidder)
 		}
-		data[bidder] = json.RawMessage(validator.Schema(bidderName))
+		data[bidder] = pretty.Ugly([]byte(validator.Schema(bidderName)))


Instead of calling the compact method manually, please add it as an extension registered with json-iter. This will allow all code in PBS to get the benefit without needing to manually make these modifications.

SyntaxNode · 2024-02-08T22:16:40Z

endpoints/openrtb2/auction_test.go

@@ -122,6 +123,7 @@ func TestJsonSampleRequests(t *testing.T) {
 		},
 	}

+	jsoniter.RegisterExtension(&jsonutil.RawMessageExtension{})


TODO: Need to move this line (and all instances of it) to the start of the app. Might be a good use of the func init(), but that doesn't work for tests so we'll need to do something different there.

SyntaxNode · 2024-02-08T22:16:55Z

endpoints/openrtb2/auction_test.go

+func TestSingleJSONTest(t *testing.T) {
+	jsoniter.RegisterExtension(&jsonutil.RawMessageExtension{})
+	runJsonBasedTest(t, "sample-requests/valid-whole/exemplary/simple.json", "")
+}


TODO: Remove dev test.

router/router.go

SyntaxNode · 2024-02-09T03:53:33Z

util/jsonutil/jsonutil.go

+func init() {
+	jsonConfigValidationOff.RegisterExtension(&RawMessageExtension{})
+	jsonConfigValidationOn.RegisterExtension(&RawMessageExtension{})
+}


My untested attempt at an init func. Doesn't work for tests, but I think that's what TestMain is for.

Added a TestMain per every *_test.go file that needs to register the extension

SyntaxNode · 2024-02-09T03:54:23Z

util/jsonutil/jsonutil.go

+		dst := bytes.NewBuffer(make([]byte, 0, len(jsonRawMsg)))
+		if err := json.Compact(dst, jsonRawMsg); err != nil {
+			stream.Error = err
+			return


TODO: Add a test for the error path.

endpoints/openrtb2/auction_test.go

SyntaxNode · 2024-02-13T16:47:23Z

main.go

@@ -21,6 +23,7 @@ import (

 func init() {
 	rand.Seed(time.Now().UnixNano())
+	jsoniter.RegisterExtension(&jsonutil.RawMessageExtension{})


Do we want to register it for all of the jsoniter library, or can/should it be scoped to the jsonConfigValidationOn and jsonConfigValidationOff configs?

SyntaxNode · 2024-02-13T16:48:34Z

util/jsonutil/jsonutil.go

@@ -211,3 +213,44 @@ func tryExtractErrorMessage(err error) string {
 func isLikelyDetailedErrorMessage(msg string) bool {
 	return !strings.HasPrefix(msg, "request.")
 }
+
+// RawMessageExtension will call json.Compact() on every json.RawMessage field when getting marshalled.
+// All other types will be marshalled as usual


Recommend removing the "All other types will be marshalled as usual" part. Doesn't seem necessary.

SyntaxNode · 2024-02-13T16:49:25Z

util/jsonutil/jsonutil.go

+	return nil
+}
+
+// jsonRawMessageType is declared here so we don't have to call TypeOfPtr(&json.RawMessage{}).Elem() everytime we encode


IMHO I don't think this comment is necessary.

util/jsonutil/jsonutil.go

util/jsonutil/jsonutil_test.go

SyntaxNode · 2024-02-13T16:52:02Z

util/jsonutil/jsonutil_test.go

+					},
+				},
+				{
+					desc: "json.RawMessage is passed to Marshal(), expect inner JSON blob to be line-break-free, tab-free, spaces only found inside strings, and compacted into one line",


Test names should be simple, short, and contain no spaces. Characters like - and _ are fine. Please use comments to add more description as needed. Same for other tests.

SyntaxNode · 2024-02-13T16:52:58Z

util/jsonutil/jsonutil_test.go

+					test: func(t *testing.T) {
+						jsoniter.RegisterExtension(&RawMessageExtension{})
+						out, err := Marshal(json.RawMessage(formatted))
+						assert.NoError(t, err)


The error check should be require.NoError so we don't try to. compare a potentially invalid out in the following assertion. Same for the other tests.

Test was modified, no longer asserts for error

SyntaxNode · 2024-02-13T16:54:42Z

util/jsonutil/jsonutil.go

+		jsonRawMsg := *(*[]byte)(ptr)
+
+		dst := bytes.NewBuffer(make([]byte, 0, len(jsonRawMsg)))
+		json.Compact(dst, jsonRawMsg)


Please handle the error.

Good catch. Corrected

SyntaxNode · 2024-02-13T16:56:54Z

router/router_test.go

@@ -275,3 +282,22 @@ func TestValidateDefaultAliases(t *testing.T) {
 		}
 	}
 }
+
+func TestBidderParamsCompactedOutput(t *testing.T) {
+	expectedPrefix := `{"33across":{"$schema":"http://json-schema.org/draft-04/schema#","title":"33Across Adapter Params","description":"A schema which validates params accepted by the 33Across adapter","type":"object","properties":{"productId":{"type":"string","description":"Product type"},"siteId":{"type":"string","description":"Site Id"},"zoneId":{"type":"string","description":"Zone Id"}},"required":["productId","siteId"]}`


Does this rely on the real 33across static file? Will this test fail if that bidder makes an update?

Yes, I forgot to swap the real file for a proper JSON test file. Modified.

hhhjort

LGTM

SyntaxNode

Nitpick with the json test file. Otherwise looks great.

SyntaxNode · 2024-02-27T05:24:39Z

router/bidder_params_tests/appnexus.json

Does this need to be a real bidder name? Could we use foo instead? The file name is "appnexus" and integer param is for AAX.

addressed in latest commit

guscarreon added 2 commits January 10, 2024 12:24

Use Json compacter in the bidders/params endpoint

2355064

Fix aliases

cd0a663

bsardo assigned bsardo and AlexBVolcy Jan 10, 2024

AlexBVolcy previously approved these changes Jan 10, 2024

View reviewed changes

bsardo added the do not port label Jan 11, 2024

escape HTML

144ec17

guscarreon dismissed AlexBVolcy’s stale review via 144ec17 January 11, 2024 06:02

SyntaxNode reviewed Jan 11, 2024

View reviewed changes

guscarreon added 5 commits January 22, 2024 13:26

Create Encoder and Decoder

8cb7a3b

Recovered code. Extension call in auction_test.go still missing

d69938b

CreateDecoder func

5686755

removed go.mod and go.sum entries

1be65a6

cleaned up code a little

a93e96b

guscarreon marked this pull request as draft January 23, 2024 00:42

guscarreon added 4 commits January 23, 2024 11:48

Merge branch 'master' into removeSpaceFromBidderParams

9268615

Merge branch 'master' into removeSpaceFromBidderParams

8b72dd2

Commented out CreateEncode

6edfb64

Tried registering the encoder

0ffb7ed

SyntaxNode reviewed Feb 9, 2024

View reviewed changes

guscarreon added 5 commits February 12, 2024 10:11

Scott's fix

f1eb5c3

init()

7747b2e

TestMain(m *testing.M)

86ee89c

jsonutils test cases

84f852b

Cleaned up code

b6b8ba5

guscarreon force-pushed the removeSpaceFromBidderParams branch from dcf5e27 to b6b8ba5 Compare February 13, 2024 16:10

SyntaxNode reviewed Feb 13, 2024

View reviewed changes

guscarreon added 2 commits February 21, 2024 10:59

Scott's 2nd review part 1

6ed5813

Merge branch 'master' into removeSpaceFromBidderParams

92c00e2

Scott's 2nd review part 2

4d08de6

guscarreon marked this pull request as ready for review February 22, 2024 04:38

Scott's 2nd review part 3

4978277

bsardo assigned SyntaxNode and hhhjort and unassigned bsardo and AlexBVolcy Feb 22, 2024

hhhjort previously approved these changes Feb 26, 2024

View reviewed changes

SyntaxNode reviewed Feb 27, 2024

View reviewed changes

guscarreon added 2 commits February 27, 2024 16:01

LoadSchema function

0fc3933

InitBidderParamsValidator

9f13d75

guscarreon dismissed hhhjort’s stale review via 9f13d75 February 27, 2024 21:30

Rolledback functions

dbebd64

SyntaxNode approved these changes Feb 28, 2024

View reviewed changes

hhhjort approved these changes Feb 28, 2024

View reviewed changes

bsardo merged commit e8267b8 into master Feb 28, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Json compacter in the bidders/params endpoint #3395

Use Json compacter in the bidders/params endpoint #3395

guscarreon commented Jan 10, 2024

bsardo commented Jan 11, 2024

SyntaxNode Jan 10, 2024

SyntaxNode Jan 10, 2024

SyntaxNode Feb 8, 2024

SyntaxNode Feb 8, 2024

SyntaxNode Feb 9, 2024

guscarreon Feb 13, 2024

SyntaxNode Feb 9, 2024

SyntaxNode Feb 13, 2024

SyntaxNode Feb 13, 2024

guscarreon Feb 16, 2024

SyntaxNode Feb 13, 2024

guscarreon Feb 16, 2024

SyntaxNode Feb 13, 2024

guscarreon Feb 22, 2024

SyntaxNode Feb 13, 2024

guscarreon Feb 22, 2024

SyntaxNode Feb 13, 2024

guscarreon Feb 22, 2024

SyntaxNode Feb 13, 2024

guscarreon Feb 22, 2024

hhhjort left a comment

SyntaxNode left a comment

SyntaxNode Feb 27, 2024

guscarreon Feb 27, 2024

Use Json compacter in the bidders/params endpoint #3395

Use Json compacter in the bidders/params endpoint #3395

Conversation

guscarreon commented Jan 10, 2024

bsardo commented Jan 11, 2024

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

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

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

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

SyntaxNode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment