New Adapter: Matterfull#4343
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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. |
| // 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.
| 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?
| "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?
| 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? |
| 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.
| 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.
|
|
||
| // 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.
| 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.
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
Hi @SyntaxNode @hhhjort @scr-oath pls check the updated pr |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
Hi team @SyntaxNode , @hhhjort , @scr-oath |
|
Hi @SyntaxNode , @scr-oath , @bsardo , @hhhjort Just a gentle follow-up on this PR. All review comments have been addressed, and the adapter is now fully ready (including 100% test coverage). Would really appreciate a re-review and approval when you have time |
|
Hi @ShriprasadM , @scr-oath , @bsardo , @hhhjort Quick ping on this PR — all review comments have been addressed and tests are complete. |
scr-oath
left a comment
scr-oath
left a comment
There was a problem hiding this comment.
Re-review (re-requested by @VolodymyrKapatsyn / @Matterfull).
The bottom line: Adapter code is in great shape — but this PR includes an out-of-scope CI workflow change that should be split out before merge.
What's improved since the prior approval (which was auto-dismissed by the master merge)
- Coverage rose 85.3% → 92.6%. New tests cover previously-uncovered paths:
buildEndpointURLfailure,jsonutil.Marshalfailure (NaN),compatBannerImpression(missing/single/multi-format incl. deep-copy verification),compatImpressionwithout banner,getMediaTypeForImpID(banner/video/missing). - The
multiple formats deep copysubtest atmatterfull_test.go:146mutatesoriginalFormats[1]after the call to prove the copy is independent — exactly right. - v3 → v4 import bumps came in via the master merge (#4710); mechanical and correct.
- All adapter code is otherwise unchanged from the previously-approved state.
Blocker: out-of-scope change
.github/workflows/adapter-code-coverage.yml is modified by commit dc2ae180 ("Implement retry logic for pushing coverage preview branch"). That commit is not on prebid/master — it was authored on the matterfull branch and is being carried into this PR. Adapter PRs should be scoped to the adapter; CI/infrastructure changes deserve their own PR so the right reviewers (CI owners) see them.
The retry logic itself looks reasonable for handling concurrent pushes to coverage-preview, and may even be welcomed as a standalone PR — but please drop it from this branch (e.g. git rebase -i and remove dc2ae180c, or push without that commit) and open it separately.
Praise
- Responsive iteration through many rounds. The diff is finally clean and well-tested.
- Good test design: subtests with descriptive names, deep-copy verification proves the invariant rather than just the visible output.
One non-blocking note (carried over)
matterfull.go:203 — outer for _, seatBid := range bidResp.SeatBid still copies the SeatBid struct. iterutil.SlicePointerValues(bidResp.SeatBid) would match the inner loop on line 204 for consistency. Not blocking.
Once dc2ae180c is dropped, this is ready to merge from my side. Sorry for the long round-trip.
| git add $directory/* | ||
| git commit -m 'Add coverage files' | ||
| git push origin coverage-preview | ||
| for attempt in 1 2 3; do |
There was a problem hiding this comment.
issue (blocking): Out-of-scope CI workflow change — please split into a separate PR.
This block (and commit dc2ae180) is not on prebid/master and is unrelated to the matterfull adapter. Adapter PRs should only touch their adapter; CI/infra changes deserve their own PR so the right reviewers see them.
The retry logic itself looks reasonable for concurrent pushes to coverage-preview — feel free to open it separately and it can be evaluated on its own merits.
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
@scr-oath @SyntaxNode @hhhjort thanks for the review. I pushed updates addressing the requested changes in commit 5054430.
Verified with go test ./adapters/matterfull. |
|
@scr-oath I realized I may have missed the nuance in your earlier comment about dropping commit dc2ae18 itself, not only removing the workflow change from the final diff. Right now the out-of-scope CI workflow change is removed from the PR files: My understanding is that this should address the practical blocker because the PR no longer changes CI/infra code. If you strongly prefer the branch history to be cleaned up so dc2ae18 is no longer present at all, I can do that as well with a history rewrite/force-push. I avoided doing that initially because the PR has a long history with prior merges and reviews, and the final diff is already scoped back to the adapter. |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
Code coverage summaryNote:
matterfullRefer here for heat map coverage report |
|
@scr-oath @SyntaxNode @hhhjort gentle follow-up on this PR when you have a chance. The latest checks are passing, and the final PR diff no longer includes the out-of-scope CI workflow change. I also left a note above about the remaining question around commit dc2ae18: if the clean final diff is sufficient, I think this should be ready for re-review; if you would still prefer that commit removed from the branch history entirely, I can do that as well. Thanks again for your time. |
10 participants
Docs PR - prebid/prebid.github.io#6042