Skip to content

Commit b305f21

Browse files
franklong-safranklong-sa
committed
address comments - use jsonutil, query escape params, pub/supply resolution, simplify native adm unwrap, add test cases
1 parent 1703abf commit b305f21

5 files changed

Lines changed: 146 additions & 25 deletions

File tree

‎adapters/stackadapt/params_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,4 +55,6 @@ var invalidParams = []string{
5555
`{"publisherId":"pub-123","supplyId":""}`,
5656
`{"publisherId":"pub-123","supplyId":"ssp-1","bidfloor":-1}`,
5757
`{"publisherId":"pub-123","supplyId":"ssp-1","banner":"invalid"}`,
58+
`{"publisherId":"pub-123","supplyId":"ssp-1","bidfloor":"high"}`,
59+
`{"publisherId":"pub-123","supplyId":"ssp-1","banner":{"expdir":["a"]}}`,
5860
}

‎adapters/stackadapt/stackadapt.go‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
package stackadapt
22

33
import (
4-
"encoding/json"
54
"errors"
65
"fmt"
76
"net/http"
7+
"net/url"
88
"strconv"
99
"strings"
1010
"text/template"
@@ -50,7 +50,7 @@ func (a *adapter) MakeRequests(request *openrtb2.BidRequest, _ *adapters.ExtraRe
5050
return nil, []error{err}
5151
}
5252

53-
body, err := json.Marshal(request)
53+
body, err := jsonutil.Marshal(request)
5454
if err != nil {
5555
return nil, []error{fmt.Errorf("marshal bidRequest: %w", err)}
5656
}
@@ -69,8 +69,8 @@ func (a *adapter) MakeRequests(request *openrtb2.BidRequest, _ *adapters.ExtraRe
6969

7070
func (a *adapter) buildEndpointURL(publisherID, supplyID string) (string, error) {
7171
params := macros.EndpointTemplateParams{
72-
PublisherID: publisherID,
73-
SupplyId: supplyID,
72+
PublisherID: url.QueryEscape(publisherID),
73+
SupplyId: url.QueryEscape(supplyID),
7474
}
7575
return macros.ResolveMacros(a.endpoint, params)
7676
}
@@ -88,8 +88,12 @@ func setImpsAndGetEndpointParams(request *openrtb2.BidRequest) (string, string,
8888
return "", "", &errortypes.BadInput{Message: fmt.Sprintf("imp[%d]: unable to unmarshal bidder ext: %s", i, err.Error())}
8989
}
9090

91-
publisherID = saExt.PublisherId
92-
supplyID = saExt.SupplyId
91+
if i == 0 {
92+
publisherID = saExt.PublisherId
93+
supplyID = saExt.SupplyId
94+
} else if saExt.PublisherId != publisherID || saExt.SupplyId != supplyID {
95+
return "", "", &errortypes.BadInput{Message: fmt.Sprintf("imp[%d]: all imps must have the same publisherId and supplyId", i)}
96+
}
9397

9498
if saExt.PlacementId != nil {
9599
request.Imp[i].TagID = *saExt.PlacementId
@@ -210,18 +214,15 @@ func resolveMacros(bid *openrtb2.Bid) {
210214
}
211215

212216
func getNativeAdm(adm string) (string, error) {
213-
nativeAdm := make(map[string]interface{})
214-
if err := jsonutil.Unmarshal([]byte(adm), &nativeAdm); err != nil {
217+
value, dataType, _, err := jsonparser.Get([]byte(adm), string(openrtb_ext.BidTypeNative))
218+
if err != nil {
219+
if errors.Is(err, jsonparser.KeyPathNotFoundError) {
220+
return adm, nil
221+
}
215222
return adm, errors.New("unable to unmarshal native adm")
216223
}
217-
218-
if _, ok := nativeAdm["native"]; ok {
219-
value, dataType, _, err := jsonparser.Get([]byte(adm), string(openrtb_ext.BidTypeNative))
220-
if err != nil || dataType != jsonparser.Object {
221-
return adm, errors.New("unable to get native adm")
222-
}
223-
adm = string(value)
224+
if dataType != jsonparser.Object {
225+
return adm, errors.New("unable to get native adm")
224226
}
225-
226-
return adm, nil
227+
return string(value), nil
227228
}

‎adapters/stackadapt/stackadapt_test.go‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,13 @@ func TestGetNativeAdm(t *testing.T) {
129129
expectedAdm: `{"link":{"url":"https://example.com"},"assets":[{"id":1,"title":{"text":"Title"}}]}`,
130130
},
131131
{
132-
name: "invalid_json",
133-
adm: `not json`,
132+
name: "no_native_key_passthrough",
133+
adm: `not json`,
134+
expectedAdm: `not json`,
135+
},
136+
{
137+
name: "malformed_native_envelope",
138+
adm: `{"native":`,
134139
wantErr: true,
135140
},
136141
{
@@ -146,7 +151,7 @@ func TestGetNativeAdm(t *testing.T) {
146151
assert.Error(t, err)
147152
} else {
148153
assert.NoError(t, err)
149-
assert.JSONEq(t, tt.expectedAdm, result)
154+
assert.Equal(t, tt.expectedAdm, result)
150155
}
151156
})
152157
}
@@ -257,17 +262,29 @@ func TestSetImpsAndGetEndpointParams(t *testing.T) {
257262
assert.Contains(t, err.Error(), "unable to unmarshal bidder ext")
258263
})
259264

260-
t.Run("multi_imp_uses_last_publisher_id_and_supply_id", func(t *testing.T) {
265+
t.Run("multi_imp_same_publisher_id_and_supply_id", func(t *testing.T) {
261266
req := &openrtb2.BidRequest{
262267
Imp: []openrtb2.Imp{
263-
{Ext: json.RawMessage(`{"bidder":{"publisherId":"first","supplyId":"ssp-first"}}`)},
264-
{Ext: json.RawMessage(`{"bidder":{"publisherId":"second","supplyId":"ssp-second"}}`)},
268+
{Ext: json.RawMessage(`{"bidder":{"publisherId":"pub-1","supplyId":"ssp-1"}}`)},
269+
{Ext: json.RawMessage(`{"bidder":{"publisherId":"pub-1","supplyId":"ssp-1"}}`)},
265270
},
266271
}
267272
pubID, supplyID, err := setImpsAndGetEndpointParams(req)
268273
assert.NoError(t, err)
269-
assert.Equal(t, "second", pubID)
270-
assert.Equal(t, "ssp-second", supplyID)
274+
assert.Equal(t, "pub-1", pubID)
275+
assert.Equal(t, "ssp-1", supplyID)
276+
})
277+
278+
t.Run("multi_imp_mismatched_publisher_id_or_supply_id", func(t *testing.T) {
279+
req := &openrtb2.BidRequest{
280+
Imp: []openrtb2.Imp{
281+
{Ext: json.RawMessage(`{"bidder":{"publisherId":"pub-1","supplyId":"ssp-1"}}`)},
282+
{Ext: json.RawMessage(`{"bidder":{"publisherId":"pub-2","supplyId":"ssp-1"}}`)},
283+
},
284+
}
285+
_, _, err := setImpsAndGetEndpointParams(req)
286+
assert.Error(t, err)
287+
assert.Contains(t, err.Error(), "all imps must have the same publisherId and supplyId")
271288
})
272289

273290
t.Run("zero_bidfloor_not_set", func(t *testing.T) {

‎adapters/stackadapt/stackadapttest/supplemental/endpoint-param-escaping.json‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
{
2+
"mockBidRequest": {
3+
"id": "test-request-id",
4+
"imp": [{
5+
"id": "test-imp-id",
6+
"banner": {
7+
"format": [{
8+
"w": 300,
9+
"h": 250
10+
}]
11+
},
12+
"ext": {
13+
"bidder": {
14+
"publisherId": "123",
15+
"supplyId": "ssp-1"
16+
}
17+
}
18+
}],
19+
"site": {
20+
"page": "https://example.com/page"
21+
}
22+
},
23+
"httpCalls": [{
24+
"expectedRequest": {
25+
"uri": "http://localhost/br?publisher_id=pub%26123&supply_id=ssp+1",
26+
"body": {
27+
"id": "test-request-id",
28+
"imp": [{
29+
"id": "test-imp-id",
30+
"banner": {
31+
"format": [{
32+
"w": 300,
33+
"h": 250
34+
}]
35+
},
36+
"ext": {
37+
"bidder": {
38+
"publisherId": "123",
39+
"supplyId": "ssp-1"
40+
}
41+
}
42+
}],
43+
"site": {
44+
"page": "https://example.com/page",
45+
"publisher": {
46+
"id": "123"
47+
}
48+
}
49+
},
50+
"impIDs": ["test-imp-id"]
51+
},
52+
"mockResponse": {
53+
"status": 204,
54+
"body": {}
55+
}
56+
}],
57+
"expectedBidResponses": []
58+
}

‎adapters/stackadapt/stackadapttest/supplemental/multi-imp-mismatch.json‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
{
2+
"mockBidRequest": {
3+
"id": "test-request-id",
4+
"imp": [{
5+
"id": "test-imp-id",
6+
"banner": {
7+
"format": [{
8+
"w": 300,
9+
"h": 250
10+
}]
11+
},
12+
"ext": {
13+
"bidder": {
14+
"publisherId": "123",
15+
"supplyId": "ssp-1"
16+
}
17+
}
18+
},
19+
{
20+
"id": "test-imp-id-2",
21+
"banner": {
22+
"format": [{
23+
"w": 728,
24+
"h": 90
25+
}]
26+
},
27+
"ext": {
28+
"bidder": {
29+
"publisherId": "456",
30+
"supplyId": "ssp-1"
31+
}
32+
}
33+
}
34+
],
35+
"site": {
36+
"page": "https://example.com/page"
37+
}
38+
},
39+
"expectedMakeRequestsErrors": [{
40+
"value": "imp[1]: all imps must have the same publisherId and supplyId",
41+
"comparison": "literal"
42+
}]
43+
}

0 commit comments

Comments
 (0)