Fix: Inject default DSA before auction request is rebuilt #3639
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
VeronikaSolovei9
approved these changes
Apr 25, 2024
Merged
scr-oath
reviewed
May 31, 2024
| if err != nil { | ||
| return nil, err | ||
| } | ||
| channelEnabled := r.TCF2Config.ChannelEnabled(channelTypeMap[r.LegacyLabels.RType]) |
There was a problem hiding this comment.
This causes panic for the /openrtb2/video endpoint now.
There was a problem hiding this comment.
Could you please share a stack trace?
There was a problem hiding this comment.
Ah, nevermind, A pull request to fix it is better,. :)
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.
In v2.15.0, #3540 was added which would inject a default DSA into the auction request prior to splitting it into bid requests. This was done using the request wrapper and a new call was made to the wrapper
RebuildRequestfunction to sync the underlyingBidRequestobject with the wrapper ext objects that may have been modified by PBS-Core. All of this was happening within the exchangecleanOpenRTBRequestsfunction. This change introduced a bug in the stored bid response logic which runs incleanOpenRTBRequestsand works by removing any impressions from the original bid request that map to a stored bid response.RebuildRequestshould not be called after this bid response logic as by the timecleanOpenRTBRequestsruns, the assumption is that all modifications to the auction request by PBS-Core are complete and the wrapper is in sync.As a result, the default DSA logic and the GDPR logic required to determine whether to write the default DSA has been moved up a level prior to a preexisting
RebuildRequestcall intended to sync the wrapper after modifications by PBS-Core and before the call to `cleanOpenRTBRequests.Regarding the test changes made here, I deleted a bunch of test cases in
TestCleanOpenRTBRequestsGDPRas they are no longer applicable now thatgdprEnforcedis computed a level above. The deleted test cases were testing that the proper priority was being given to the account vs host gdpr enabled configurations when calculatinggdprEnforced. We shouldn't need additional test cases in the exchange even though the logic was moved there because this logic should be adequately tested via unit test ingdpr/aggregated_config_test.go#TestIntegrationEnabled.And we also already have a couple of exchange JSON tests that verify the logic is executed. Some of those GDPR JSON tests,
exchange/exchangetest/gdpr-*.json, set the host config gdpr enabled flag.