New Module: ID5 User ID - #4318
pkowalski-id5 wants to merge 28 commits into
Conversation
ID5 basic module See merge request id5-sync/prebid-server-java!1
|
@CTMBNara Thank you for the review. I've updated the code. Could you please take another look? |
| @JsonProperty("us_privacy") | ||
| String usPrivacy; |
There was a problem hiding this comment.
No need for @JsonProperty here. snake_case is a standard for our object mapper
There was a problem hiding this comment.
Thank you for raising this. It made me realise how tightly coupled and risky it was to rely on the shared mapper.
The global ObjectMapper uses SNAKE_CASE today, but that is an implementation detail of the host application, not a guarantee this module can rely on. What's more, several fields use names that don't follow SNAKE_CASE at all like providerMetadata or _trace. They are defined by the ID5 API schema, not by any naming convention. Removing @JsonProperty and trusting the naming strategy would silently couple the wire format of ID5 requests to the
host configuration. If that ever changes, the breakage would be invisible at compile time and potentially very hard to trace in production.
HttpFetchClient will own a private ObjectMapper configured independently of the application-wide one. Together with explicit @JsonProperty annotations, this guarantees the request is serialized according to the ID5 API schema regardless of any changes to the host configuration.
|
I’ve updated the module based on your suggestions. Whenever you have a chance to re-review this, I’d love to hear your thoughts @Net-burst @CTMBNara |
Hello @pkowalski-id5! Thank you for your contribution. There are still a few comments that need to be addressed before we can proceed with merging your PR. |
|
@And1sS thanks for looking at this!
Can you help me better understand what is missing? Are there any issues not yet submitted, or are some of the submitted not fully resolved and cannot be approved? |
It looks like a few remarks from @CTMBNara are still pending. When you have a moment, could you please address them so we can continue with the review process? |
@And1sS @CTMBNara You can find the relevant updates in these commits: If I’ve missed anything specific, could you please point out which remarks are still outstanding? I’m happy to take another look |
|
@pkowalski-id5 Please, update your branch with latest master. |
…, WireMock scaffolding, InvocationContext httpMethod, alias vendor-id, module parent 4.2.0)
|
@CTMBNara, thanks for the review. I've updated the code as suggested. Please verify. |
| public List<Eid> toEids() { | ||
| return Optional.ofNullable(ids) | ||
| .map(userIds -> userIds.values().stream().map(UserId::eid).toList()) | ||
| .orElse(List.of()); |
There was a problem hiding this comment.
List.of -> Collections.emptyList
|
|
||
| public record Id5UserId(List<Eid> eids) { | ||
|
|
||
| private static final Id5UserId EMPTY = new Id5UserId(List.of()); |
There was a problem hiding this comment.
List.of -> Collections.emptyList
| return resultBuilder(invocationContext) | ||
| .status(InvocationStatus.success) | ||
| .action(InvocationAction.update) | ||
| .payloadUpdate(_ -> BidderRequestPayloadImpl.of(updatedBidRequest)) |
There was a problem hiding this comment.
Note: _ -> BidderRequestPayloadImpl.of(updatedBidRequest) means "override the result of the call to previous modules in this group"; if this is indeed the intended behavior, leave it as is.
There was a problem hiding this comment.
Remove wiremock dir and all of the content
| <dependencies> | ||
| <!-- TEST --> | ||
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter-test</artifactId> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| </dependencies> |
There was a problem hiding this comment.
We don't usually test the configuration, so you can remove this dependency and delete these tests.
| false, | ||
| null, | ||
| null // no Id5IdModuleContext provided | ||
| ); |
There was a problem hiding this comment.
move ); on previous line
| static Stream<Arguments> mergeEidsScenarios() { | ||
| final Eid existingEid = Eid.builder() | ||
| .source("other-sync.com") | ||
| .uids(List.of(Uid.builder().id("other-123").build())) | ||
| .build(); | ||
| return Stream.of( | ||
| Arguments.of("null user", | ||
| null, | ||
| List.of("id5-sync.com")), | ||
| Arguments.of("null user eids", | ||
| User.builder().build(), | ||
| List.of("id5-sync.com")), | ||
| Arguments.of("existing non-id5 eids", | ||
| User.builder().eids(List.of(existingEid)).build(), | ||
| List.of("other-sync.com", "id5-sync.com")) | ||
| ); | ||
| } | ||
|
|
||
| @ParameterizedTest(name = "{0}") | ||
| @MethodSource("mergeEidsScenarios") |
There was a problem hiding this comment.
Replace with 3 different tests
| // when | ||
| final InvocationResult<BidderRequestPayload> result = hook.call(BidderRequestPayloadImpl.of(bidRequest), | ||
| bidderCtx) | ||
| .toCompletionStage().toCompletableFuture().join(); |
There was a problem hiding this comment.
There is no real async so Future.result is enough
| AuctionContext.builder().account(Account.builder().id("acc").build()).build(), | ||
| false, | ||
| null, | ||
| // provide module context with empty Ids future to ensure fetch path would continue if not filtered |
|
|
||
| class Id5IdInjectHookTest { | ||
|
|
||
| private BidderInvocationContextImpl bidderCtxWithEmptyIds() { |
There was a problem hiding this comment.
Move all utility methods to the bottom of the tests class. Also, add static modifier if possible
5 participants
🔧 Type of changes
✨ What's the context?
ID5 wants to have its own module that can enrich bid requests with ID5 universal identifiers. This module fetches identity signals from ID5's API and automatically injects them into OpenRTB bid requests as Extended
Identifiers (EIDs).
🧠 Rationale behind the change
Why a dedicated module?
Key design decisions:
ProcessedAuctionRequestHook): Initiates async ID5 API call early in the request lifecycleBidderRequestHook): Injects EIDs into each bidder request only when not already presentTrade-offs:
🔎 New Bid Adapter Checklist
Not applicable - this is a module, not a bid adapter🧪 Test plan
Unit Tests:
Integration Tests:
sample/directoryManual Testing:
The module has been tested with various configurations and filter combinations to ensure safe production deployment.
🏎 Quality check
🤔 Questions for reviewers
1. Hook invocation status/action handling:
We would particularly appreciate review of our hook invocation status and action returns. Please verify that we're correctly returning:
InvocationStatusvalues in different scenarios (success, failure, skipped)InvocationActionto control execution flow2. Execution plan configuration simplification:
Both hooks (fetch + inject) are required for the module to function - the module is non-functional if either hook is missing from the execution plan. Currently, operators must configure both hooks separately in the
execution plan:
Question: Is there a way to simplify this configuration? For example: