Skip to content

Commit caced15

Browse files
Pubmatic-Dhruv-SononePubmatic-Dhruv-Sonone
authored
Account Service Optimisation (prebid#2440)
1 parent e6ff80a commit caced15

File tree

18 files changed

+85
-61
lines changed

18 files changed

+85
-61
lines changed

‎account/account.go

+15-18
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
2828
Message: fmt.Sprintf("Prebid-server has been configured to discard requests without a valid Account ID. Please reach out to the prebid server host."),
2929
}}
3030
}
31-
if accountJSON, accErrs := fetcher.FetchAccount(ctx, accountID); len(accErrs) > 0 || accountJSON == nil {
31+
if accountJSON, accErrs := fetcher.FetchAccount(ctx, cfg.AccountDefaultsJSON(), accountID); len(accErrs) > 0 || accountJSON == nil {
3232
// accountID does not reference a valid account
3333
for _, e := range accErrs {
3434
if _, ok := e.(stored_requests.NotFoundError); !ok {
@@ -49,25 +49,22 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
4949
} else {
5050
// accountID resolved to a valid account, merge with AccountDefaults for a complete config
5151
account = &config.Account{}
52-
completeJSON, err := jsonpatch.MergePatch(cfg.AccountDefaultsJSON(), accountJSON)
53-
if err == nil {
54-
err = json.Unmarshal(completeJSON, account)
52+
err := json.Unmarshal(accountJSON, account)
53+
54+
// this logic exists for backwards compatibility. If the initial unmarshal fails above, we attempt to
55+
// resolve it by converting the GDPR enforce purpose fields and then attempting an unmarshal again before
56+
// declaring a malformed account error.
57+
// unmarshal fetched account to determine if it is well-formed
58+
if _, ok := err.(*json.UnmarshalTypeError); ok {
59+
// attempt to convert deprecated GDPR enforce purpose fields to resolve issue
60+
accountJSON, err = ConvertGDPREnforcePurposeFields(accountJSON)
61+
// unmarshal again to check if unmarshal error still exists after GDPR field conversion
62+
err = json.Unmarshal(accountJSON, account)
5563

56-
// this logic exists for backwards compatibility. If the initial unmarshal fails above, we attempt to
57-
// resolve it by converting the GDPR enforce purpose fields and then attempting an unmarshal again before
58-
// declaring a malformed account error.
59-
// unmarshal fetched account to determine if it is well-formed
6064
if _, ok := err.(*json.UnmarshalTypeError); ok {
61-
// attempt to convert deprecated GDPR enforce purpose fields to resolve issue
62-
completeJSON, err = ConvertGDPREnforcePurposeFields(completeJSON)
63-
// unmarshal again to check if unmarshal error still exists after GDPR field conversion
64-
err = json.Unmarshal(completeJSON, account)
65-
66-
if _, ok := err.(*json.UnmarshalTypeError); ok {
67-
return nil, []error{&errortypes.MalformedAcct{
68-
Message: fmt.Sprintf("The prebid-server account config for account id \"%s\" is malformed. Please reach out to the prebid server host.", accountID),
69-
}}
70-
}
65+
return nil, []error{&errortypes.MalformedAcct{
66+
Message: fmt.Sprintf("The prebid-server account config for account id \"%s\" is malformed. Please reach out to the prebid server host.", accountID),
67+
}}
7168
}
7269
}
7370

‎account/account_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ var mockAccountData = map[string]json.RawMessage{
2525
type mockAccountFetcher struct {
2626
}
2727

28-
func (af mockAccountFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
28+
func (af mockAccountFetcher) FetchAccount(ctx context.Context, accountDefaultsJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
2929
if account, ok := mockAccountData[accountID]; ok {
3030
return account, nil
3131
}

‎endpoints/cookie_sync_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -1815,8 +1815,8 @@ type FakeAccountsFetcher struct {
18151815
AccountData map[string]json.RawMessage
18161816
}
18171817

1818-
func (f FakeAccountsFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
1819-
defaultAccountJSON := json.RawMessage(`{"disabled":false}`)
1818+
func (f FakeAccountsFetcher) FetchAccount(ctx context.Context, defaultAccountJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
1819+
defaultAccountJSON = json.RawMessage(`{"disabled":false}`)
18201820

18211821
if accountID == metrics.PublisherUnknown {
18221822
return defaultAccountJSON, nil

‎endpoints/events/event_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ type mockAccountsFetcher struct {
8383
DurationMS int
8484
}
8585

86-
func (maf mockAccountsFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
86+
func (maf mockAccountsFetcher) FetchAccount(ctx context.Context, defaultAccountJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
8787
if maf.DurationMS > 0 {
8888
select {
8989
case <-time.After(time.Duration(maf.DurationMS) * time.Millisecond):

‎endpoints/openrtb2/test_utils.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -1341,7 +1341,7 @@ type mockAccountFetcher struct {
13411341
data map[string]json.RawMessage
13421342
}
13431343

1344-
func (af *mockAccountFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
1344+
func (af *mockAccountFetcher) FetchAccount(ctx context.Context, defaultAccountJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
13451345
if account, ok := af.data[accountID]; ok {
13461346
return account, nil
13471347
}

‎endpoints/setuid_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -533,7 +533,7 @@ func TestSetUIDEndpointMetrics(t *testing.T) {
533533
Status: 400,
534534
Bidder: "pubmatic",
535535
UID: "",
536-
Errors: []error{errors.New("Invalid JSON Patch")},
536+
Errors: []error{errors.New("unexpected end of JSON input")},
537537
Success: false,
538538
}
539539
a.On("LogSetUIDObject", &expected).Once()
@@ -710,6 +710,7 @@ func doRequest(req *http.Request, analytics analytics.PBSAnalyticsModule, metric
710710
BlacklistedAcctMap: map[string]bool{
711711
"blocked_acct": true,
712712
},
713+
AccountDefaults: config.Account{},
713714
}
714715
cfg.MarshalAccountDefaults()
715716

‎stored_requests/backends/db_fetcher/fetcher.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ func (fetcher *dbFetcher) FetchResponses(ctx context.Context, ids []string) (dat
151151

152152
}
153153

154-
func (fetcher *dbFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
154+
func (fetcher *dbFetcher) FetchAccount(ctx context.Context, accountDefaultsJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
155155
return nil, []error{stored_requests.NotFoundError{accountID, "Account"}}
156156
}
157157

‎stored_requests/backends/empty_fetcher/fetcher.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func (fetcher EmptyFetcher) FetchResponses(ctx context.Context, ids []string) (d
3232
return nil, nil
3333
}
3434

35-
func (fetcher EmptyFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
35+
func (fetcher EmptyFetcher) FetchAccount(ctx context.Context, accountDefaultJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
3636
return nil, []error{stored_requests.NotFoundError{accountID, "Account"}}
3737
}
3838

‎stored_requests/backends/file_fetcher/fetcher.go

+8-2
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"strings"
99

1010
"github.com/prebid/prebid-server/stored_requests"
11+
jsonpatch "gopkg.in/evanphx/json-patch.v4"
1112
)
1213

1314
// NewFileFetcher _immediately_ loads stored request data from local files.
@@ -38,7 +39,7 @@ func (fetcher *eagerFetcher) FetchResponses(ctx context.Context, ids []string) (
3839
}
3940

4041
// FetchAccount fetches the host account configuration for a publisher
41-
func (fetcher *eagerFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
42+
func (fetcher *eagerFetcher) FetchAccount(ctx context.Context, accountDefaultsJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
4243
if len(accountID) == 0 {
4344
return nil, []error{fmt.Errorf("Cannot look up an empty accountID")}
4445
}
@@ -49,7 +50,12 @@ func (fetcher *eagerFetcher) FetchAccount(ctx context.Context, accountID string)
4950
DataType: "Account",
5051
}}
5152
}
52-
return accountJSON, nil
53+
54+
completeJSON, err := jsonpatch.MergePatch(accountDefaultsJSON, accountJSON)
55+
if err != nil {
56+
return nil, []error{err}
57+
}
58+
return completeJSON, nil
5359
}
5460

5561
func (fetcher *eagerFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {

‎stored_requests/backends/file_fetcher/fetcher_test.go

+9-3
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,20 @@ func TestAccountFetcher(t *testing.T) {
2828
fetcher, err := NewFileFetcher("./test")
2929
assert.NoError(t, err, "Failed to create test fetcher")
3030

31-
account, errs := fetcher.FetchAccount(context.Background(), "valid")
31+
account, errs := fetcher.FetchAccount(context.Background(), json.RawMessage(`{"events_enabled":true}`), "valid")
3232
assertErrorCount(t, 0, errs)
33-
assert.JSONEq(t, `{"disabled":false, "id":"valid"}`, string(account))
33+
assert.JSONEq(t, `{"disabled":false, "events_enabled":true, "id":"valid" }`, string(account))
3434

35-
_, errs = fetcher.FetchAccount(context.Background(), "nonexistent")
35+
_, errs = fetcher.FetchAccount(context.Background(), json.RawMessage(`{"events_enabled":true}`), "nonexistent")
3636
assertErrorCount(t, 1, errs)
3737
assert.Error(t, errs[0])
3838
assert.Equal(t, stored_requests.NotFoundError{"nonexistent", "Account"}, errs[0])
39+
40+
_, errs = fetcher.FetchAccount(context.Background(), json.RawMessage(`{"events_enabled"}`), "valid")
41+
assertErrorCount(t, 1, errs)
42+
assert.Error(t, errs[0])
43+
assert.Equal(t, fmt.Errorf("Invalid JSON Document"), errs[0])
44+
3945
}
4046

4147
func TestInvalidDirectory(t *testing.T) {

‎stored_requests/backends/http_fetcher/fetcher.go

+7-2
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strings"
1212

1313
"github.com/prebid/prebid-server/stored_requests"
14+
jsonpatch "gopkg.in/evanphx/json-patch.v4"
1415

1516
"github.com/golang/glog"
1617
"golang.org/x/net/context/ctxhttp"
@@ -151,7 +152,7 @@ func (fetcher *HttpFetcher) FetchAccounts(ctx context.Context, accountIDs []stri
151152
}
152153

153154
// FetchAccount fetchers a single accountID and returns its corresponding json
154-
func (fetcher *HttpFetcher) FetchAccount(ctx context.Context, accountID string) (accountJSON json.RawMessage, errs []error) {
155+
func (fetcher *HttpFetcher) FetchAccount(ctx context.Context, accountDefaultsJSON json.RawMessage, accountID string) (accountJSON json.RawMessage, errs []error) {
155156
accountData, errs := fetcher.FetchAccounts(ctx, []string{accountID})
156157
if len(errs) > 0 {
157158
return nil, errs
@@ -163,7 +164,11 @@ func (fetcher *HttpFetcher) FetchAccount(ctx context.Context, accountID string)
163164
DataType: "Account",
164165
}}
165166
}
166-
return accountJSON, nil
167+
completeJSON, err := jsonpatch.MergePatch(accountDefaultsJSON, accountJSON)
168+
if err != nil {
169+
return nil, []error{err}
170+
}
171+
return completeJSON, nil
167172
}
168173

169174
func (fetcher *HttpFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {

‎stored_requests/backends/http_fetcher/fetcher_test.go

+17-7
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package http_fetcher
33
import (
44
"context"
55
"encoding/json"
6+
"fmt"
67
"net/http"
78
"net/http/httptest"
89
"strings"
@@ -134,16 +135,25 @@ func TestFetchAccount(t *testing.T) {
134135
fetcher, close := newTestAccountFetcher(t, []string{"acc-1"})
135136
defer close()
136137

137-
account, errs := fetcher.FetchAccount(context.Background(), "acc-1")
138+
account, errs := fetcher.FetchAccount(context.Background(), json.RawMessage(`{"disabled":true}`), "acc-1")
138139
assert.Empty(t, errs, "Unexpected error fetching existing account")
139-
assert.JSONEq(t, `"acc-1"`, string(account), "Unexpected account data fetching existing account")
140+
assert.JSONEq(t, `{"disabled": true, "id":"acc-1"}`, string(account), "Unexpected account data fetching existing account")
141+
}
142+
143+
func TestAccountMergeError(t *testing.T) {
144+
fetcher, close := newTestAccountFetcher(t, []string{"acc-1"})
145+
defer close()
146+
147+
_, errs := fetcher.FetchAccount(context.Background(), json.RawMessage(`{"disabled"}`), "acc-1")
148+
assert.Error(t, errs[0])
149+
assert.Equal(t, fmt.Errorf("Invalid JSON Document"), errs[0])
140150
}
141151

142152
func TestFetchAccountNoData(t *testing.T) {
143153
fetcher, close := newFetcherBrokenBackend()
144154
defer close()
145155

146-
unknownAccount, errs := fetcher.FetchAccount(context.Background(), "unknown-acc")
156+
unknownAccount, errs := fetcher.FetchAccount(context.Background(), json.RawMessage(`{disabled":true}`), "unknown-acc")
147157
assert.NotEmpty(t, errs, "Retrieving unknown account should return error")
148158
assert.Nil(t, unknownAccount, "Retrieving unknown account should return nil json.RawMessage")
149159
}
@@ -152,7 +162,7 @@ func TestFetchAccountNoIDProvided(t *testing.T) {
152162
fetcher, close := newTestAccountFetcher(t, nil)
153163
defer close()
154164

155-
account, errs := fetcher.FetchAccount(context.Background(), "")
165+
account, errs := fetcher.FetchAccount(context.Background(), json.RawMessage(`{disabled":true}`), "")
156166
assert.Len(t, errs, 1, "Fetching account with empty id should error")
157167
assert.Nil(t, account, "Fetching account with empty id should return nil")
158168
}
@@ -233,12 +243,12 @@ func newHandler(t *testing.T, expectReqIDs []string, expectImpIDs []string, json
233243
}
234244

235245
func newTestAccountFetcher(t *testing.T, expectAccIDs []string) (fetcher *HttpFetcher, closer func()) {
236-
handler := newAccountHandler(t, expectAccIDs, jsonifyID)
246+
handler := newAccountHandler(t, expectAccIDs)
237247
server := httptest.NewServer(http.HandlerFunc(handler))
238248
return NewFetcher(server.Client(), server.URL), server.Close
239249
}
240250

241-
func newAccountHandler(t *testing.T, expectAccIDs []string, jsonifier func(string) json.RawMessage) func(w http.ResponseWriter, r *http.Request) {
251+
func newAccountHandler(t *testing.T, expectAccIDs []string) func(w http.ResponseWriter, r *http.Request) {
242252
return func(w http.ResponseWriter, r *http.Request) {
243253
query := r.URL.Query()
244254
gotAccIDs := richSplit(query.Get("account-ids"))
@@ -249,7 +259,7 @@ func newAccountHandler(t *testing.T, expectAccIDs []string, jsonifier func(strin
249259

250260
for _, accID := range gotAccIDs {
251261
if accID != "" {
252-
accIDResponse[accID] = jsonifier(accID)
262+
accIDResponse[accID] = json.RawMessage(`{"id":"` + accID + `"}`)
253263
}
254264
}
255265

‎stored_requests/data/by_id/stored_imps/.gitignore

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
# Ignore everything in this directory, except for this file
22
*
3-
!.gitignore
3+
!.gitignore

‎stored_requests/data/by_id/stored_requests/.gitignore

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
# Ignore everything in this directory, except for this file
22
*
3-
!.gitignore
3+
!.gitignore

‎stored_requests/fetcher.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ type Fetcher interface {
2828

2929
type AccountFetcher interface {
3030
// FetchAccount fetches the host account configuration for a publisher
31-
FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error)
31+
FetchAccount(ctx context.Context, accountDefaultJSON json.RawMessage, accountID string) (json.RawMessage, []error)
3232
}
3333

3434
type CategoryFetcher interface {
@@ -210,7 +210,7 @@ func (f *fetcherWithCache) FetchResponses(ctx context.Context, ids []string) (da
210210
return
211211
}
212212

213-
func (f *fetcherWithCache) FetchAccount(ctx context.Context, accountID string) (account json.RawMessage, errs []error) {
213+
func (f *fetcherWithCache) FetchAccount(ctx context.Context, acccountDefaultJSON json.RawMessage, accountID string) (account json.RawMessage, errs []error) {
214214
accountData := f.cache.Accounts.Get(ctx, []string{accountID})
215215
// TODO: add metrics
216216
if account, ok := accountData[accountID]; ok {
@@ -219,7 +219,7 @@ func (f *fetcherWithCache) FetchAccount(ctx context.Context, accountID string) (
219219
} else {
220220
f.metricsEngine.RecordAccountCacheResult(metrics.CacheMiss, 1)
221221
}
222-
account, errs = f.fetcher.FetchAccount(ctx, accountID)
222+
account, errs = f.fetcher.FetchAccount(ctx, acccountDefaultJSON, accountID)
223223
if len(errs) == 0 {
224224
f.cache.Accounts.Save(ctx, map[string]json.RawMessage{accountID: account})
225225
}

‎stored_requests/fetcher_test.go

+5-6
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ func TestAccountCacheHit(t *testing.T) {
243243
})
244244

245245
metricsEngine.On("RecordAccountCacheResult", metrics.CacheHit, 1)
246-
account, errs := aFetcherWithCache.FetchAccount(ctx, "known")
246+
account, errs := aFetcherWithCache.FetchAccount(ctx, json.RawMessage("{}"), "known")
247247

248248
accCache.AssertExpectations(t)
249249
fetcher.AssertExpectations(t)
@@ -263,18 +263,17 @@ func TestAccountCacheMiss(t *testing.T) {
263263
// Test read from cache
264264
accCache.On("Get", ctx, uncachedAccounts).Return(map[string]json.RawMessage{})
265265
accCache.On("Save", ctx, uncachedAccountsData)
266-
fetcher.On("FetchAccount", ctx, "uncached").Return(uncachedAccountsData["uncached"], []error{})
266+
fetcher.On("FetchAccount", ctx, json.RawMessage("{}"), "uncached").Return(uncachedAccountsData["uncached"], []error{})
267267
metricsEngine.On("RecordAccountCacheResult", metrics.CacheMiss, 1)
268268

269-
account, errs := aFetcherWithCache.FetchAccount(ctx, "uncached")
269+
account, errs := aFetcherWithCache.FetchAccount(ctx, json.RawMessage("{}"), "uncached")
270270

271271
accCache.AssertExpectations(t)
272272
fetcher.AssertExpectations(t)
273273
metricsEngine.AssertExpectations(t)
274274
assert.JSONEq(t, `true`, string(account), "FetchAccount should fetch the right account data")
275275
assert.Len(t, errs, 0, "FetchAccount shouldn't return any errors")
276276
}
277-
278277
func TestComposedCache(t *testing.T) {
279278
c1 := &mockCache{}
280279
c2 := &mockCache{}
@@ -348,8 +347,8 @@ func (f *mockFetcher) FetchResponses(ctx context.Context, ids []string) (data ma
348347
return args.Get(0).(map[string]json.RawMessage), args.Get(1).([]error)
349348
}
350349

351-
func (a *mockFetcher) FetchAccount(ctx context.Context, accountID string) (json.RawMessage, []error) {
352-
args := a.Called(ctx, accountID)
350+
func (a *mockFetcher) FetchAccount(ctx context.Context, defaultAccountsJSON json.RawMessage, accountID string) (json.RawMessage, []error) {
351+
args := a.Called(ctx, defaultAccountsJSON, accountID)
353352
return args.Get(0).(json.RawMessage), args.Get(1).([]error)
354353
}
355354

‎stored_requests/multifetcher.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,10 @@ func (mf MultiFetcher) FetchResponses(ctx context.Context, ids []string) (data m
4040
return nil, nil
4141
}
4242

43-
func (mf MultiFetcher) FetchAccount(ctx context.Context, accountID string) (account json.RawMessage, errs []error) {
43+
func (mf MultiFetcher) FetchAccount(ctx context.Context, accountDefaultJSON json.RawMessage, accountID string) (account json.RawMessage, errs []error) {
4444
for _, f := range mf {
4545
if af, ok := f.(AccountFetcher); ok {
46-
if account, accErrs := af.FetchAccount(ctx, accountID); len(accErrs) == 0 {
46+
if account, accErrs := af.FetchAccount(ctx, accountDefaultJSON, accountID); len(accErrs) == 0 {
4747
return account, nil
4848
} else {
4949
accErrs = dropMissingIDs(accErrs)

0 commit comments

Comments
 (0)