Syncer Level ExternalURL Override #2072
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guscarreon
reviewed
Nov 15, 2021
usersync/syncer.go
Outdated
| redirectTemplate := syncerEndpoint.RedirectURL | ||
| if redirectTemplate == "" { | ||
| redirectTemplate = hostConfig.RedirectURL | ||
| } | ||
|
|
||
| externalURL := syncerEndpoint.ExternalURL | ||
| if externalURL == "" { | ||
| externalURL = syncerExternalURL | ||
| } |
There was a problem hiding this comment.
I find the consecutive if externalURL == "" a little confusing. Can we be more explicit? Maybe even with a comment to make it extra clear?
156 - externalURL := syncerEndpoint.ExternalURL
157 - if externalURL == "" {
158 - externalURL = syncerExternalURL
159 - }
160 - if externalURL == "" {
161 - externalURL = hostConfig.ExternalURL
162 - }
+ // Find an externalURL value either of the following: syncerEndpoint.ExternalURL, or syncerExternalURL or hostConfig.ExternalURL
+ // they take precedence in that exact order
+ var externalURL string
+ if syncerEndpoint.ExternalURL != "" {
+ externalURL = syncerEndpoint.ExternalURL
+ } else if syncerExternalURL != "" {
+ externalURL = syncerExternalURL
+ } else {
+ externalURL = hostConfig.ExternalURL
+ }
usersync/syncer.go
There was a problem hiding this comment.
I gotcha. Refactored.
| @@ -1102,6 +1102,7 @@ func setBidderDefaults(v *viper.Viper, bidder string) { | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.redirect.redirect_url") | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.redirect.external_url") | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.redirect.user_macro") | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.external_url") | |||
There was a problem hiding this comment.
Can we add an assertion in config/config_test.go? Maybe in TestFullConfig or even in the TestUnmarshalAdapterExtraInfo test?
There was a problem hiding this comment.
Do you really think we need to? There's nothing special with this config value. Adding it to TestFullConfig is really just testing Viper and not so much this specific value.
There was a problem hiding this comment.
Sure, I can live with that.
| @@ -109,6 +112,10 @@ func (s *Syncer) Override(original *Syncer) *Syncer { | |||
| copy.Redirect = s.Redirect.Override(original.Redirect) | |||
| } | |||
There was a problem hiding this comment.
I realize this isn't part of your PR but, should we use this opportunity to merge the repetitive if statements above into one?
103 if original == nil {
104 copy.IFrame = s.IFrame.Override(nil)
+ copy.Redirect = s.Redirect.Override(nil)
105 } else {
106 copy.IFrame = s.IFrame.Override(original.IFrame)
+ copy.Redirect = s.Redirect.Override(original.Redirect)
107 }
108
109 - if original == nil {
110 - copy.Redirect = s.Redirect.Override(nil)
111 - } else {
112 - copy.Redirect = s.Redirect.Override(original.Redirect)
113 - }
config/bidderinfo.go
Or maybe we could correct it in a separate PR and properly test the change there. I'm good either way.
There was a problem hiding this comment.
Sure. I didn't notice the repetition. Collapsed. This section of code has 100% test coverage. I'm confident it's a safe change.
guscarreon
approved these changes
Nov 17, 2021
| return syncerURL | ||
| } | ||
|
|
||
| return hostConfigURL |
| givenSyncerEndpointURL: "", | ||
| givenSyncerURL: "", | ||
| givenHostConfigURL: "c", | ||
| expected: "c", |
| @@ -1102,6 +1102,7 @@ func setBidderDefaults(v *viper.Viper, bidder string) { | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.redirect.redirect_url") | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.redirect.external_url") | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.redirect.user_macro") | |||
| v.BindEnv(adapterCfgPrefix + ".usersync.external_url") | |||
There was a problem hiding this comment.
Sure, I can live with that.
| // ExternalURL is available as a macro to the RedirectURL template. If not specified, the host configuration | ||
| // value is used. | ||
| // ExternalURL is available as a macro to the RedirectURL template. If not specified, either the syncer configuration | ||
| // value or the host configuration value is used. |
VeronikaSolovei9
approved these changes
Nov 19, 2021
wwwyyy
pushed a commit
to wwwyyy/prebid-server
that referenced
this pull request
Dec 20, 2021
* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits) Add GPID Reserved Bidder Name (prebid#2107) Marsmedia adapter - param zoneId can get string and int (prebid#2105) Algorix: add Server Region Support (prebid#2096) New Adapter: Bizzclick (prebid#2056) Collect Prometheus Go process metrics (prebid#2101) Added channel support (prebid#2037) Rubicon: update fpd fields resolving (prebid#2091) Beachfront - Currency conversion (prebid#2078) New Adapter: Groupm (Alias Of PubMatic) (prebid#2042) Adform adapter lacked gross/net parameter support (prebid#2084) add iframe sync for openx (prebid#2094) changes to the correct user sync url (prebid#2092) Smaato: Add instl to tests (prebid#2089) Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087) Adview: adapter currency fix. (prebid#2060) New Adapter: Coinzilla (prebid#2074) Syncer Level ExternalURL Override (prebid#2072) 33Across: Enable Support for SRA requests (prebid#2079) Currency conversion on adapter JSON tests (prebid#2071) New Adapter: VideoByte (prebid#2058) ...
jorgeluisrocha
pushed a commit
to jwplayer/prebid-server
that referenced
this pull request
Sep 28, 2022
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allows a host to override the external url at the syncer level in addition to the existing syncer endpoint level. This will make it easier for the host to configure an override since the endpoint type is no longer required, but is still available if different endpoints require different values.