Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Viper #1969

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Update Viper #1969

merged 2 commits into from
Aug 20, 2021

Conversation

SyntaxNode
Copy link
Contributor

The /cookie_sync Endpoint Rewrite exposed a performance bug in the version of Viper we were using when using BindEnv. This PR updates Viper to the latest version to solve the performance bug, cleans up the config tests related to user syncing, and adds a user syncing environment variable override test.

endpoint: http://test-bid.ybp.yahoo.com/bid/appnexuspbs
adkerneladn:
usersync_url: https://tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=
blacklisted_apps: ["spamAppID","sketchy-app-id"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values are not important for this test case and have been removed.

usersync_url: https://tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=
usersync:
redirect:
url: http://http://test-bh.ybp.yahoo.com/sync/appnexuspbs?gdpr={{.GDPR}}&euconsent={{.GDPRConsent}}&url=%s
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to the new user sync config values.

func cmpNils(t *testing.T, key string, a interface{}) {
t.Helper()
assert.Nilf(t, a, "%s: %t != nil", key, a)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept with the test pattern. I think we'll want to deprecate this approach in the future, but kept with the pattern for now.

@@ -597,9 +591,6 @@ func TestUnmarshalAdapterExtraInfo(t *testing.T) {
// Assert correctly unmarshaled
assert.NoError(t, err, "invalid endpoint in config should return an error")

// Unescape quotes of JSON-formatted string
strings.Replace(cfg.Adapters[string(openrtb_ext.BidderAppnexus)].ExtraAdapterInfo, "\\\"", "\"", -1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually performs no operation since it returns the new string which is ignored. Removed. The unescape isn't necessary.

if assert.IsType(t, errortypes.AggregateError{}, err) {
aggErr := err.(errortypes.AggregateError)
assert.ElementsMatch(t, []error{errors.New("The endpoint: ib.adnxs.com/some/endpoint for appnexus is not a valid URL")}, aggErr.Errors)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the exact error to be sure it's catching the error we expect.

assert.Nil(t, cfg.Adapters["rubicon"].Syncer.SupportCORS)

assert.Nil(t, cfg.Adapters["brightroll"].Syncer)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensures the proper handling of pointers with nil defaults via BindEnv.

@@ -53,7 +45,7 @@ require (
github.com/yudai/gojsondiff v0.0.0-20170107030110-7b1b7adf999d
github.com/yudai/golcs v0.0.0-20170316035057-ecda9a501e82 // indirect
github.com/yudai/pp v2.0.1+incompatible // indirect
golang.org/x/net v0.0.0-20201202161906-c7110b5ffcbb
golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4
golang.org/x/text v0.3.6
gopkg.in/yaml.v2 v2.4.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only updated viper. The rest of the updates are transitive.

@@ -1,135 +1,316 @@
github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this trigger so many changes in go.sum? go.mod seems more reasonable since there are roughly the same number of new lines as old lines.

Copy link
Contributor Author

@SyntaxNode SyntaxNode Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm not sure. They are from the Viper go.sum, but I don't understand why the hashes need to exist. go.sum is kind of a mystery to me. They don't appear in the vendor directory. I suppose it's some kind of security feature or maybe the viper go.sum just needs to be tidied?

@guscarreon guscarreon self-requested a review August 20, 2021 15:24
@guscarreon guscarreon self-assigned this Aug 20, 2021
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@guscarreon guscarreon merged commit 7beb2cb into prebid:master Aug 20, 2021
wwwyyy pushed a commit to wwwyyy/prebid-server that referenced this pull request Aug 20, 2021
… prebid-master

* 'master' of https://github.com/prebid/prebid-server:
  Update Viper (prebid#1969)
  /cookie_sync Endpoint Rewrite (prebid#1879)
  New Adapter: IQZone (prebid#1964)
  Remove old race conidtion tests (prebid#1958)
  Remove time.Now() as the first parameter of NewRates() (prebid#1953)

# Conflicts:
#	config/config.go
#	usersync/usersyncers/syncer_test.go
@SyntaxNode SyntaxNode deleted the update_viper branch September 2, 2021 16:37
jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants