-
Notifications
You must be signed in to change notification settings - Fork 224
New Module: LiveIntent Omni-channel Identity #3938
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
Conversation
| this.httpClient = Objects.requireNonNull(httpClient); | ||
| } | ||
|
|
||
| // TODO: Caching |
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.
How and whether or not to cache is being decided now.
|
@And1sS, Can you please have a look at this draft and leave me a couple of bullet points on what might be missing? |
|
Hi, yes, you got the general idea right. |
|
@And1sS It looks like a prebid-server test is constantly failing. But it does not seem to be related to my changes. |
|
|
||
| @Builder | ||
| @Data | ||
| @Jacksonized |
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.
We don't use this annotation. We try to avoid experimental features of Lombok as muck as possible. I also believe that it will work without that annotation.
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.
Changed to using NoArgsConstructor in b588f6a
| @Jacksonized | ||
| public class IdResResponse { | ||
|
|
||
| @JsonProperty("eids") |
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.
Useless annotation, please remove. Since you are using default mapper - it will deserialize by name of the field.
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.
Done in 3ed7ee8
| public final class ModuleConfig { | ||
|
|
||
| long requestTimeoutMs; | ||
| String identityResolutionEndpoint; |
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.
Please, add separation lines between all fields.
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.
Done in bedb969
| JacksonMapper mapper, | ||
| HttpClient httpClient) { | ||
|
|
||
| this(config, mapper, httpClient, new Random()); |
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.
This one won't work. Default Random class is not thread safe, and module can be called on any threads concurrently. Please, to simplify code, just leave one constructor with 'RandomGenerator' as parameter (for tests) and provide it with lambda that implements that functional interface using ThreadLocalRandom like this:
() -> ThreadLocalRandom.current().nextLong()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.
Done in d94e06f
| .action(InvocationAction.update) | ||
| .payloadUpdate(requestPayload -> updatedPayload(requestPayload, resolutionResult)) | ||
| .build() | ||
| ); |
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.
Please, see our styling doc on how we style our code and apply changes accordingly.
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.
Done in 61ceeed
| } | ||
|
|
||
| @Override | ||
| public String code() { |
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.
Same here about method order.
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.
Done in fa66d18
| "{\"eids\": [ { \"source\": \"liveintent.com\", " | ||
| + "\"uids\": [ { \"atype\": 3, \"id\" : \"some_id\" } ] } ] }", | ||
| IdResResponse.class); | ||
| // when and then |
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.
Please, add nice empty line before comment to separate test stages. Also, check for similar occurrences.
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.
Dine in 09450d2
|
@And1sS Addressed your comments. Please let me know if you see anything else that requires changes. |
|
@3link fyi maven resolution issue: |
|
@osulzhenko Should be fixed now. |
|
@And1sS Is anything missing for merging this PR? |
| @Configuration | ||
| @ConditionalOnProperty( | ||
| prefix = "hooks." + LiveIntentOmniChannelIdentityModule.CODE, name = "enabled", havingValue = "true" | ||
| ) |
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.
Please, change parantesis placement, look at our code style guidelines.
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.
Done in 5f15bf1
| this.config = Objects.requireNonNull(config); | ||
| this.mapper = Objects.requireNonNull(mapper); | ||
| this.httpClient = Objects.requireNonNull(httpClient); | ||
| this.random = random; |
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.
Objects.requiereNonNull here
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.
Done in 5f15bf1
| public Future<InvocationResult<AuctionRequestPayload>> call( | ||
| AuctionRequestPayload auctionRequestPayload, | ||
| AuctionInvocationContext invocationContext | ||
| ) { |
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.
Same here.
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.
Done in 5f15bf1
| .build()); | ||
|
|
||
| return update.onFailure(throwable -> logger.error("Failed enrichment:", throwable)); | ||
| } else { |
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.
No need for else clause, unnecessary nesting.
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.
Done in 5f15bf1
| final Future<InvocationResult<AuctionRequestPayload>> update = requestEnrichment(auctionRequestPayload) | ||
| .map(resolutionResult -> | ||
| InvocationResultImpl.<AuctionRequestPayload>builder() | ||
| .status(InvocationStatus.success) | ||
| .action(InvocationAction.update) | ||
| .payloadUpdate(requestPayload -> updatedPayload(requestPayload, resolutionResult)) | ||
| .build()); | ||
|
|
||
| return update.onFailure(throwable -> logger.error("Failed enrichment:", throwable)); |
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.
No need for local variable, just chain calls.
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.
Done in 5f15bf1
| + ", \"id\" : \"" | ||
| + enrichedUid.getId() + "\" } ] } ] }"); | ||
|
|
||
| when( |
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.
Same here, parentheses. Please, check for similar occurences.
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.
Done in b61986a
| httpClient.post( | ||
| eq(moduleConfig.getIdentityResolutionEndpoint()), | ||
| argThat(new ArgumentMatcher<MultiMap>() { | ||
| @Override | ||
| public boolean matches(MultiMap entries) { | ||
| return entries.contains("Authorization", "Bearer " + moduleConfig.getAuthToken(), true); | ||
| } | ||
| }), | ||
| eq(jacksonMapper.encodeToString(bidRequest)), | ||
| eq(moduleConfig.getRequestTimeoutMs()) | ||
| ) |
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.
Unreadable, please, extract matcher.
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.
Done in 5f15bf1
| assertThat(future).isNotNull(); | ||
| assertThat(future.succeeded()).isTrue(); |
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.
I mean, why bother checking for null? This is unit tests. It will crash anyways.
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.
Done in 5f15bf1
| assertThat(result).isNotNull(); | ||
| assertThat(result.status()).isEqualTo(InvocationStatus.success); | ||
| assertThat(result.action()).isEqualTo(InvocationAction.no_action); | ||
| assertThat(result.payloadUpdate()).isNull(); |
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.
assertThat(result).isEqualTo(
InvocationResult.<AuctionRequestPayload>builder()
.status(...)
.action(...)
.build());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.
Done in 5f15bf1
| <parent> | ||
| <groupId>org.prebid.server.hooks.modules</groupId> | ||
| <artifactId>all-modules</artifactId> | ||
| <version>3.27.0-SNAPSHOT</version> |
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.
Please, bump version to latest PBS version.
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.
Done in e458a4c
# Conflicts: # extra/bundle/pom.xml # extra/modules/pom.xml
# Conflicts: # extra/bundle/pom.xml # extra/modules/pom.xml
|
Closed in favor of #4127. |
|
Awesome, thanks for your answer |
🔧 Type of changes
✨ What's the context?
This PR introduces a new module that allows enriching auctions with user EIDs using LiveIntent's identity graph.
🏎 Quality check