New Adapter: Matterfull #4343
New Adapter: Matterfull #4343
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
adapters/matterfull/matterfull.go
Outdated
| return nil, fmt.Errorf("unable to parse endpoint url template: %v", err) | ||
| } | ||
|
|
||
| bidder := &MatterfullAdapter{ |
There was a problem hiding this comment.
You can call this simply "adapter", the MatterfullAdapter identification is already supplied by the package name. As you have it, referencing your adapter from outside the package would be MatterfullAdapter.MatterfullAdapter which looks a little redundant. See example below:
package foo
type adapter struct {
endpoint string
}
func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {
return &adapter{endpoint: "https://www.foo.com"}, nil
}
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
@ShriprasadM can you please review? |
|
@ShriprasadM Hi! Can you please tell me the status of our adapter and what we need to do next? |
|
@ShriprasadM Please let us know if you need anything from our side in order to complete the review. |
adapters/matterfull/matterfull.go
Outdated
| // MakeRequests prepares request information for prebid-server core | ||
| func (adapter *adapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
| errs := make([]error, 0, len(request.Imp)) | ||
| if len(request.Imp) == 0 { |
There was a problem hiding this comment.
While I appreciate the defensive programming, this check is not necessary. Adapters will always be called with at least one impression.
adapters/matterfull/matterfull.go
Outdated
| errors = append(errors, err) | ||
| continue | ||
| } | ||
| if res[*impExt] == nil { |
There was a problem hiding this comment.
This line may not be necessary, as append will create a slice if it's nil. Can you try removing this check?
static/bidder-params/matterfull.json
Outdated
| "description": "A schema which validates params accepted by the Matterfull adapter", | ||
| "type": "object", | ||
| "properties": { | ||
| "pubid": { |
There was a problem hiding this comment.
Should this be pid to match the required field?
adapters/matterfull/matterfull.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func getImpressionExt(imp *openrtb2.Imp) (*openrtb_ext.ExtImpMatterfull, error) { |
There was a problem hiding this comment.
Consider returning a value type instead of a reference to simplify the map key in the context above. It's a good approach in Go to stick with value types by default and only move to pointers if it needs to be modified or is very large.
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
@Matterfull : Was busy with some other work. I will look into this today |
|
@ShriprasadM Do we have any updates? |
adapters/matterfull/matterfull.go
Outdated
| resImps := make([]openrtb2.Imp, 0, len(imps)) | ||
| res := make(map[openrtb_ext.ExtImpMatterfull][]openrtb2.Imp) | ||
|
|
||
| for _, imp := range imps { |
There was a problem hiding this comment.
suggestion: avoid shallow copies of imps in a loop.
| for _, imp := range imps { | |
| for imp := range iterutil.SliceValuePointers(imps) { |
There was a problem hiding this comment.
Can you please help me with this situation, I can't import iterutil because the folder name and package name are different.
There was a problem hiding this comment.
@MaksymTeqBlaze Can you please assist with the question?
There was a problem hiding this comment.
UPDATE #4447 is merged now, please merge latest with your fork/branch and it should work now.
adapters/matterfull/matterfull.go
Outdated
| for i := 0; i < len(seatBid.Bid); i++ { | ||
| bid := seatBid.Bid[i] | ||
| bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{ |
There was a problem hiding this comment.
suggestion: Can you comment why you need a copy of the seatBid? Is it being altered in some way? Could you use iterutils.SliceValuePointers to get the pointer to the bid that's in the seatBid.Bid without needing the shallow-copy?
There was a problem hiding this comment.
You are right, a copy of seatBid is not needed here, I will rework it to use iterutils.SliceValuePointers.
adapters/matterfull/matterfull.go
Outdated
|
|
||
| // getMediaTypeForImp figures out which media type this bid is for | ||
| func getMediaTypeForImpID(impID string, imps []openrtb2.Imp) openrtb_ext.BidType { | ||
| for _, imp := range imps { |
There was a problem hiding this comment.
suggestion: Do you need shallow copies of the imps here? Can you use iterutils.SliceValuePointers
| return "", err | ||
| } | ||
| if bidExt.Prebid != nil { | ||
| return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)) |
There was a problem hiding this comment.
Consider this as a suggestion. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, recommends implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
|
@scr-oath I fixed the errors according to the last code review. Tell me, I think I incorrectly brought to the current version of the repository and pushed. |
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
Hmm… I started to review changes in other adapters and then realized - is this merged properly? Why are we picking up other changes and not merely changes to the materfull adapter?
| bidderResponse.Currency = bidResponse.Cur | ||
| } | ||
|
|
||
| for _, seatBid := range bidResponse.SeatBid { |
There was a problem hiding this comment.
suggestion: Please avoid deep copies; consider using iterutil.SlicePointerValues here
Note that an iterator over a slice is merely assigned the value of each element and, since each is a (large) struct, it's less work to use the pointer. The iterutil helps with this.
| for _, seatBid := range bidResponse.SeatBid { | |
| for seatBid := range iterutil.SlicePointerValues(bidResponse.SeatBid) { |
| for i := range seatBid.Bid { | ||
| bid := seatBid.Bid[i] |
There was a problem hiding this comment.
suggestion: You can also use iterutil here for both of these lines and, in fact, what you have here is still a copy. I'm not sure if that was intended or not - but you are shallow copying the element in this assignment.
There was a problem hiding this comment.
Outdated comment from another commit. Not relevant now.
| bidExt, bidExtErr := getBidExt(bid.Ext) | ||
| if bidExtErr != nil { | ||
| errs = append(errs, &errortypes.FailedToUnmarshal{ | ||
| Message: fmt.Errorf("bid ext, err: %w", bidExtErr).Error(), |
There was a problem hiding this comment.
suggestion (style):
Use %w to wrap errors only when you need to return or propagate an error type, allowing callers to use errors.Is/errors.As for error inspection. Here, since the formatted error message is being converted to a string (not returned as an error), %v is more appropriate. Wrapping with %w is only meaningful in functions that return an error value.
Reference: https://golang.org/pkg/fmt/#hdr-Printing
Example:
// Good: wrapping when returning an error
return fmt.Errorf("context: %w", err)
// Good: using %v when embedding error in a string
Message: fmt.Sprintf("bid ext, err: %v", bidExtErr)suggestion - why do this and not merely Sprintf?
| Message: fmt.Errorf("bid ext, err: %w", bidExtErr).Error(), | |
| Message: fmt.Sprintf("bid ext, err: %v", bidExtErr), |
There was a problem hiding this comment.
Outdated comment from another commit. Not relevant now.
| if err != nil { | ||
| t.Fatalf("Builder returned unexpected error %v", err) | ||
| } |
There was a problem hiding this comment.
Please use testify
| if err != nil { | |
| t.Fatalf("Builder returned unexpected error %v", err) | |
| } | |
| require.NoError(t, "Builder returned unexpected error") |
There was a problem hiding this comment.
Outdated comment from another commit. Not relevant now.
adapters/conversant/conversant.go
Outdated
| if imp.Video != nil { | ||
| bidType = openrtb_ext.BidTypeVideo | ||
| } | ||
| if imp.Audio != nil { | ||
| bidType = openrtb_ext.BidTypeAudio | ||
| switch { | ||
| case imp.Native != nil: | ||
| return openrtb_ext.BidTypeNative | ||
| case imp.Audio != nil: | ||
| return openrtb_ext.BidTypeAudio | ||
| case imp.Video != nil: | ||
| return openrtb_ext.BidTypeVideo |
There was a problem hiding this comment.
question: why are these diffs showing up? Is there some way this could be merged with main so that the files diffed here are only your changes?
There was a problem hiding this comment.
@scr-oath
I don't know what to do in this situation, I downloaded the changes from the master to my branch, because I needed the current version of the utilities, and all the commits that were during this time appeared. Tell me, maybe I should recreate the pull request from the current version?
There was a problem hiding this comment.
I'll pull this down and take a look… FYI, I enjoy using https://github.com/git-up/GitUp which is a great tool to visualize local branches…
There was a problem hiding this comment.
Hmm… These files don't show up in its view of the diff… I wonder why GH is getting confused…
There was a problem hiding this comment.
says:
git fetch origin
git merge origin/master
# or alternatively:
# git rebase origin/master
git push --force-with-lease
| return "", err | ||
| } | ||
| if bidExt.Prebid != nil { | ||
| return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type)) |
There was a problem hiding this comment.
Consider this as a suggestion. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, recommends implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
There was a problem hiding this comment.
Outdated comment from another commit. Not relevant now.
|
@scr-oath We have checked and responded to all previous tasks to improve the adapter. The latest found defects have been fixed. Please review the code again. |
|
@scr-oath Please check our latest changes. |
|
@scr-oath Can you please tell us what to do next? |
adapters/matterfull/matterfull.go
Outdated
| bannerCopy := *imp.Banner | ||
| banner := &bannerCopy | ||
| //As banner.w/h are required fields for Matterfull platform - take the first format entry | ||
| if banner.W == nil || banner.H == nil { |
There was a problem hiding this comment.
suggestion: no need to copy unless you need to; move these into the if block
| bannerCopy := *imp.Banner | |
| banner := &bannerCopy | |
| //As banner.w/h are required fields for Matterfull platform - take the first format entry | |
| if banner.W == nil || banner.H == nil { | |
| //As banner.w/h are required fields for Matterfull platform - take the first format entry | |
| if banner.W == nil || banner.H == nil { | |
| bannerCopy := *imp.Banner | |
| banner := &bannerCopy |
… headers, move creating
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
adapters/matterfull/matterfull.go
Outdated
| } | ||
| var bidResp openrtb2.BidResponse | ||
| if err := jsonutil.Unmarshal(response.Body, &bidResp); err != nil { | ||
| msg = fmt.Sprintf("Bad server response: %d", err) |
There was a problem hiding this comment.
It doesn't look like err is an integer (%d)
| func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) { | ||
| urlTemplate, err := template.New("endpointTemplate").Parse(config.Endpoint) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to parse endpoint url template: %v", err) |
There was a problem hiding this comment.
When wrapping another err, use %w
| return nil, fmt.Errorf("unable to parse endpoint url template: %v", err) | |
| return nil, fmt.Errorf("unable to parse endpoint url template: %w", err) |
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Claude Code
Summary
The Matterfull adapter implementation is well-structured and follows Prebid Server patterns correctly. However, there are 3 critical issues that must be fixed before merging, along with some style improvements.
Critical Issues ⚠️
- Memory Safety: Direct mutation of input
prebidBidRequestcan cause side effects for other adapters - Error Handling: Only accepting exactly 1 SeatBid is too restrictive - real SSPs return 0 (no bids) or multiple SeatBids
- String Formatting Bug: Using
%dfor error formatting will cause runtime issues
Build Failures 🔨
The test failures you're seeing are likely related to the analytics module, not your adapter code. However, the above fixes will make your adapter more robust.
Strengths ✅
- Good test coverage (88.3%)
- Proper impression grouping by publisher ID
- Correct banner format handling
- Appropriate error types used
- Follows standard Prebid adapter patterns
Next Steps
- Fix the 3 critical issues highlighted in the line comments
- Address the style issues (variable naming, typo)
- The adapter should then be ready for merge
Great work overall! The core functionality is solid, just needs these safety improvements.
adapters/matterfull/matterfull.go
Outdated
| } | ||
|
|
||
| func createBidRequest(prebidBidRequest *openrtb2.BidRequest, params *openrtb_ext.ExtImpMatterfull, imps []openrtb2.Imp) *openrtb2.BidRequest { | ||
| prebidBidRequest.Imp = imps |
There was a problem hiding this comment.
🤖 Reviewed by Claude Code
Critical: Memory Safety Issue
This directly mutates the input prebidBidRequest, which can cause side effects for other adapters processing the same request. You need to create a deep copy first:
func createBidRequest(prebidBidRequest *openrtb2.BidRequest, params *openrtb_ext.ExtImpMatterfull, imps []openrtb2.Imp) *openrtb2.BidRequest {
// Create a deep copy to avoid mutating the original request
reqCopy := *prebidBidRequest
newBidRequest := &reqCopy
newBidRequest.Imp = imps
if newBidRequest.Site != nil {
siteCopy := *newBidRequest.Site
newBidRequest.Site = &siteCopy
newBidRequest.Site.Publisher = nil
newBidRequest.Site.Domain = ""
}
if newBidRequest.App != nil {
appCopy := *newBidRequest.App
newBidRequest.App = &appCopy
newBidRequest.App.Publisher = nil
}
return newBidRequest
}
adapters/matterfull/matterfull.go
Outdated
| msg = fmt.Sprintf("Bad server response: %d", err) | ||
| return nil, []error{&errortypes.BadServerResponse{Message: msg}} | ||
| } | ||
| if len(bidResp.SeatBid) != 1 { |
There was a problem hiding this comment.
🤖 Reviewed by Claude Code
Critical: Inflexible Error Handling
This code only accepts exactly 1 SeatBid, but real SSPs often return:
- 0 SeatBids (no bids available) - should return
nil, nil - Multiple SeatBids (multiple demand sources) - should process all
Fix:
if len(bidResp.SeatBid) == 0 {
return nil, nil // No bids
}
bidResponse := adapters.NewBidderResponse()
for _, seatBid := range bidResp.SeatBid {
for bid := range iterutil.SlicePointerValues(seatBid.Bid) {
bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Bid: bid,
BidType: getMediaTypeForImpID(bid.ImpID, internalRequest.Imp),
})
}
}
return bidResponse, nil
adapters/matterfull/matterfull.go
Outdated
| } | ||
| var bidResp openrtb2.BidResponse | ||
| if err := jsonutil.Unmarshal(response.Body, &bidResp); err != nil { | ||
| msg = fmt.Sprintf("Bad server response: %d", err) |
There was a problem hiding this comment.
🤖 Reviewed by Claude Code
Bug: Incorrect String Formatting
Using %d format specifier for an error type will cause formatting issues. Use %s or %v:
msg = fmt.Sprintf("Bad server response: %s", err)or better yet:
msg = fmt.Sprintf("Bad server response: %v", err)
adapters/matterfull/matterfull.go
Outdated
| Message: err.Error(), | ||
| } | ||
| } | ||
| var MatterfullExt openrtb_ext.ExtImpMatterfull |
There was a problem hiding this comment.
🤖 Reviewed by Claude Code
Style: Variable Naming Convention
Variable names in Go should follow camelCase convention:
var matterfullExt openrtb_ext.ExtImpMatterfull
if err := jsonutil.Unmarshal(bidderExt.Bidder, &matterfullExt); err != nil {
return openrtb_ext.ExtImpMatterfull{}, &errortypes.BadInput{
Message: err.Error(),
}
}
return matterfullExt, nil
adapters/matterfull/matterfull.go
Outdated
| return prebidBidRequest | ||
| } | ||
|
|
||
| // Builds enpoint url based on adapter-specific pub settings from imp.ext |
There was a problem hiding this comment.
🤖 Reviewed by Claude Code
Typo in Comment
// Builds endpoint url based on adapter-specific pub settings from imp.ext(missing "d" in "endpoint")
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
@scr-oath We've taken all your suggestions for improving code quality and made the final changes. Could you please review the code? We hope this will be the last one before the merge. |
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
Comprehensive review of the Matterfull adapter implementation using golang-pro and code-reviewer agents.
Summary
Found 3 critical issues and 4 high-priority issues that should be addressed before merging.
Critical Issues (Must Fix) 🔴
-
Typo in JSON schema (
static/bidder-params/matterfull.json:3)- "Metterfull" should be "Matterfull"
-
Email domain inconsistency (
static/bidder-info/matterfull.yaml:7)- Email uses
@bematterfull.combut bidder ismatterfull - Please verify correct email domain
- Email uses
-
Endpoint URL mismatch
- YAML config:
https://prebid.matterfull.co/ - Tests use:
http://us.matterfull.co/ - Need to align these or document regional endpoints
- YAML config:
High Priority Issues (Should Fix) 🟡
-
Error handling loses partial results (
adapters/matterfull/matterfull.go:36-45)- When one publisher fails, function returns
nilfor ALL results - Should
continueinstead ofreturn nil, errs
- When one publisher fails, function returns
-
Missing runtime validation
- Add runtime check for empty
pidlike other adapters
- Add runtime check for empty
-
Potential state mutation (
adapters/matterfull/matterfull.go:90)- Shallow copy doesn't prevent slice mutation
- Add comment or deep copy remaining formats
-
Unused parameter (
adapters/matterfull/matterfull.go:115)paramsonly used for URL building- Either remove or document why unused
Medium Priority 📋
- Use traditional iteration instead of
iterutil.SlicePointerValues - Fix comment formatting (line 60)
- Type/field naming inconsistency
- Missing package documentation
Positive Observations ⭐
- ✅ Good test coverage
- ✅ Proper use of error types
- ✅ Correct impression grouping
- ✅ Template-based endpoints
See detailed review document: PR_4343_REVIEW.md in the conductor workspace.
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
CRITICAL - Error Handling Bug
adapters/matterfull/matterfull.go:40
When building requests for multiple publishers, if one fails, this returns nil for ALL results, discarding previously successful requests:
if err != nil {
errs = append(errs, err)
return nil, errs // ❌ Loses all previous successful requests
}Fix: Change to continue to return partial results:
if err != nil {
errs = append(errs, err)
continue // ✅ Continue processing other publishers
}This way if 3 publishers succeed and 1 fails, you return the 3 successful requests with 1 error, which is the expected prebid-server behavior.
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
State Mutation Issue
adapters/matterfull/matterfull.go:90
The shallow copy doesn't prevent slice mutation:
banner.Format = banner.Format[1:] // Modifies backing arrayIssue: bannerCopy := *imp.Banner creates a shallow copy. The banner.Format slice shares the underlying array with the original impression.
Options:
- Deep copy remaining formats:
if len(banner.Format) > 1 {
banner.Format = append([]openrtb2.Format{}, banner.Format[1:]...)
} else {
banner.Format = nil
}- Or add explanatory comment (as LunaMedia does):
// Create a copy of the banner, since imp is a shallow copy of the original.
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
Typo in JSON Schema
static/bidder-params/matterfull.json:3
"title": "Metterfull Adapter Params", // ❌ Should be "Matterfull"Please fix the typo.
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
Email Domain Inconsistency
static/bidder-info/matterfull.yaml:7
maintainer:
email: "[email protected]" # ❌ bematterfull vs matterfullThe email uses @bematterfull.com domain while the bidder is named matterfull.
Please verify:
- Is this the correct email domain?
- Or should it match the bidder name?
- Is there a relationship between bematterfull and matterfull that should be documented?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
@scr-oath. We checked our mail and it is absolutely correct. [email protected]. |
8 participants
Docs PR - prebid/prebid.github.io#6042