SuperEdge: add new adapter - #4846
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| default: | ||
| for _, imp := range imps { | ||
| if imp.ID == bid.ImpID { | ||
| if imp.Banner != nil { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner, nil. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
| if imp.Banner != nil { | ||
| return openrtb_ext.BidTypeBanner, nil | ||
| } | ||
| if imp.Native != nil { |
There was a problem hiding this comment.
Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative, nil. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.
|
@przemkaczmarek @jney @minaguib @dlackty @asweeney86 Please help review the code and merge it. Thanks |
Code coverage summaryNote:
superedgeRefer here for heat map coverage report |
|
@przemkaczmarek @jney @minaguib @dlackty @asweeney86 Please help review the code and merge it. Thanks |
| endpoint: "https://rtb-us.superedge.co.jp/bid?sk={{.sk}}" | ||
| endpointCompression: gzip | ||
| geoscope: | ||
| - USA | ||
| maintainer: | ||
| email: op@superedge.co.jp | ||
| capabilities: | ||
| site: | ||
| mediaTypes: | ||
| - banner | ||
| - native |
Code coverage summaryNote:
superedgeRefer here for heat map coverage report |
| geoscope: | ||
| - USA | ||
| maintainer: | ||
| email: op@superedge.co.jp |
There was a problem hiding this comment.
waiting for response
| @@ -0,0 +1,11 @@ | |||
| endpoint: "https://rtb-us.superedge.co.jp/bid?sk={{.sk}}" | |||
| @@ -0,0 +1,122 @@ | |||
| { | |||
There was a problem hiding this comment.
please add multi imps example
There was a problem hiding this comment.
Thanks, I will handle the issues with glvid and multi-imp as soon as possible.
| "page": "https://www.example.com/" | ||
| } | ||
| }, | ||
| "httpcalls": [ |
| banner.W = &firstFormat.W | ||
| banner.H = &firstFormat.H | ||
| request.Imp[i].Banner = &banner | ||
| } |
There was a problem hiding this comment.
This line mutates the caller's original BidRequest — it overwrites the
Banner pointer on the request object owned by PBS core. PBS adapters must
not modify the incoming request (copy-on-write rule). After MakeRequests
returns, downstream PBS code (logging, floors, other adapters) will see the
modified Banner.W/Banner.H values.
Fix: make a shallow copy of the request and its Imp slice before calling
preProcess:
func (a *adapter) makeRequest(request *openrtb2.BidRequest) (*adapters.RequestData, error) {
...
requestCopy := *request
requestCopy.Imp = make([]openrtb2.Imp, len(request.Imp))
copy(requestCopy.Imp, request.Imp)
preProcess(&requestCopy)
reqBody, err := jsonutil.Marshal(&requestCopy)
...
}| "$schema": "http://json-schema.org/draft-04/schema#", | ||
| "title": "SuperEdge Adapter Params", | ||
| "description": "A schema which validates params accepted by the SuperEdge adapter", | ||
| "type": "object", |
There was a problem hiding this comment.
Missing "additionalProperties": false. Without it the schema accepts objects
with arbitrary unknown fields, e.g. {"sk": "abc", "foo": "bar"} passes
validation. All other PBS adapter schemas include this constraint.
…ti-imp tests & schema
|
@przemkaczmarek All issues you mentioned — copy-on-write mutation, bid-type anti-pattern, multi-imp tests, httpCalls casing, additionalProperties, and gvlVendorID — have been addressed. Please take another look, thanks. |
Code coverage summaryNote:
superedgeRefer here for heat map coverage report |
| - USA | ||
| maintainer: | ||
| email: op@superedge.co.jp | ||
| gvlVendorID: 1554 |
|
@przemkaczmarek Thanks for the approval! It looks like we still need one more review to merge — could you help get another reviewer? |
|
@przemkaczmarek Thanks for the approval! |
|
@ChrisHuie @Taxel @postindustria-code Sorry for the ping — would any of you be available to review this PR? Much appreciated! |
|
Docs PR required (before merge). I don't see a link to a prebid.github.io Per the Prebid Server bidder guide:
|
| "required": [ | ||
| "sk" | ||
| ], | ||
| "additionalProperties": false |
There was a problem hiding this comment.
- Schema
additionalProperties:falsecontradicts the exemplary fixtures —static/bidder-params/superedge.json:16allows onlysk, but exemplaryimp.ext.bidderblocks sendregion+placementId(e.g.sample-banner-apac.json:15-21,sample-banner-euc.json:15-22,sample-banner.json,sample-nobid.json,sample-banner-fallback-*.json). Self-contradicting within the PR:params_test.go:47-48explicitly asserts{"region":"APAC"}/{"region":"US"}are invalid.- Mechanism (proven, not inferred). Ran the real PBS validator (
openrtb_ext.NewBidderParamsValidator→Validate, the same oneparams_test.gouses) against the exact fixture shape:{"sk":"abc"}→ ACCEPTED{"sk":"abc","region":"APAC"}→ REJECTED:Additional property region is not allowed{"sk":"abc","region":"APAC","placementId":"p1"}→ REJECTED: region + placementId not allowed
This closes the gap in the naive reading —{"region":"APAC"}ininvalidParamsfails on both missing-skand additionalProperties, but a validskplus the extra keys is still rejected purely byadditionalProperties:false.
- Impact is a hard 400, stronger than "imp dropped".
ortb/request_validator.go:133-135: on a params validation error itreturn []error{fmt.Errorf("request.imp[%d].ext.prebid.bidder.%s failed validation…")}— the whole bid request is rejected (400), gated only by the host'scfg.SkipBidderParams(defaultfalse= validation ON). A publisher copying the exemplaryimp.ext.biddershape gets the auction rejected, not a silent partial drop.
- Mechanism (proven, not inferred). Ran the real PBS validator (
Fix: remove region/placementId from all exemplary imp.ext.bidder blocks (the adapter reads neither) — or, if they are real params, add them to the schema + ExtSuperEdge + validParams and drop them from invalidParams. Option A matches the current schema/yaml.
| if err := jsonutil.Unmarshal(response.Body, &bidResp); err != nil { | ||
| return nil, []error{err} | ||
| } | ||
| bidResponse := adapters.NewBidderResponseWithBidsCapacity(1) |
There was a problem hiding this comment.
Response currency dropped — builds NewBidderResponseWithBidsCapacity(1) (which defaults Currency: "USD", adapters/bidder.go:75) and never reads bidResp.Cur. Latent: every exemplary uses cur: "USD" / currency: "USD", so no test exposes it — the mislabel only surfaces if the endpoint returns a non-USD cur. Fix: if bidResp.Cur != "" { bidResponse.Currency = bidResp.Cur }.
"Please avoid common mistakes, such as not specifying the bid currency and not properly detecting the media type from the bidding server response." — https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html
| func (a *adapter) getEndPoint(ext *openrtb_ext.ExtSuperEdge) (string, error) { | ||
| return macros.ResolveMacros(a.EndpointTemplate, map[string]string{"sk": ext.Sk}) | ||
| } |
There was a problem hiding this comment.
region / apac-euc-use fixtures are non-functional & misleading — getEndPoint resolves only sk; all 8 exemplaries emit the identical rtb-us... URI (confirmed). region is in no struct/schema and routes nothing. Drop it and rename/merge the fixtures, or (if geo routing is intended) implement region→host mapping.
| var invalidParams = []string{ | ||
| `{}`, | ||
| `{"tn": "0c3356713c184ca186779eecdd5aff5d"}`, | ||
| `{"region": "APAC"}`, | ||
| `{"region": "US"}`, | ||
| `{"tn": "27bb74d57068406ebcbb29ab9bfeb9b9"}`, | ||
| } |
There was a problem hiding this comment.
params_test.go missing type-mismatch / minLength cases — invalidParams covers missing-sk + unknown-field, but not {"sk":123} (type) or {"sk":""} (minLength:1). Two-line add.
"Please include tests for required fields, optional fields, conditional fields such as oneOf, regex filters, and data type mismatches." — https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html
| // Try to get sk from request.ext.prebid.bidderparams first | ||
| if request.Ext != nil { | ||
| reqExt := &openrtb_ext.ExtRequest{} | ||
| if err := jsonutil.Unmarshal(request.Ext, reqExt); err == nil { | ||
| if len(reqExt.Prebid.BidderParams) > 0 { | ||
| if err := jsonutil.Unmarshal(reqExt.Prebid.BidderParams, &extSuperEdge); err == nil && extSuperEdge.Sk != "" { | ||
| return &extSuperEdge, nil | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
- Redundant request-level
skextraction + dead guard — becauseskis schema-requiredonimp.ext.bidder, the imp path always resolves it, so therequest.ext.prebid.bidderparamsprimary path (superedge.go:78-88) is never necessary: it only adds a per-requestrequest.Extunmarshal on the hot path and a surprising precedence (request-levelsksilently overrides the per-imp value). Theif extSuperEdge.Sk != ""guard atsuperedge.go:104is dead given the schema. Consider readingskfromimp[0].ext.bidderonly. (Not a blocker.)
| @@ -0,0 +1,104 @@ | |||
| { | |||
There was a problem hiding this comment.
Fixtures that don't test what their names imply — sample-banner-fallback-to-first-imp-to-get-ep.json has no request.ext and a single imp with sk, so it uses the plain imp path — the same path as the other banner fixtures; it doesn't uniquely exercise the fallback-selection branch (which needs ext.prebid.bidderparams present-but-without-sk). bad_request_no_token.json sends imp.ext.bidder={placementId} (no sk) and asserts superEdge sk not found, but in prod such an imp is rejected by schema validation (required sk + additionalProperties:false) before the adapter runs, so that branch is unreachable in production (tied to the redundant-check item above).
| headers := http.Header{} | ||
| headers.Add("Content-Type", "application/json;charset=utf-8") | ||
| headers.Add("Accept", "application/json") | ||
| headers.Add("x-openrtb-version", "2.5") |
There was a problem hiding this comment.
x-openrtb-version: 2.5 header vs bid.MType (a 2.6 field) usage — set the header to 2.6
|
@przemkaczmarek @postindustria-code all review comments have been addressed — removed redundant sk extraction, fixed response currency and x-openrtb-version header, added region-based multi-endpoint routing (US/EU/APAC), updated test fixtures and schema. |
Code coverage summaryNote:
superedgeRefer here for heat map coverage report |
|
@LeeZXin, approved. Do not forget upgrade your branch "This branch is out-of-date with the base branch" |
@postindustria-code Thanks for the approval, and thanks for the heads-up on the branch! |
Code coverage summaryNote:
superedgeRefer here for heat map coverage report |
|
@przemkaczmarek friendly reminder — the approval was dismissed due to the latest commit. Would appreciate a re-review when you get a chance. Thanks! |
|
@bsardo could you take a look when you have a moment? we're just waiting on one more review. Happy to address any feedback. Thanks! |
|
@przemkaczmarek Thanks for approval!! |
|
@bsardo both approvals are in, could you take a final look or merge when you have a moment? Thanks! |
3 participants
New Adapter: SuperEdge
SuperEdge is a DSP/SSP provided by SuperEdge Inc.
Supported Media Types
Bidder Parameters
Endpoint
Test Parameters
{ "sk": "your-sk-here" }Contact