optable-targeting: Hid plus id5 Mobile In-app resolver params - #3
Conversation
softcoder594
changed the title
eugenedorfman-optable
left a comment
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
Review
Blocking issues below. Two of them (un-encoded query params, cache key) are also security-relevant, so flagging as request-changes rather than nits.
Most findings are left as inline comments. Two anchor on lines outside the diff, so they are here:
Blocking: new user-identifying params are excluded from the cache key
CachedAPIClient.java:55-63 keys the cache on tenant:origin:ips.getFirst():query.getIds(). This PR adds hid (including i6, which is explicitly filtered out of the ids string in buildIdsString), id5_signature, bundle and ver to the request, and none of them enter the key.
In exactly the post-ATT mobile in-app case this PR targets there is no IFA, so ipv6 and the signature are the only discriminators. Two devices behind the same first IP with the same (possibly empty) id= set will collide: the second gets the first user's cached TargetingResult, including ID5 EIDs and the refs signature. That signature is then round-tripped into the second user's client via ext.prebid.passthrough.optable.id5_signature and replayed on their subsequent auctions, so the contamination persists past the cache TTL.
CacheTest was only touched for constructor arity; nothing covers the key contents. Please add the new params to createCachingKey and a test that asserts two requests differing only in hid / id5_signature / bundle / ver do not share a cache entry.
Should fix: the response hook now blocks on the network future under a 10 ms budget
The sample plan gives the auction-response group timeout: 10 (sample/configs/sample-app-settings-optable.yaml:73). The hook now does moduleContext.getOptableTargetingCall().compose(...), so if the targeting call is still pending at response time (for example the bidder-request hook already recovered on its own timeout) the response hook waits on it and the group times out. Before this PR the response hook enriched synchronously from moduleContext.getTargeting() and could not be delayed by the network call.
Suggest either awaiting the future only when the id5 signature is actually needed, or short-circuiting when the future is not yet complete and falling back to the already-resolved targeting.
Minor
Id5Resolver.java:16-20:STR_OPTABLE_CO/STR_ID_5_SYNC_COMis not the naming convention used elsewhere in PBS;OPTABLE_INSERTER/ID5_SOURCEwould read better.QueryBuilder.java:61usesString.formatwhere the surrounding code uses.formatted().
| private Future<InvocationResult<AuctionResponsePayload>> enrichedById5SignaturePayload( | ||
| ModuleContext moduleContext) { | ||
|
|
||
| return moduleContext.getOptableTargetingCall() |
There was a problem hiding this comment.
Blocking: NPE when optableTargetingCall was never set.
moduleContext.getOptableTargetingCall() is dereferenced here (and at line 74) with no null guard, but the future is set in exactly one place, OptableRawAuctionRequestHook:98. Paths where it is null at response time:
- Deprecated plan (processed-auction-request hook instead of the raw hook): nothing ever stores the future, so even a fully successful targeting call NPEs here. That silently drops adserver-targeting keyword enrichment for every request, which is a regression - before this PR the hook enriched synchronously from
moduleContext.getTargeting(). - Raw-hook plan, early returns:
OptableRawAuctionRequestHookreturns without setting the future whenbiddersToEnrichis empty (lines 83-85) or the properties are invalid (lines 66-74), andshouldSkipEnrichmentstaysfalse. The sample config shipspubmatic: 0, so a pubmatic-only request hits this on every auction.OptableBidderRequestHook:41-44guards this case; this hook does not. - A fresh
ModuleContext(ModuleContext.ofreturnsnew ModuleContext()when the invocation context has none).
GroupExecutor catches the Throwable, so nothing crashes - the module just fails and all response enrichment is dropped, which is why this does not show up as a test failure. The test churn is the tell: pre-existing tests had to be handed futures (OptableTargetingAuctionResponseHookTest.java:66-68) to keep passing.
Please null-guard and fall back to the previous synchronous path, and add a test that runs the response hook with a ModuleContext that has no optableTargetingCall.
| final String hidParameters = Arrays.stream(hidPrefixesString.split(",")) | ||
| .map(prefixToIdValue::get) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> String.format("hid=%s:%s", it.getName(), it.getValue())) |
There was a problem hiding this comment.
Blocking: hid values are appended un-encoded. Compare with buildIdsString below, which percent-encodes id= values, and with id5_signature at line 140 which is also encoded.
These values are client-controlled (user.ext.optable.*, eids, device.ipv6). Two consequences:
- Corruption. An E.164 phone
p:+15551234reaches the edge asp: 15551234because gin decodes+as a space.idtype.Parsethen fails and the resolver silentlycontinues past the hint, so the hid is dropped with no error anywhere. - Query-param injection. A value containing
&injects arbitrary params into the targeting call, and the v2 targeting handler exposes live control params (skip_resolvers,skip_matchers,environment,id5_signature). So a crafted id lets a caller disable or redirect resolver behaviour server-side.
URLEncoder.encode the name:value pair the same way buildIdsString does. Note the new tests assert the un-encoded output (shouldBuildHidWhenHidPrefixesMatchIds), so they need updating too - right now they lock the bug in.
There was a problem hiding this comment.
Confirmed fixed at 903aebb, with a reserved-character test. Closing.
| .ifPresent(app -> { | ||
| final String bundle = app.getBundle(); | ||
| if (StringUtils.isNotEmpty(bundle)) { | ||
| sb.append("&bundle=").append(bundle); |
There was a problem hiding this comment.
Blocking (same class of bug as hid): bundle and ver are appended un-encoded.
app.bundle and app.ver come straight from the bid request. A bundle such as com.example&skip_resolvers=id5 injects a control param into the targeting call. Percent-encode both.
There was a problem hiding this comment.
Confirmed fixed at 903aebb. Closing.
| return StringUtils.EMPTY; | ||
| } | ||
|
|
||
| final String hidParameters = Arrays.stream(hidPrefixesString.split(",")) |
There was a problem hiding this comment.
Should fix: hid-prefixes tokens are not trimmed.
"e, p, i6".split(",") yields " p" and " i6", which never match an id name, so those prefixes are silently dropped and the operator gets no hid params and no error. The config is documented as "a comma-separated set", and a space after a comma is the natural way to write that.
Add .map(String::trim) and drop empty tokens. Worth logging (or failing config validation on) a prefix that matches no known id name, otherwise a typo is invisible. Tests for whitespace and empty tokens are missing.
| } | ||
| final Map<String, Id> prefixToIdValue = ids.stream() | ||
| .collect(Collectors.toMap(Id::getName, it -> it, (a, b) -> b)); | ||
| if (MapUtils.isEmpty(prefixToIdValue)) { |
There was a problem hiding this comment.
Dead code: ids is already known non-empty at this point, so the collected map cannot be empty.
There was a problem hiding this comment.
Confirmed removed. Closing.
| final String ver = app.getVer(); | ||
| if (StringUtils.isNotEmpty(ver)) { | ||
| sb.append("&ver=").append(ver); | ||
| } |
There was a problem hiding this comment.
ver is only emitted when bundle is non-empty, but the server treats them as independent ResolverParams fields. An app that sets app.ver without app.bundle loses the version for no reason.
There was a problem hiding this comment.
Agreed, your call stands - withdrawn on the round-2 thread too. Closing.
| return null; | ||
| } | ||
|
|
||
| final String ref = Optional.of(targetingResult) | ||
| .map(TargetingResult::getOrtb2) | ||
| .map(Ortb2::getUser) | ||
| .map(User::getEids) | ||
| .orElseGet(List::of) | ||
| .stream() | ||
| .filter(it -> STR_OPTABLE_CO.equals(it.getInserter()) && STR_ID_5_SYNC_COM.equals(it.getSource())) | ||
| .map(Eid::getUids) | ||
| .flatMap(Collection::stream) | ||
| .map(Uid::getExt) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> it.get(STR_OPTABLE)) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> it.get(STR_REF)) | ||
| .filter(Objects::nonNull) | ||
| .map(JsonNode::asText) | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| if (ref == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return Optional.ofNullable(targetingResult.getRefs()) | ||
| .map(refs -> refs.get(ref)) |
There was a problem hiding this comment.
Only the first matching eid's ref is considered. If refs has no entry under that particular ref, a valid signature reachable through a second matching eid is ignored and the request goes out without a signature.
shouldReturnSignatureWhenMultipleMatchingEidsExist asserts exactly this first-only behaviour, so the test documents the limitation rather than catching it. Suggest iterating the matching eids until one resolves against refs.
There was a problem hiding this comment.
Confirmed fixed - all matching refs are iterated now. Closing (the separate "null" literal issue is on its own thread).
| return builder.build(); | ||
| } | ||
|
|
||
| private static ExtUserOptable parseExtUserOptable(JsonNode node) { |
There was a problem hiding this comment.
Three things here:
- This re-parses
user.ext.optable, whichIdsMapperhas already parsed for the same request. - It uses the static
ObjectMapperProvider.mapper()rather than the mapper injected into this class, which diverges from how the rest of the module gets its mapper. - The parse failure is swallowed silently, whereas
IdsMapperlogs it. A malformeduser.ext.optablewill drop the id5 signature with no trace.
Also, the if (extUserOptable.isPresent()) { extUserOptable.map(...).ifPresent(...) } wrapper just above is redundant - the ifPresent already covers it.
| id-prefix-order: "e,v,c" | ||
| ppid-mapping: { "pubcid.org": "c" } | ||
| hid-prefixes: "c,i6" | ||
| ppid-mapping: { "pubmatic.com": "c" } |
There was a problem hiding this comment.
Unrelated to this PR and looks like a debugging leftover: ppid-mapping changed from {"pubcid.org": "c"} to {"pubmatic.com": "c"}. pubmatic.com is a bidder, not an eid source, so this mapping cannot match anything. Please revert.
There was a problem hiding this comment.
Confirmed reverted. Closing.
… resolving id5Signature
There was a problem hiding this comment.
Re-review at 903aebb
Encoding of the query params, the Id5Resolver first-match limitation, the OptableAttributesResolver re-parse and the dead MapUtils.isEmpty check are resolved. The rest is below.
I built the module and ran the suite this time rather than reviewing by inspection: 192 tests, all green at 903aebb2a. So the remaining findings are things the tests do not reach, not things the tests disagree with. Two of them I turned into executable repros - the output is quoted in the relevant inline comments.
Still open: the NPE in the auction-response hook (still reachable, and it also keeps the 10 ms stall alive), the cache key (file untouched), the hid-prefixes handling (partially addressed), and the sample-config endpoint drive-by (still in the diff). Two new issues introduced by the fix-up are flagged inline as well.
Happy to share the two repro tests as a starting point for regression coverage if useful.
|
|
||
| final String id5Signature = moduleContext.getId5Signature(); | ||
|
|
||
| return moduleContext.getOptableTargetingCall() |
There was a problem hiding this comment.
Still blocking: the NPE moved but did not go away. It also keeps the 10 ms stall flagged in the last review alive.
The keyword path is now synchronous and correct, but this branch still dereferences the future unguarded. Every path where the future was never set also has null targeting, so AuctionResponseValidator.checkEnrichmentPossibility returns NOKEYWORD (AuctionResponseValidator.java:21-22) and control lands exactly here. Live paths: the raw-hook early returns at OptableRawAuctionRequestHook.java:73,79,84 (the shipped sample pubmatic: 0 reaches this), a fresh ModuleContext, and the deprecated processed-only plan whenever the API returns no audience or the response has no bids.
Reproduced - a ModuleContext with no optableTargetingCall and no targeting, plus a bid response:
java.lang.NullPointerException: Cannot invoke "io.vertx.core.Future.compose(java.util.function.Function)"
because the return value of "...ModuleContext.getOptableTargetingCall()" is null
Note the compose is dead weight regardless: targetingResult is ignored and id5Signature is read from the context on the line above. Deleting the compose and calling update(id5SignatureEnrichmentChain(id5Signature), moduleContext) directly fixes the NPE and removes the last place the response hook can stall on a pending network call under the auction-response group's timeout: 10.
The existing tests pass because they all hand the hook a future. Please add one that does not.
| .recover(ignore -> apiClient.getTargeting(properties, query, ips, userAgent, timeout) | ||
| .recover(throwable -> isCircuitBreakerEnabled | ||
| ? Future.succeededFuture(new TargetingResult(null, null)) | ||
| ? Future.succeededFuture(new TargetingResult(null, null, null)) |
There was a problem hiding this comment.
Still blocking, and unaddressed: the cache key. (Anchored here because createCachingKey itself is outside the diff; the issue is in that method at lines 55-63, unchanged since the last review.)
The key is still tenant:origin:ips.getFirst():query.getIds(). Query has three parts, and getIds() excludes everything this PR added: hid (including i6, explicitly filtered out of the ids string at QueryBuilder.java:75), id5_signature, bundle, ver.
In the post-ATT mobile in-app case this PR exists to serve there is no IFA, so ipv6 and the signature are the only discriminators - and neither is in the key. Two devices behind one first-hop IP with the same id set collide: the second gets the first user's cached TargetingResult, including their ID5 EIDs and refs signature, which then round-trips into the second user's client via ext.prebid.passthrough.optable.id5_signature and is replayed on their next auctions, so it outlives the cache TTL.
Please include the full query in the key, not just getIds(), and add a test asserting two requests differing only in hid / id5_signature / bundle / ver do not share an entry.
| final String hidParameters = Arrays.stream(hidPrefixesString.split(",")) | ||
| .map(String::trim) | ||
| .filter(StringUtils::isNotEmpty) | ||
| .map(prefixToIdValue::get) |
There was a problem hiding this comment.
Partially fixed. Trimming and empty-token filtering are in, thanks.
Two things left:
- An unknown or typo'd prefix is still silently dropped here -
prefixToIdValue::getreturns null and the followingfilterswallows it. An operator who writesi5instead ofi6gets no hid params and no signal anywhere. Worth a log line, or config validation against the known id names. - The trim itself is untested: every case in
QueryBuilderTest.java:207-252uses space-free config, so nothing would catch a regression here.
Minor, same lines: String.format at line 59 where the surrounding code uses .formatted().
There was a problem hiding this comment.
It's expected behaviour. No need logs or config validation here.
| final String bundle = app.getBundle(); | ||
| if (StringUtils.isNotEmpty(bundle)) { | ||
| sb.append("&bundle=").append(URLEncoder.encode(bundle, StandardCharsets.UTF_8)); | ||
|
|
||
| final String ver = app.getVer(); | ||
| if (StringUtils.isNotEmpty(ver)) { | ||
| sb.append("&ver=").append(URLEncoder.encode(ver, StandardCharsets.UTF_8)); | ||
| } |
There was a problem hiding this comment.
Not addressed: ver is still nested inside the bundle check, so an app that sets app.ver without app.bundle loses the version. The server treats them as independent ResolverParams fields. The README now documents the current behaviour as intended - if that is a deliberate decision rather than an accident, disregard, but it is worth stating the reason since the server does not require the pairing.
Also no encoding test covers bundle / ver / id5_signature with reserved characters (QueryBuilderTest.java:292-364 all use safe values), so the encoding fix is only asserted for hid.
There was a problem hiding this comment.
Withdrawn - you had already answered this and the rejection is right: a version without a bundle is not useful for ID5 app matching, and the README documents the pairing. Re-raised in error.
One unrelated point from that comment still stands on its own: bundle / ver / id5_signature have no reserved-character encoding test, so the encoding fix is only asserted for hid.
There was a problem hiding this comment.
Ver without bundle does not have sense. It's Ok.
| .map(it -> it.get(STR_SIGNATURE)) | ||
| .filter(Objects::nonNull) | ||
| .filter(JsonNode::isValueNode) | ||
| .map(JsonNode::asText) | ||
| .filter(StringUtils::isNotBlank) |
There was a problem hiding this comment.
New issue introduced here: this can return the literal string "null".
A refs entry whose signature is JSON null gives a NullNode. NullNode.isValueNode() is true, so it passes the filter; asText() on it yields "null"; and isNotBlank("null") is true. The string "null" is then sent as the id5_signature= query param and written into the client passthrough.
Reproduced with a refs entry of {"signature": null}:
expected: null
but was: "null"
Fix: .filter(it -> !it.isNull()) before .map(JsonNode::asText).
Unrelated nit while here: STR_OPTABLE_CO / STR_ID_5_SYNC_COM is not the naming style used elsewhere in PBS - OPTABLE_INSERTER / ID5_SOURCE reads better.
|
|
||
| if (hasData) { | ||
| moduleContext.setTargeting(targetingResult.getAudience()); | ||
| moduleContext.setId5Signature(Id5Resolver.resolveId5Signature(targetingResult)); |
There was a problem hiding this comment.
New issue introduced by the refactor: the signature is silently dropped in the raw+bidder plan when per-bidder enrichment is off.
This is now the only place the signature is set in that plan, but the hook returns early at lines 37-39 when properties.isPerBidderEnrichmentEnabled() is false - and that is false for the default config, since it requires enrichmentPercentage != 100 || !bidderEnrichmentPercentages.isEmpty() (OptableTargetingProperties.java:59-61). With both hooks in the execution plan, OptableTargetingProcessedAuctionRequestHook.java:76-78 also returns without setting it.
So with enrichment-percentage: 100 and no per-bidder map, moduleContext.getId5Signature() is null at response time and the signature never reaches the client. At 073b1d543 the response hook resolved it from the future's TargetingResult, so it still got through. Behavior regression from the fix-up, and no test covers this combination.
| private final OptableTargeting optableTargeting; | ||
| private final UserFpdActivityMask userFpdActivityMask; | ||
| private final TimeoutFactory timeoutFactory; | ||
| private double logSamplingRate; |
There was a problem hiding this comment.
Nit: logSamplingRate should be final like the three fields above it - it is assigned once in the constructor.
| modules: | ||
| optable-targeting: | ||
| api-endpoint: https://ca.edge.optable.co/v2/targeting?t={TENANT}&o={ORIGIN} | ||
| api-endpoint: https://na.edge.optable.co/v2/targeting?t={TENANT}&o={ORIGIN} |
There was a problem hiding this comment.
Still here: ca.edge -> na.edge is unrelated to this PR's scope. The ppid-mapping revert landed, this one did not. Please revert or explain.
There was a problem hiding this comment.
Withdrawn - this was already answered on the round-1 thread (resolver enabled on na.edge, and the endpoint has ID5 signature support) and I re-raised it without reading the replies. My mistake, please ignore.
There was a problem hiding this comment.
It's Ok. This endpoint returns Id5
| resolves the signature through the matching optable-eid and propagates it back into the request context, where it | ||
| is later injected into the bid response (`ext.prebid.passthrough.optable.id5_signature`) for downstream use. | ||
|
|
||
| The `id5_signature` field is also part of the Optable input erasure — see below. |
There was a problem hiding this comment.
Says the erasure section is "below" when it is above. Also this new section uses em-dashes, which the rest of the doc does not.
eugenedorfman-optable
left a comment
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
Review of the hid + ID5 in-app resolver changes. Every point below except the dead objectMapper is posted as an applicable suggestion, so it can be taken with the "Add suggestion to batch" button and committed in one go. That one lands mostly on lines outside the diff, so it is spelled out as a patch instead.
Needs fixing
- Every app request sends
bundle,verandid5_signaturetwice, because bothbuildAttributesStringandbuildHidAttributesStringemit them andtoQueryString()concatenates both. - A failed targeting call now fails the bidder request hook on the not-enriched path, where it used to return
no_action. IdsMapper.objectMapperis dead afterparseExtUserOptablewas removed.- Four em-dashes in the README.
Plus three regression tests for the gaps that let 1 and 2 through, and one for the hid-prefixes trim() this PR adds without asserting it.
Two things worth a decision rather than a patch
- Serializing the bidder requests behind the targeting call is what makes the signature propagation work, but it moves the targeting latency onto the bidder request path for every bidder, including the ones that are not enriched. Intentional?
- The auction response hook now returns
updateunconditionally, even when there is no signature and no advertiser targeting to write. That is observability noise (the analytics tag says the payload changed when it did not) rather than a bug, but it makes the module's tags harder to read.
| Optional.ofNullable(optableAttributes.getApp()) | ||
| .ifPresent(app -> { | ||
| final String bundle = app.getBundle(); | ||
| if (StringUtils.isNotEmpty(bundle)) { | ||
| sb.append("&bundle=").append(URLEncoder.encode(bundle, StandardCharsets.UTF_8)); | ||
|
|
||
| final String ver = app.getVer(); | ||
| if (StringUtils.isNotEmpty(ver)) { | ||
| sb.append("&ver=").append(URLEncoder.encode(ver, StandardCharsets.UTF_8)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Optional.ofNullable(optableAttributes.getId5Signature()) | ||
| .ifPresent(id5Signature -> sb.append("&id5_signature=") | ||
| .append(URLEncoder.encode(id5Signature, StandardCharsets.UTF_8))); | ||
|
|
There was a problem hiding this comment.
bundle, ver and id5_signature are emitted here and in buildHidAttributesString, and Query.toQueryString() concatenates both, so every app request ends up with each parameter twice (...&bundle=x&ver=y&id5_signature=z&bundle=x&ver=y&id5_signature=z). shouldIncludeHidAttributesInToQueryString passes because it asserts with endsWith, which the duplicate satisfies.
They have to stay in buildHidAttributesString (that is what puts them in the cache key), so the fix is to drop the copy here:
| Optional.ofNullable(optableAttributes.getApp()) | |
| .ifPresent(app -> { | |
| final String bundle = app.getBundle(); | |
| if (StringUtils.isNotEmpty(bundle)) { | |
| sb.append("&bundle=").append(URLEncoder.encode(bundle, StandardCharsets.UTF_8)); | |
| final String ver = app.getVer(); | |
| if (StringUtils.isNotEmpty(ver)) { | |
| sb.append("&ver=").append(URLEncoder.encode(ver, StandardCharsets.UTF_8)); | |
| } | |
| } | |
| }); | |
| Optional.ofNullable(optableAttributes.getId5Signature()) | |
| .ifPresent(id5Signature -> sb.append("&id5_signature=") | |
| .append(URLEncoder.encode(id5Signature, StandardCharsets.UTF_8))); |
| return sb.toString(); | ||
| } | ||
|
|
||
| private static String buildHidAttributesString(OptableAttributes optableAttributes) { |
There was a problem hiding this comment.
Worth saying why the same attributes are built in a separate method, so the next reader does not "fix" it by merging the two back together:
| private static String buildHidAttributesString(OptableAttributes optableAttributes) { | |
| // Kept apart from buildAttributesString because these are the only attributes that | |
| // discriminate one user from another, so they have to take part in the cache key. | |
| private static String buildHidAttributesString(OptableAttributes optableAttributes) { |
| return targetingCall.compose(targetingResult -> { | ||
| moduleContext.setId5Signature(Id5Resolver.resolveId5Signature(targetingResult)); | ||
|
|
||
| return Future.succeededFuture( | ||
| InvocationResultImpl.<BidderRequestPayload>builder() | ||
| .status(InvocationStatus.success) | ||
| .action(InvocationAction.no_action) | ||
| .analyticsTags(analyticsTags) | ||
| .moduleContext(moduleContext) | ||
| .build()); | ||
| }); |
There was a problem hiding this comment.
This compose has no failure branch, so when the targeting call fails (timeout, non-2xx, circuit breaker open) the returned future fails and the hook execution is reported as failed. Before the signature change this path returned no_action unconditionally. This bidder is not being enriched anyway, so a failed targeting call should not change the outcome.
Adding .recover fixes it, and the inline builder can reuse the existing noActionResponse helper so both branches return the same thing:
| return targetingCall.compose(targetingResult -> { | |
| moduleContext.setId5Signature(Id5Resolver.resolveId5Signature(targetingResult)); | |
| return Future.succeededFuture( | |
| InvocationResultImpl.<BidderRequestPayload>builder() | |
| .status(InvocationStatus.success) | |
| .action(InvocationAction.no_action) | |
| .analyticsTags(analyticsTags) | |
| .moduleContext(moduleContext) | |
| .build()); | |
| }); | |
| return targetingCall | |
| .compose(targetingResult -> { | |
| moduleContext.setId5Signature(Id5Resolver.resolveId5Signature(targetingResult)); | |
| return noActionResponse(moduleContext, analyticsTags); | |
| }) | |
| // this bidder is not enriched anyway, so a failed targeting call | |
| // must not turn into a hook execution failure | |
| .recover(ignored -> noActionResponse(moduleContext, analyticsTags)); |
|
|
||
| For app traffic (requests with an `app` object) the module forwards the application's bundle identifier as the | ||
| `bundle=` query parameter and, when available, its version as `ver=`. Both values are URL-encoded. The `ver` parameter | ||
| is only sent when `bundle` is present and non-empty — requests with a version but no bundle will not produce either |
There was a problem hiding this comment.
Em-dash:
| is only sent when `bundle` is present and non-empty — requests with a version but no bundle will not produce either | |
| is only sent when `bundle` is present and non-empty; requests with a version but no bundle will not produce either |
| 1. **Request-side signature** — when the incoming OpenRTB request carries `user.ext.optable.id5_signature`, that value | ||
| is sent to the Targeting API as the `id5_signature=` query parameter. | ||
| 2. **Response-side signature** — when the Targeting API response contains refs with an ID5 signature, the module |
There was a problem hiding this comment.
Em-dashes:
| 1. **Request-side signature** — when the incoming OpenRTB request carries `user.ext.optable.id5_signature`, that value | |
| is sent to the Targeting API as the `id5_signature=` query parameter. | |
| 2. **Response-side signature** — when the Targeting API response contains refs with an ID5 signature, the module | |
| 1. **Request-side signature**: when the incoming OpenRTB request carries `user.ext.optable.id5_signature`, that value | |
| is sent to the Targeting API as the `id5_signature=` query parameter. | |
| 2. **Response-side signature**: when the Targeting API response contains refs with an ID5 signature, the module |
| resolves the signature through the matching optable-eid and propagates it back into the request context, where it | ||
| is later injected into the bid response (`ext.prebid.passthrough.optable.id5_signature`) for downstream use. | ||
|
|
||
| The `id5_signature` field is also part of the Optable input erasure — see above. |
There was a problem hiding this comment.
Em-dash:
| The `id5_signature` field is also part of the Optable input erasure — see above. | |
| The `id5_signature` field is also part of the Optable input erasure, see above. |
|
|
||
| import com.fasterxml.jackson.core.JsonProcessingException; | ||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; |
There was a problem hiding this comment.
Moving the parsing out to ExtUserOptableResolver left objectMapper with no remaining use in this class: this import, the field, its requireNonNull and the constructor parameter are all dead.
The other three edits sit outside the diff, so this cannot be a one-click suggestion. Applying it by hand:
--- a/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/core/IdsMapper.java
+++ b/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/core/IdsMapper.java
@@
-import com.fasterxml.jackson.databind.ObjectMapper;
import com.iab.openrtb.request.BidRequest;
@@
- private final ObjectMapper objectMapper;
private final double logSamplingRate;
- public IdsMapper(ObjectMapper objectMapper, double logSamplingRate) {
- this.objectMapper = Objects.requireNonNull(objectMapper);
+ public IdsMapper(double logSamplingRate) {
this.logSamplingRate = logSamplingRate;
}Two call sites follow, OptableTargetingConfig:51:
- return new IdsMapper(ObjectMapperProvider.mapper(), logSamplingRate);
+ return new IdsMapper(logSamplingRate);and IdsMapperTest:35:
- target = new IdsMapper(objectMapper, 0.01);
+ target = new IdsMapper(0.01);java.util.Objects is still used further down the file, so that import stays; same for ObjectMapperProvider in OptableTargetingConfig.
The alternative is to keep the field and thread the injected mapper into ExtUserOptableResolver instead of having it call ObjectMapperProvider.mapper() itself. That touches more call sites for no behavioural gain, so the removal above looks like the better trade, but it is your call.
| @@ -74,6 +78,61 @@ public void shouldReturnNoActionWhenPerBidderEnrichmentIsDisabled() { | |||
| assertThat(result.moduleContext()).isSameAs(moduleContext); | |||
| } | |||
There was a problem hiding this comment.
Nothing covers the failed targeting call on this path, which is how the missing recover got through. Suggested regression test:
| } | |
| } | |
| @Test | |
| public void shouldReturnNoActionWhenPerBidderEnrichmentIsDisabledAndTargetingCallFailed() { | |
| // given | |
| final ModuleContext moduleContext = givenModuleContextWithProperties( | |
| givenOptableTargetingProperties(false)); | |
| moduleContext.setOptableTargetingCall(Future.failedFuture(new RuntimeException("timeout"))); | |
| when(invocationContext.moduleContext()).thenReturn(moduleContext); | |
| // when | |
| final Future<InvocationResult<BidderRequestPayload>> future = | |
| target.call(bidderRequestPayload, invocationContext); | |
| // then | |
| assertThat(future.succeeded()).isTrue(); | |
| final InvocationResult<BidderRequestPayload> result = future.result(); | |
| assertThat(result).isNotNull() | |
| .returns(InvocationStatus.success, InvocationResult::status) | |
| .returns(InvocationAction.no_action, InvocationResult::action); | |
| assertThat(moduleContext.getId5Signature()).isNull(); | |
| } |
| @BeforeEach | ||
| public void setUp() { | ||
| mapper = ObjectMapperProvider.mapper(); | ||
| } |
There was a problem hiding this comment.
The signature node being JSON null rather than absent is not covered, and it is the shape the API actually returns when a ref has no signature. Suggested test:
| } | |
| } | |
| @Test | |
| public void shouldReturnNullWhenSignatureIsJsonNull() { | |
| // given | |
| final ObjectNode uidExt = mapper.createObjectNode(); | |
| uidExt.set("optable", mapper.createObjectNode() | |
| .set("ref", TextNode.valueOf("refValue"))); | |
| final Eid eid = Eid.builder() | |
| .source("id5-sync.com") | |
| .inserter("optable.co") | |
| .uids(List.of(Uid.builder().id("id").ext(uidExt).build())) | |
| .build(); | |
| final ObjectNode refs = mapper.createObjectNode(); | |
| refs.set("refValue", mapper.createObjectNode().putNull("signature")); | |
| final TargetingResult targetingResult = new TargetingResult( | |
| List.of(), | |
| new Ortb2(new User(List.of(eid), null)), | |
| refs); | |
| // when | |
| final String result = Id5Resolver.resolveId5Signature(targetingResult); | |
| // then | |
| assertThat(result).isNull(); | |
| } |
| assertThat(queryString).endsWith("&bundle=com.example.app&ver=1.2.3&id5_signature=signature"); | ||
| } |
There was a problem hiding this comment.
endsWith is what let the duplicated bundle/ver/id5_signature through, since the doubled query string still ends with the expected suffix. Counting occurrences pins it. The second test covers the trim() on hid-prefixes added in this PR, which is currently unasserted:
| assertThat(queryString).endsWith("&bundle=com.example.app&ver=1.2.3&id5_signature=signature"); | |
| } | |
| assertThat(queryString).endsWith("&bundle=com.example.app&ver=1.2.3&id5_signature=signature"); | |
| // they live in hidAttributes so that they take part in the cache key, and must not | |
| // also be emitted from buildAttributesString | |
| assertThat(queryString.split("&bundle=", -1)).hasSize(2); | |
| assertThat(queryString.split("&ver=", -1)).hasSize(2); | |
| assertThat(queryString.split("&id5_signature=", -1)).hasSize(2); | |
| } | |
| @Test | |
| public void shouldTrimWhitespaceAroundHidPrefixes() { | |
| // given | |
| final List<Id> ids = List.of(Id.of(Id.EMAIL, "email"), Id.of(Id.PHONE, "phone")); | |
| // when | |
| final Query query = QueryBuilder.build( | |
| ids, OptableAttributes.builder().build(), givenProperties(idPrefixOrder, " e , p ")); | |
| // then | |
| assertThat(query.getHid()).isEqualTo("&hid=e:email&hid=p:phone"); | |
| } |
There was a problem hiding this comment.
Re-review at b8441d5
Went back over the whole PR and over every open thread. Everything raised in the first two rounds is now either verified fixed or settled by your reply, the itemised confirmations are in the follow-up comment. Nothing from a rejected thread is being raised again.
Still open from the last round, unchanged at b8441d5, all with applicable suggestions already attached:
bundle,verandid5_signatureare emitted twice per app request (buildAttributesStringandbuildHidAttributesStringboth write them,toQueryString()concatenates both).noActioninOptableBidderRequestHookhas no failure branch, so a failed targeting call now fails the hook on the path where the bidder is not enriched.objectMapperinIdsMapperis dead since the parsing moved toExtUserOptableResolver.- Three regression tests for the gaps that let the first two through, plus the
hid-prefixestrim().
One thing on the duplication fix, since it interacts with the cache key you just landed: the copies have to come out of buildAttributesString, not buildHidAttributesString. Only hidAttributes feeds createCachingKey, so removing the wrong copy would silently stop the key varying with bundle, version and signature, and requests from different apps would start sharing cached targeting.
One new finding below.
| .map(ExtUserOptable::getId5Signature) | ||
| .ifPresent(builder::id5Signature); |
There was a problem hiding this comment.
A blank id5_signature from the request is forwarded as-is. bundle and ver are guarded with StringUtils.isNotEmpty in QueryBuilder, and the response side filters blanks in Id5Resolver, but this path only checks for null.
An SDK that has no signature yet and sends user.ext.optable.id5_signature: "" therefore produces a dangling &id5_signature= in the Targeting API query. Since hidAttributes now feeds createCachingKey, the same user also gets two distinct cache entries depending on whether the empty field was present.
Filtering here fixes both the query string and the cache key in one place:
| .map(ExtUserOptable::getId5Signature) | |
| .ifPresent(builder::id5Signature); | |
| .map(ExtUserOptable::getId5Signature) | |
| .filter(StringUtils::isNotBlank) | |
| .ifPresent(builder::id5Signature); |
There was a problem hiding this comment.
Status of the earlier rounds, verified against the code at b8441d5.
Round 2:
- Auction response hook NPE - fixed at 9f5bef2. The hook no longer touches
getOptableTargetingCall()at all, so the NPE is structurally gone and the 10 ms stall with it. Every test inOptableTargetingAuctionResponseHookTestnow exercises the no-future case. - Cache key - fixed at 144d1e6.
createCachingKeyfolds inids,hidandhidAttributes, andCachedAPIClientTestasserts hid, bundle and id5_signature each reach the key. Id5Resolverreturning the literal"null"- fixed at a54cecd by theisNullfilter.- Signature dropped when per-bidder enrichment is off - fixed at 9f5bef2. That fix is what left the compose without a failure branch, which is the open
recoverthread. logSamplingRatenot final - fixed.- README "below" vs "above" - fixed. The em-dashes were still in the file after that commit; removed in b8441d5.
- Unknown
hid-prefixestoken dropped silently - your call stands, withdrawn. The trim test is the only part still outstanding and it has its own thread onQueryBuilderTest.
Round 1 is entirely closed: query param encoding, the dead MapUtils check, first-ref-only resolution, the shared ext.user.optable parsing, and the ppid-mapping leftover are all confirmed fixed; ver without bundle and the na.edge endpoint are settled as your call and will not come up again.
The open items are unchanged from the previous comment.
eugenedorfman-optable
left a comment
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
Product decision: no ID5 signature when nothing was enriched
We have decided that the ID5 signature should only be delivered when the module actually enriched something. If there was no enrichment, the signature is not wanted.
That is a deliberate scope reduction on our side, not a defect report, and it goes against what this PR currently implements. Flagging it now rather than after merge, and happy to hear the case for the current behaviour if there is one.
What it changes
Two places carry the signature onto no-enrichment paths today:
-
OptableBidderRequestHook.noActioncomposes on the targeting future purely to resolve the signature for bidders the module is not enriching. Under this decision it does not need to wait at all and can return immediately as it did before the PR. This also removes the latency question raised earlier: the early-call design exists so that non-enriched bidders are never held behind the targeting call, and today they are. -
OptableTargetingAuctionResponseHookreturnsupdatewith a signature-only enrichment chain on every no-enrichment path, including whenshouldSkipEnrichmentis set and when validation fails. Under this decision those go back tono_action, and the signature survives only insidefullEnrichmentChain, that is, only when there is targeting to write. As a side effect the analytics tags stop reporting a payload change on auctions the module did not touch.
Consequence for the open recover suggestion
The suggestion on noAction about the missing failure branch is superseded by this. If the compose goes away entirely there is no failure branch to add, so please take one or the other, not both. This direction is the simpler of the two.
Tests that encode the current intent
These seven assert the behaviour being removed and would need to go or be inverted, which is the clearest evidence that the current behaviour was deliberate:
shouldEnrichBidResponseWithId5SignatureOnlyWhenNoTargetingshouldReturnUpdateActionWithId5SignatureWhenSkipEnrichmentIsTrueshouldReturnUpdateActionWhenOptableTargetingCallFailsAndTargetingIsEmptyshouldReturnResultWithUpdateActionWhenTargetingCallFailsAndNoBidsshouldReturnResultWithUpdateActionAndWithPBSAnalyticsTagsshouldSetId5SignatureOnModuleContextWhenPerBidderEnrichmentIsDisabledAndTargetingCallIsPresentshouldSetId5SignatureOnModuleContextWhenBidderNotInEnrichmentSetAndTargetingCallIsPresent
The production change
11 insertions, 40 deletions across the two hooks:
diff --git a/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableBidderRequestHook.java b/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableBidderRequestHook.java
index 6128343a4..a8926b0de 100644
--- a/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableBidderRequestHook.java
+++ b/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableBidderRequestHook.java
@@ -84,7 +84,7 @@ public class OptableBidderRequestHook implements BidderRequestHook {
final Tags analyticsTags = AnalyticTagsResolver.toBidderEnrichRequestAnalyticTags(
bidder, outcome, executionTime);
- return noActionResponse(moduleContext, analyticsTags);
+ return noAction(moduleContext, analyticsTags);
}
private static boolean hasEnrichmentData(TargetingResult targetingResult) {
@@ -100,27 +100,6 @@ public class OptableBidderRequestHook implements BidderRequestHook {
}
private Future<InvocationResult<BidderRequestPayload>> noAction(ModuleContext moduleContext, Tags analyticsTags) {
- final Future<TargetingResult> targetingCall = moduleContext.getOptableTargetingCall();
- if (targetingCall != null) {
- return targetingCall.compose(targetingResult -> {
- moduleContext.setId5Signature(Id5Resolver.resolveId5Signature(targetingResult));
-
- return Future.succeededFuture(
- InvocationResultImpl.<BidderRequestPayload>builder()
- .status(InvocationStatus.success)
- .action(InvocationAction.no_action)
- .analyticsTags(analyticsTags)
- .moduleContext(moduleContext)
- .build());
- });
- }
-
- return noActionResponse(moduleContext, analyticsTags);
- }
-
- private Future<InvocationResult<BidderRequestPayload>> noActionResponse(ModuleContext moduleContext,
- Tags analyticsTags) {
-
return Future.succeededFuture(
InvocationResultImpl.<BidderRequestPayload>builder()
.status(InvocationStatus.success)
diff --git a/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableTargetingAuctionResponseHook.java b/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableTargetingAuctionResponseHook.java
index 8b0e233a0..4d714f5d4 100644
--- a/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableTargetingAuctionResponseHook.java
+++ b/extra/modules/optable-targeting/src/main/java/org/prebid/server/hooks/modules/optable/targeting/v1/OptableTargetingAuctionResponseHook.java
@@ -63,24 +63,15 @@ public class OptableTargetingAuctionResponseHook implements AuctionResponseHook
return validationStatus.getStatus() == Status.SUCCESS
? enrichedPayload(moduleContext)
- : enrichedById5SignaturePayload(moduleContext);
+ : success(moduleContext);
}
private Future<InvocationResult<AuctionResponsePayload>> enrichedPayload(ModuleContext moduleContext) {
final List<Audience> targeting = moduleContext.getTargeting();
- final String id5Signature = moduleContext.getId5Signature();
return CollectionUtils.isNotEmpty(targeting)
- ? update(fullEnrichmentChain(targeting, id5Signature), moduleContext)
- : update(id5SignatureEnrichmentChain(id5Signature), moduleContext);
- }
-
- private Future<InvocationResult<AuctionResponsePayload>> enrichedById5SignaturePayload(
- ModuleContext moduleContext) {
-
- final String id5Signature = moduleContext.getId5Signature();
-
- return update(id5SignatureEnrichmentChain(id5Signature), moduleContext);
+ ? update(fullEnrichmentChain(targeting, moduleContext.getId5Signature()), moduleContext)
+ : success(moduleContext);
}
private PayloadUpdate<AuctionResponsePayload> fullEnrichmentChain(final List<Audience> targeting,
@@ -90,10 +81,6 @@ public class OptableTargetingAuctionResponseHook implements AuctionResponseHook
.andThen(Id5SignatureBidResponseEnricher.of(id5Signature, objectMapper, jsonMerger))::apply;
}
- private PayloadUpdate<AuctionResponsePayload> id5SignatureEnrichmentChain(String id5Signature) {
- return Id5SignatureBidResponseEnricher.of(id5Signature, objectMapper, jsonMerger);
- }
-
private Future<InvocationResult<AuctionResponsePayload>> update(
PayloadUpdate<AuctionResponsePayload> payloadUpdate,
ModuleContext moduleContext) {
@@ -109,8 +96,13 @@ public class OptableTargetingAuctionResponseHook implements AuctionResponseHook
}
private Future<InvocationResult<AuctionResponsePayload>> success(ModuleContext moduleContext) {
- final String id5Signature = moduleContext.getId5Signature();
- return update(id5SignatureEnrichmentChain(id5Signature), moduleContext);
+ return Future.succeededFuture(
+ InvocationResultImpl.<AuctionResponsePayload>builder()
+ .status(InvocationStatus.success)
+ .action(InvocationAction.no_action)
+ .moduleContext(moduleContext)
+ .analyticsTags(AnalyticTagsResolver.toEnrichResponseAnalyticTags(moduleContext))
+ .build());
}
@OverrideIf you disagree and there is a client-side reason the SDK needs the signature refreshed on auctions where nothing was enriched, say so and we will leave it as is.
3 participants
🔧 Type of changes
What's the context?
This update introduces hid (resolver hint) support together with the ID5 Mobile In-App resolver additions.
New features
hid-prefixes— a comma-separated set of id prefixes to emit ashid=, in the given order (e.g. "e,p,i6,a,g,c,c1"). Selection semantics: only the listed prefixes are sent as hid, unlike id-prefix-order which only orders.Bundleandverparams of Targeting API call - emit app bundle and version parameters to the Targeting API when they are present as part of the request (bidRequest.app).id5_signaturefrom client and propagate it as Targeting API query attribute (alongside email/phone/zip/vid).ext.prebid.passthrough.optable.id5_signatureon the next auction (step 3)🧪 Test plan
🏎 Quality check