Repository navigation
New Adapter: Clickio - #4786
New Adapter: Clickio#4786ClickioTech wants to merge 25 commits into
Conversation
|
Hi @przemkaczmarek, sorry for the direct ping. Could you please take a look at this adapter PR when you have a chance, or help route it to the right reviewer? It has been open since May 20 and currently has no assigned reviewer or review comments. Happy to address anything needed to move it forward. Thanks! |
| @@ -0,0 +1,138 @@ | |||
| { | |||
There was a problem hiding this comment.
test coverage is to low, check others adapter and try addy more json examples
There was a problem hiding this comment.
Thanks for flagging the coverage. I looked at how comparable adapters are tested (median is roughly 4 exemplary + 5–6 supplemental JSON samples) and added samples aimed at our actual code paths rather than padding the count.
Exemplary (success paths):
video.json — video imp resolving to BidTypeVideo
native.json — native imp resolving to BidTypeNative
params-via-prebid.json — params sourced from imp.ext.prebid.bidder.clickio (a separate extraction branch from the top-level imp.ext.bidder)
Supplemental (edge/error paths):
status-204.json — 204 → no bids, no error
status-400.json — 400 → BadInput
status-500.json — non-200 → BadServerResponse
bad-response.json — malformed response body → unmarshal error
bad-media-type.json — imp with no media object → Failed to resolve media type
Together with the existing banner.json and the invalid-imp-ext-params.json case added for the error-propagation fix, this brings us to 4 exemplary + 6 supplemental and raises statement coverage to ~87%. Every branch in MakeRequests/MakeBids (media-type resolution, param extraction sources, and all status-code handling) is now exercised.
| - global | ||
| maintainer: | ||
| email: "github@clickio.com" | ||
| gvlVendorID: 1500 |
There was a problem hiding this comment.
are You suer its correc ID?
There was a problem hiding this comment.
Yes, our Vendor Name in TCF Vendor List is "ALZ Software Ltd"
| geoscope: | ||
| - global | ||
| maintainer: | ||
| email: "github@clickio.com" |
There was a problem hiding this comment.
waiting for response
| @@ -0,0 +1,19 @@ | |||
| endpoint: "https://o.clickiocdn.com/bids" | |||
| }, | ||
| "ext": { | ||
| "bidder": { | ||
| "said": "auction-1", |
There was a problem hiding this comment.
The expectedRequest values don't match the mockBidRequest values.
The adapter does not transform parameter values — updateImpExtWithParams
copies them verbatim from imp.ext.bidder to imp.ext.params.
mockBidRequestsets"said": "123","psid": "321"expectedRequestasserts"said": "auction-1","psid": "placement-1"
These values will never match, so TestJsonSamples will fail. Please
update the expectedRequest body to use "said": "123" and "psid": "321"
(or update the mockBidRequest to use the intended values consistently).
| BidderCadentApertureMX BidderName = "cadent_aperture_mx" | ||
| BidderCcx BidderName = "ccx" | ||
| BidderClydo BidderName = "clydo" | ||
| BidderClickio BidderName = "clickio" |
There was a problem hiding this comment.
Same ordering issue here: BidderClickio should appear before
BidderClydo in the const block to maintain alphabetical order.
| BidderCadentApertureMX, | ||
| BidderCcx, | ||
| BidderClydo, | ||
| BidderClickio, |
There was a problem hiding this comment.
BidderClickio is inserted after BidderClydo, but alphabetically
clickio (cli…) sorts before clydo (cly…). PBS requires the
coreBidderNames slice to be in alphabetical order. Please move
BidderClickio to the line before BidderClydo.
| "required": [ | ||
| "said" | ||
| ], | ||
| "additionalProperties": true |
There was a problem hiding this comment.
PBS adapter schemas should use "additionalProperties": false to reject
unexpected fields and prevent misconfigured requests from passing
validation silently. Setting it to true (even explicitly) allows
arbitrary unknown fields through the validator. Are You sure about this?
There was a problem hiding this comment.
We removed the explicit "additionalProperties": true.
That said, we deliberately don't want a strict schema here. Setting "additionalProperties": false isn't actually the PBS convention — out of 264 schemas in static/bidder-params, 258 omit the field entirely (which defaults to true under JSON Schema draft-04), and only one uses false. So the vast majority of adapters allow unknown fields.
We aligned with that majority: rather than keeping the explicit true, we dropped the field so the schema reads like everyone else's while keeping the same permissive behavior. The adapter only ever reads said/psid/template from imp.ext.bidder, so unexpected fields can't leak into the outgoing request regardless.
| }}, nil | ||
| } | ||
|
|
||
| func updateImpExtWithParams(imp *openrtb2.Imp) { |
There was a problem hiding this comment.
All JSON errors inside this function are silently swallowed with a bare
return. If any unmarshal or marshal call fails, imp.Ext is left
unmodified and the request is sent to the exchange without imp.ext.params
— including the required said field — with no error surfaced to the
caller or the publisher.
Consider returning an error from this function and propagating it
through MakeRequests as a BadInput, so callers can detect and report
the failure:
func updateImpExtWithParams(imp *openrtb2.Imp) error {
...
unmodified and the request is sent to the exchange without `imp.ext.params`
— including the required `said` field — with no error surfaced to the
caller or the publisher.
Consider returning an `error` from this function and propagating it
through `MakeRequests` as a `BadInput`, so callers can detect and report
the failure:
```go
func updateImpExtWithParams(imp *openrtb2.Imp) error {
...
if err := json.Unmarshal(imp.Ext, &impEx
return fmt.Errorf("failed to parse imp.ext for imp %s: %w", imp.ID, err)
}
...
}There was a problem hiding this comment.
We changed updateImpExtWithParams to return an error instead of swallowing failures with a bare return. Each unmarshal/marshal path now returns a wrapped error including the imp.ID (e.g. failed to parse imp.ext.params for imp %s: %w), and MakeRequests propagates it to the caller as an errortypes.BadInput, so the failure is surfaced instead of silently sending a request without imp.ext.params.
We also added a supplemental JSON test (clickiotest/supplemental/invalid-imp-ext-params.json) that feeds a malformed imp.ext.params and asserts the resulting MakeRequests error, covering the new error path.
|
@ClickioTech please check this pr #4806 and try to write as many test jsons as it is |
- banner.json: fix expectedRequest said/psid to match mockBidRequest values (adapter copies params verbatim, so 123/321 must match) - clickio.json: drop explicit additionalProperties:true to align with the majority of bidder-params schemas (no strict schema required) - clickio.go: return errors from updateImpExtWithParams and propagate them through MakeRequests as BadInput instead of silently swallowing JSON unmarshal/marshal failures; add supplemental test for a malformed imp.ext.params - bidders.go: fix alphabetical ordering of Clickio bidder entries Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover code paths that were previously untested, matching the coverage level of comparable adapters (~4 exemplary + ~6 supplemental): - exemplary: video, native (media-type resolution beyond banner) and params-via-prebid (params sourced from imp.ext.prebid.bidder.clickio) - supplemental: status-204/400/500, bad-response (malformed body) and bad-media-type (imp with no media object) Raises statement coverage to ~87%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er into clickio-adapter
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
Fix struct field alignment flagged by the gofmt CI check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er into clickio-adapter
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
|
@ClickioTech dont woory about this. Fix MR is done. |
|
Hi @przemkaczmarek @postindustria-code @bsardo, Just following up on this — it’s been about 3 weeks since the PR was approved, but we’re still unable to get it merged because the automated check is failing due to the Since the PR comes from a fork, there isn’t anything we can change on our side to resolve this — the workflow configuration needs to be adjusted in the base repository. Could you please take a look when you get a chance? We’d really appreciate getting this unblocked and merged. Thanks! |
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
|
Hi @bsardo, could you please share an ETA for unblocking the failing Both reviews are approved, but the workflow is still failing for this fork PR. @przemkaczmarek mentioned that a fix had been made, so could you please confirm what is still needed to get it merged? Thank you! |
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if imp.ID != impID { | ||
| continue | ||
| } | ||
| 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.
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
| } | ||
| if len(params) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
@ClickioTech I believe the logic on lines 55-147 spanning functions updateImpExtWithParams, extractParamFromPrebid and extractParamFromBidderExt can be greatly simplified as there is no need to check for bidder params at both imp.ext.bidder and imp.ext.prebid.bidder. Upstream logic maps any bidder params defined in imp.ext.prebid.bidder to imp.ext.bidder in exchange/utils.go#splitImps.
Something like this should suffice:
func updateImpExtWithParams(imp *openrtb2.Imp) error {
if len(imp.Ext) == 0 {
return nil
}
var impExt map[string]json.RawMessage
if err := jsonutil.Unmarshal(imp.Ext, &impExt); err != nil {
return fmt.Errorf("failed to parse imp.ext for imp %s: %w", imp.ID, err)
}
params := make(map[string]json.RawMessage)
if rawParams := impExt["params"]; len(rawParams) > 0 {
if err := jsonutil.Unmarshal(rawParams, ¶ms); err != nil {
return fmt.Errorf("failed to parse imp.ext.params for imp %s: %w", imp.ID, err)
}
}
var bidderParams map[string]json.RawMessage
if err := jsonutil.Unmarshal(impExt["bidder"], &bidderParams); err != nil {
return fmt.Errorf("failed to parse imp.ext.bidder for imp %s: %w", imp.ID, err)
}
for _, key := range []string{"said", "psid", "template"} {
if value, ok := bidderParams[key]; ok {
params[key] = value
}
}
rawParams, err := jsonutil.Marshal(params)
if err != nil {
return fmt.Errorf("failed to marshal imp.ext.params for imp %s: %w", imp.ID, err)
}
impExt["params"] = rawParams
imp.Ext, err = jsonutil.Marshal(impExt)
if err != nil {
return fmt.Errorf("failed to marshal imp.ext for imp %s: %w", imp.ID, err)
}
return nil
}
Furthermore, your implementation and this proposed simplification assumes that imp.ext.params could be set on the incoming request. Is that true, in which case you want to preserve whatever is set there? If not, this can be even simpler:
func updateImpExtWithParams(imp *openrtb2.Imp) error {
if len(imp.Ext) == 0 {
return nil
}
var impExt map[string]json.RawMessage
if err := jsonutil.Unmarshal(imp.Ext, &impExt); err != nil {
return fmt.Errorf("failed to parse imp.ext for imp %s: %w", imp.ID, err)
}
bidderParams, exists := impExt["bidder"]
if !exists || len(bidderParams) == 0 {
return nil
}
impExt["params"] = bidderParams
rawExt, err := jsonutil.Marshal(impExt)
if err != nil {
return fmt.Errorf("failed to marshal imp.ext for imp %s: %w", imp.ID, err)
}
imp.Ext = rawExt
return nil
}
If params can be set on the incoming request, I suggest showing that in a JSON test.
There was a problem hiding this comment.
Please address this and ping me and then I will merge.
There was a problem hiding this comment.
@bsardo Thanks! imp.ext.params is not expected on the incoming request. I applied the simpler version and removed the tests that covered imp.ext.prebid.bidder and a pre-existing params. I also set additionalProperties: false in the params schema, since the whole bidder object is now forwarded.
Upstream splitImps already maps imp.ext.prebid.bidder.clickio to imp.ext.bidder, so copy imp.ext.bidder to imp.ext.params directly. Disallow unknown bidder params since the whole object is now forwarded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eb601ff
| if imp.ID != impID { | ||
| continue | ||
| } | ||
| 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.
There was a problem hiding this comment.
Clickio currently supports banner only (capabilities.site.mediaTypes: [banner], multiformat-supported: false), so imps reaching the adapter never contain other formats, and the media type is resolved unambiguously; any imp without banner returns an error (covered by supplemental/bad-media-type.json).
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
| if imp.ID != impID { | ||
| continue | ||
| } | ||
| 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.
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
| if imp.ID != impID { | ||
| continue | ||
| } | ||
| 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.ID != impID { | ||
| continue | ||
| } | ||
| 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.
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
Code coverage summaryNote:
clickioRefer here for heat map coverage report |
Type of change
Other information
Adds the Clickio bidder adapter for Prebid Server.
This change includes:
saidparameterDocs PR - prebid/prebid.github.io#6577