New Adapter: MadSense #4169
Merged
New Adapter: MadSense #4169
Conversation
This file contains hidden or 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
7 tasks
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
|
@pm-jaydeep-mohite can you please review? |
| @@ -0,0 +1,12 @@ | |||
| endpoint: "https://ads.madsense.io/pbs" | |||
There was a problem hiding this comment.
Endpoint is reachable
curl -i --location --request POST https://ads.madsense.io/pbs
HTTP/2 200
content-length: 0
| return openrtb_ext.BidTypeBanner, nil | ||
| case openrtb2.MarkupVideo: | ||
| return openrtb_ext.BidTypeVideo, nil | ||
| default: |
There was a problem hiding this comment.
UT can be added to cover default case
There was a problem hiding this comment.
added UT for default case
adapters/madsense/madsense.go
Outdated
| bidErrors = append(bidErrors, err) | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
Nitpick: Remove blank line
adapters/madsense/utils.go
Outdated
Comment on lines
4
to
10
| "fmt" | ||
| "github.com/prebid/openrtb/v20/openrtb2" | ||
| "github.com/prebid/prebid-server/v3/adapters" | ||
| "github.com/prebid/prebid-server/v3/errortypes" | ||
| "github.com/prebid/prebid-server/v3/openrtb_ext" | ||
| "github.com/prebid/prebid-server/v3/util/jsonutil" | ||
| "net/http" |
There was a problem hiding this comment.
Can be re-ordered as
`
"fmt"
"net/http"
"github.com/prebid/openrtb/v20/openrtb2"
"github.com/prebid/prebid-server/v3/adapters"
"github.com/prebid/prebid-server/v3/errortypes"
"github.com/prebid/prebid-server/v3/openrtb_ext"
"github.com/prebid/prebid-server/v3/util/jsonutil"
`
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
|
@pm-jaydeep-mohite I addressed all comments, could you please review again? |
bsardo
reviewed
Feb 25, 2025
adapters/madsense/madsensetest/supplemental/bad-request-example.json
Outdated
Show resolved
Hide resolved
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
scr-oath
requested changes
Feb 28, 2025
adapters/madsense/madsense.go
Outdated
| } | ||
|
|
||
| request.Imp = imps | ||
| body, err := json.Marshal(request) |
There was a problem hiding this comment.
Please use jsonutil for marshaling
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
bsardo
reviewed
Mar 10, 2025
There was a problem hiding this comment.
@madsenseops this is close. Please see my three earlier comments that are still unresolved.
adapters/madsense/madsense.go
Outdated
Comment on lines
76
to
78
| if request.Test == 1 { | ||
| ext.CompanyId = "test" | ||
| } |
There was a problem hiding this comment.
Please see my earlier comment.
adapters/madsense/utils.go
Outdated
Comment on lines
18
to
20
| return nil, &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Error while decoding extImpBidder, err: %v", err), | ||
| } |
There was a problem hiding this comment.
Please add a supplemental test to cover this case.
adapters/madsense/utils.go
Outdated
Comment on lines
26
to
28
| return nil, &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Error while decoding impExt, err: %v", err), | ||
| } |
There was a problem hiding this comment.
Please add a supplemental test to cover this case.
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
scr-oath
requested changes
Mar 11, 2025
adapters/madsense/madsense.go
Outdated
Comment on lines
53
to
55
| for _, bannerImp := range bannerImps { | ||
| appendReq([]openrtb2.Imp{bannerImp}) | ||
| } |
There was a problem hiding this comment.
suggestion: if merely appending, perhaps you wanted to just take sub-slice?
Suggested change
| for _, bannerImp := range bannerImps { | |
| appendReq([]openrtb2.Imp{bannerImp}) | |
| } | |
| for i := range bannerImps { | |
| appendReq(bannerImps[i : i+1]) | |
| } |
There was a problem hiding this comment.
done, moved to the loop before
adapters/madsense/madsense.go
Outdated
| } | ||
|
|
||
| var videoImps, bannerImps []openrtb2.Imp | ||
| for _, imp := range request.Imp { |
There was a problem hiding this comment.
Here as well, you appear to make a copy of the slice, perhaps you can use a reference to avoid the copy?
Suggested change
| for _, imp := range request.Imp { | |
| for i := range request.Imp { | |
| imp := &request.Imp[i] |
adapters/madsense/madsense.go
Outdated
Comment on lines
113
to
115
| for _, seatBid := range resp.SeatBid { | ||
| for i := range seatBid.Bid { | ||
| bid := &seatBid.Bid[i] |
There was a problem hiding this comment.
suggestion: Avoid copy of the seatbid as well
Suggested change
| for _, seatBid := range resp.SeatBid { | |
| for i := range seatBid.Bid { | |
| bid := &seatBid.Bid[i] | |
| for i := range resp.SeatBid { | |
| seatBid := &resp.SeatBid[i] | |
| for j := range seatBid.Bid { | |
| bid := &seatBid.Bid[j] |
adapters/madsense/utils.go
Outdated
Comment on lines
36
to
38
| headers.Add("Content-Type", "application/json;charset=utf-8") | ||
| headers.Add("Accept", "application/json") | ||
| headers.Add("X-Openrtb-Version", "2.6") |
There was a problem hiding this comment.
question: Was "Add" intentional? Isn't Set the right method to use?
Suggested change
| headers.Add("Content-Type", "application/json;charset=utf-8") | |
| headers.Add("Accept", "application/json") | |
| headers.Add("X-Openrtb-Version", "2.6") | |
| headers.Set("Content-Type", "application/json;charset=utf-8") | |
| headers.Set("Accept", "application/json") | |
| headers.Set("X-Openrtb-Version", "2.6") |
There was a problem hiding this comment.
changed to Set
adapters/madsense/utils.go
Outdated
|
|
||
| if request.Device != nil { | ||
| if len(request.Device.UA) > 0 { | ||
| headers.Add("User-Agent", request.Device.UA) |
There was a problem hiding this comment.
Suggested change
| headers.Add("User-Agent", request.Device.UA) | |
| headers.Set("User-Agent", request.Device.UA) |
adapters/madsense/utils.go
Outdated
| } | ||
|
|
||
| if len(request.Device.IP) > 0 { | ||
| headers.Add("X-Forwarded-For", request.Device.IP) |
There was a problem hiding this comment.
Suggested change
| headers.Add("X-Forwarded-For", request.Device.IP) | |
| headers.Set("X-Forwarded-For", request.Device.IP) |
adapters/madsense/utils.go
Outdated
| } | ||
|
|
||
| if len(request.Device.IPv6) > 0 { | ||
| headers.Add("X-Forwarded-For", request.Device.IPv6) |
There was a problem hiding this comment.
Suggested change
| headers.Add("X-Forwarded-For", request.Device.IPv6) | |
| headers.Set("X-Forwarded-For", request.Device.IPv6) |
adapters/madsense/utils.go
Outdated
| } | ||
|
|
||
| if len(request.Device.IPv6) > 0 { | ||
| headers.Add("X-Forwarded-For", request.Device.IPv6) |
There was a problem hiding this comment.
question: do we need to preserve existing values of X-Forwarded-For? It's usually a CSV list of ip-addresses seen - this behavior puts the last hop only and wouldn't actually be correct, if behind edge servers or the like which add a terminating hop
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
There was a problem hiding this comment.
Can we access the existing values? If so, how?
adapters/madsense/utils.go
Outdated
|
|
||
| if request.Site != nil { | ||
| if request.Site.Domain != "" { | ||
| headers.Add("Origin", request.Site.Domain) |
There was a problem hiding this comment.
Suggested change
| headers.Add("Origin", request.Site.Domain) | |
| headers.Set("Origin", request.Site.Domain) |
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
bsardo
reviewed
Mar 20, 2025
adapters/madsense/utils.go
Outdated
|
|
||
| if err := jsonutil.Unmarshal(imp.Ext, &bidderExt); err != nil { | ||
| return nil, &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Error while decoding extImpBidder, err: %v", err), |
There was a problem hiding this comment.
I think your error messages are backwards. This one should read "Error while decoding extImp, err: %v"
There was a problem hiding this comment.
Thank you, changed
adapters/madsense/utils.go
Outdated
| err := jsonutil.Unmarshal(bidderExt.Bidder, &ext) | ||
| if err != nil { | ||
| return nil, &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Error while decoding impExt, err: %v", err), |
There was a problem hiding this comment.
I think your error messages are backwards. This one should read "Error while decoding extImpBidder, err: %v".
There was a problem hiding this comment.
Thank you, changed
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
scr-oath
requested changes
Mar 24, 2025
adapters/madsense/madsense_test.go
Outdated
Comment on lines
14
to
16
| if buildErr != nil { | ||
| t.Fatalf("Builder returned unexpected error %v", buildErr) | ||
| } |
There was a problem hiding this comment.
suggestion: Prefer the testify assertions/requirements
Suggested change
| if buildErr != nil { | |
| t.Fatalf("Builder returned unexpected error %v", buildErr) | |
| } | |
| require.NoError(t, buildErr, "Builder returned unexpected error") |
There was a problem hiding this comment.
Thank you, changed in this and params test
Code coverage summaryNote:
madsenseRefer here for heat map coverage report |
bsardo
approved these changes
Apr 7, 2025
scr-oath
approved these changes
Apr 7, 2025
shunj-nb
pushed a commit
to ParticleMedia/prebid-server
that referenced
this pull request
Jun 13, 2025
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.
doc - prebid/prebid.github.io#5824