-
Notifications
You must be signed in to change notification settings - Fork 0
optable-targeting: Hid plus id5 Mobile In-app resolver params #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: optable-targeting-non-blocking-early-network-call
Are you sure you want to change the base?
Changes from all commits
aede90f
81c8810
a66497e
8902c11
073b1d5
65de1b6
bceaf21
41429ca
c59b63f
a54cecd
ea21f42
903aebb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package org.prebid.server.hooks.modules.optable.targeting.model; | ||
|
|
||
| import lombok.Value; | ||
|
|
||
| @Value(staticConstructor = "of") | ||
| public class App { | ||
|
|
||
| String bundle; | ||
|
|
||
| String ver; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,4 +23,8 @@ public class OptableAttributes { | |
| String userAgent; | ||
|
|
||
| Long timeout; | ||
|
|
||
| App app; | ||
|
|
||
| String id5Signature; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,4 +17,6 @@ public class ExtUserOptable extends FlexibleExtension { | |
| String zip; | ||
|
|
||
| String vid; | ||
|
|
||
| String id5Signature; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| import org.prebid.server.hooks.modules.optable.targeting.model.openrtb.TargetingResult; | ||
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.AnalyticTagsResolver; | ||
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.BidderRequestEnricher; | ||
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.Id5Resolver; | ||
| import org.prebid.server.hooks.v1.InvocationAction; | ||
| import org.prebid.server.hooks.v1.InvocationResult; | ||
| import org.prebid.server.hooks.v1.InvocationStatus; | ||
|
|
@@ -61,6 +62,7 @@ private Future<InvocationResult<BidderRequestPayload>> enrichedPayload(Targeting | |
|
|
||
| if (hasData) { | ||
| moduleContext.setTargeting(targetingResult.getAudience()); | ||
| moduleContext.setId5Signature(Id5Resolver.resolveId5Signature(targetingResult)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 So with |
||
| moduleContext.setEnrichRequestStatus(EnrichmentStatus.success()); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.AuctionResponseValidator; | ||
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.BidResponseEnricher; | ||
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.ConfigResolver; | ||
| import org.prebid.server.hooks.modules.optable.targeting.v1.core.Id5SignatureBidResponseEnricher; | ||
| import org.prebid.server.hooks.v1.InvocationAction; | ||
| import org.prebid.server.hooks.v1.InvocationResult; | ||
| import org.prebid.server.hooks.v1.InvocationStatus; | ||
|
|
@@ -62,15 +63,38 @@ public Future<InvocationResult<AuctionResponsePayload>> call(AuctionResponsePayl | |
|
|
||
| return validationStatus.getStatus() == Status.SUCCESS | ||
| ? enrichedPayload(moduleContext) | ||
| : success(moduleContext); | ||
| : enrichedById5SignaturePayload(moduleContext); | ||
| } | ||
|
|
||
| private Future<InvocationResult<AuctionResponsePayload>> enrichedPayload(ModuleContext moduleContext) { | ||
| final List<Audience> targeting = moduleContext.getTargeting(); | ||
| final String id5Signature = moduleContext.getId5Signature(); | ||
|
|
||
| return CollectionUtils.isNotEmpty(targeting) | ||
| ? update(BidResponseEnricher.of(targeting, objectMapper, jsonMerger), moduleContext) | ||
| : success(moduleContext); | ||
| ? update(fullEnrichmentChain(targeting, id5Signature), moduleContext) | ||
| : update(id5SignatureEnrichmentChain(id5Signature), moduleContext); | ||
| } | ||
|
|
||
| private Future<InvocationResult<AuctionResponsePayload>> enrichedById5SignaturePayload( | ||
| ModuleContext moduleContext) { | ||
|
|
||
| final String id5Signature = moduleContext.getId5Signature(); | ||
|
|
||
| return moduleContext.getOptableTargetingCall() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: NPE when
Please null-guard and fall back to the previous synchronous path, and add a test that runs the response hook with a
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Reproduced - a Note the The existing tests pass because they all hand the hook a future. Please add one that does not.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| .compose(targetingResult -> | ||
| update(id5SignatureEnrichmentChain(id5Signature), moduleContext)) | ||
| .recover(throwable -> success(moduleContext)); | ||
| } | ||
|
|
||
| private PayloadUpdate<AuctionResponsePayload> fullEnrichmentChain(final List<Audience> targeting, | ||
| final String id5Signature) { | ||
|
|
||
| return BidResponseEnricher.of(targeting, objectMapper, jsonMerger) | ||
| .andThen(Id5SignatureBidResponseEnricher.of(id5Signature, objectMapper, jsonMerger))::apply; | ||
| } | ||
|
|
||
| private PayloadUpdate<AuctionResponsePayload> id5SignatureEnrichmentChain(String id5Signature) { | ||
| return Id5SignatureBidResponseEnricher.of(id5Signature, objectMapper, jsonMerger); | ||
| } | ||
|
|
||
| private Future<InvocationResult<AuctionResponsePayload>> update( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package org.prebid.server.hooks.modules.optable.targeting.v1.core; | ||
|
|
||
| import com.fasterxml.jackson.core.JsonProcessingException; | ||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import org.prebid.server.hooks.modules.optable.targeting.model.openrtb.ExtUserOptable; | ||
| import org.prebid.server.json.ObjectMapperProvider; | ||
| import org.prebid.server.log.ConditionalLogger; | ||
| import org.prebid.server.log.LoggerFactory; | ||
|
|
||
| public class ExtUserOptableResolver { | ||
|
|
||
| private static final ConditionalLogger conditionalLogger = | ||
| new ConditionalLogger(LoggerFactory.getLogger(ExtUserOptableResolver.class)); | ||
|
|
||
| private ExtUserOptableResolver() { | ||
| } | ||
|
|
||
| public static ExtUserOptable resolveExtUserOptable(JsonNode node, double logSamplingRate) { | ||
| try { | ||
| return ObjectMapperProvider.mapper().treeToValue(node, ExtUserOptable.class); | ||
| } catch (JsonProcessingException e) { | ||
| conditionalLogger.warn("Can't parse $.ext.user.Optable tag", logSamplingRate); | ||
| return null; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| package org.prebid.server.hooks.modules.optable.targeting.v1.core; | ||
|
|
||
| import com.fasterxml.jackson.databind.JsonNode; | ||
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||
| import com.iab.openrtb.request.Eid; | ||
| import com.iab.openrtb.request.Uid; | ||
| import org.apache.commons.collections4.CollectionUtils; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.prebid.server.hooks.modules.optable.targeting.model.openrtb.Ortb2; | ||
| import org.prebid.server.hooks.modules.optable.targeting.model.openrtb.TargetingResult; | ||
| import org.prebid.server.hooks.modules.optable.targeting.model.openrtb.User; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
|
|
||
| public class Id5Resolver { | ||
|
|
||
| public static final String STR_OPTABLE_CO = "optable.co"; | ||
| public static final String STR_ID_5_SYNC_COM = "id5-sync.com"; | ||
| public static final String STR_OPTABLE = "optable"; | ||
| public static final String STR_REF = "ref"; | ||
| public static final String STR_SIGNATURE = "signature"; | ||
|
|
||
| private Id5Resolver() { | ||
| } | ||
|
|
||
| public static String resolveId5Signature(TargetingResult targetingResult) { | ||
| if (targetingResult == null) { | ||
| return null; | ||
| } | ||
|
|
||
| final List<String> refs = 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) | ||
| .filter(Objects::nonNull) | ||
| .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) | ||
| .toList(); | ||
|
|
||
| if (CollectionUtils.isEmpty(refs)) { | ||
| return null; | ||
| } | ||
|
|
||
| final ObjectNode references = targetingResult.getRefs(); | ||
| if (references == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return refs.stream() | ||
| .map(references::get) | ||
| .filter(Objects::nonNull) | ||
| .map(it -> it.get(STR_SIGNATURE)) | ||
| .filter(Objects::nonNull) | ||
| .filter(JsonNode::isValueNode) | ||
| .map(JsonNode::asText) | ||
| .filter(StringUtils::isNotBlank) | ||
|
Comment on lines
+65
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New issue introduced here: this can return the literal string A refs entry whose Reproduced with a refs entry of Fix: Unrelated nit while here:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| .findFirst() | ||
| .orElse(null); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed