Fix: Triplelift Native add site.publisher nil check #3824
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
bsardo
changed the title
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
|
@patrickloughrey please take a look. I also imagine that you don't have to perform a similar check on |
Discussed offline. They don't have app inventory enabled at this time but would like to handle this case to be safe. |
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
bsardo
force-pushed
the
triplelift-native-fix
branch
from
45d1f05 to
8ff11b6
Compare
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
bsardo
commented
Jul 26, 2024
Comment on lines
-53
to
-54
| var siteCopy openrtb2.Site | ||
| var extData ExtImpData |
There was a problem hiding this comment.
As pointed out by @guscarreon, these two variables are only ever read; they aren't written so we can get rid of them and just read the data from the original source.
bsardo
commented
Jul 26, 2024
bsardo
commented
Jul 26, 2024
Comment on lines
-73
to
-82
| if ext.Data != nil { | ||
| extData = *ext.Data | ||
| } | ||
|
|
||
| if extData.TagCode != "" { | ||
| if siteCopy.Publisher.Domain == "msn.com" { | ||
| imp.TagID = extData.TagCode | ||
| } else { | ||
| imp.TagID = tlext.InvCode | ||
| } |
There was a problem hiding this comment.
This logic has been simplified as pointed out by @guscarreon.
bsardo
commented
Jul 26, 2024
Comment on lines
+80
to
+82
| func msnInApp(request *openrtb2.BidRequest) bool { | ||
| return request.App != nil && request.App.Publisher != nil && request.App.Publisher.Domain == "msn.com" | ||
| } |
There was a problem hiding this comment.
Adding support for app.publisher as discussed.
bsardo
commented
Jul 26, 2024
|
|
||
| // msnInSite returns whether msn.com is in request.site.publisher.domain | ||
| func msnInSite(request *openrtb2.BidRequest) bool { | ||
| return request.Site != nil && request.Site.Publisher != nil && request.Site.Publisher.Domain == "msn.com" |
There was a problem hiding this comment.
Nil checks added to address panics.
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
SyntaxNode
requested changes
Jul 26, 2024
Code coverage summaryNote:
triplelift_nativeRefer here for heat map coverage report |
SyntaxNode
approved these changes
Jul 26, 2024
|
LGTM |
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.
This PR fixes panics in the Triplelift Native adapter introduced in v2.25.0.