Skip to content

Conversation

@3link
Copy link

@3link 3link commented Apr 25, 2025

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

This PR introduces a new module that allows enriching auctions with user EIDs using LiveIntent's identity graph.

🏎 Quality check

  • Are your changes following our code style guidelines?
  • Are there any breaking changes in your code? <- no
  • Does your test coverage exceed 90%?
  • Are there any erroneous console logs, debuggers or leftover code in your changes? <- no

this.httpClient = Objects.requireNonNull(httpClient);
}

// TODO: Caching
Copy link
Author

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.

@3link
Copy link
Author

3link commented Apr 28, 2025

@And1sS, Can you please have a look at this draft and leave me a couple of bullet points on what might be missing?

@And1sS
Copy link
Collaborator

And1sS commented Apr 29, 2025

Hi, yes, you got the general idea right.

@3link 3link changed the title WIP - LiveIntent Module New Module: LiveIntent Omni-channel Identity May 7, 2025
@3link 3link marked this pull request as ready for review May 7, 2025 11:12
@3link
Copy link
Author

3link commented May 21, 2025

@And1sS It looks like a prebid-server test is constantly failing. But it does not seem to be related to my changes.

Error:  Failures: 
Error:    StoredResponseSpec.PBS should set seatBids in response from multiple imp.ext.prebid.storedBidResponse.seatbidobj when it is defined:339 Condition not satisfied:

@3link 3link closed this May 21, 2025
@3link 3link reopened this May 21, 2025
@osulzhenko osulzhenko requested a review from And1sS May 26, 2025 09:41

@Builder
@Data
@Jacksonized
Copy link
Collaborator

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.

Copy link
Author

@3link 3link Jun 11, 2025

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")
Copy link
Collaborator

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.

Copy link
Author

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;
Copy link
Collaborator

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.

Copy link
Author

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());
Copy link
Collaborator

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()

Copy link
Author

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()
);
Copy link
Collaborator

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.

Copy link
Author

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() {
Copy link
Collaborator

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.

Copy link
Author

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
Copy link
Collaborator

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dine in 09450d2

@3link
Copy link
Author

3link commented Jun 11, 2025

@And1sS Addressed your comments. Please let me know if you see anything else that requires changes.

@osulzhenko
Copy link
Collaborator

@3link fyi maven resolution issue:

Error: ] Some problems were encountered while processing the POMs:
[FATAL] Non-resolvable parent POM for org.prebid.server.hooks.modules:live-intent-omni-channel-identity:3.26.0-SNAPSHOT: The following artifacts could not be resolved: org.prebid.server.hooks.modules:all-modules:pom:3.26.0-SNAPSHOT (absent): Could not find artifact org.prebid.server.hooks.modules:all-modules:pom:3.26.0-SNAPSHOT and 'parent.relativePath' points at wrong local POM @ line 5, column 13
 @ 
Error:  The build could not read 1 project -> [Help 1]
Error:    
Error:    The project org.prebid.server.hooks.modules:live-intent-omni-channel-identity:3.26.0-SNAPSHOT (/home/runner/work/prebid-server-java/prebid-server-java/extra/modules/live-intent-omni-channel-identity/pom.xml) has 1 error
Error:      Non-resolvable parent POM for org.prebid.server.hooks.modules:live-intent-omni-channel-identity:3.26.0-SNAPSHOT: The following artifacts could not be resolved: org.prebid.server.hooks.modules:all-modules:pom:3.26.0-SNAPSHOT (absent): Could not find artifact org.prebid.server.hooks.modules:all-modules:pom:3.26.0-SNAPSHOT and 'parent.relativePath' points at wrong local POM @ line 5, column 13 -> [Help 2]

@3link
Copy link
Author

3link commented Jun 11, 2025

@osulzhenko Should be fixed now.

@3link
Copy link
Author

3link commented Jun 24, 2025

@And1sS Is anything missing for merging this PR?

@Configuration
@ConditionalOnProperty(
prefix = "hooks." + LiveIntentOmniChannelIdentityModule.CODE, name = "enabled", havingValue = "true"
)
Copy link
Collaborator

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.

Copy link
Contributor

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects.requiereNonNull here

Copy link
Contributor

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
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor

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 {
Copy link
Collaborator

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5f15bf1

Comment on lines 61 to 69
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));
Copy link
Collaborator

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.

Copy link
Contributor

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(
Copy link
Collaborator

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.

Copy link
Contributor

@SuperIzya SuperIzya Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b61986a

Comment on lines 96 to 106
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())
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreadable, please, extract matcher.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5f15bf1

Comment on lines 144 to 145
assertThat(future).isNotNull();
assertThat(future.succeeded()).isTrue();
Copy link
Collaborator

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5f15bf1

Comment on lines 146 to 149
assertThat(result).isNotNull();
assertThat(result.status()).isEqualTo(InvocationStatus.success);
assertThat(result.action()).isEqualTo(InvocationAction.no_action);
assertThat(result.payloadUpdate()).isNull();
Copy link
Collaborator

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());

Copy link
Contributor

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>
Copy link
Collaborator

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e458a4c

ilya added 5 commits July 29, 2025 17:33
# Conflicts:
#	extra/bundle/pom.xml
#	extra/modules/pom.xml
# Conflicts:
#	extra/bundle/pom.xml
#	extra/modules/pom.xml
@SuperIzya
Copy link
Contributor

@And1sS, I'll repeat @3link's question: Is anything missing for merging this PR?

@osulzhenko osulzhenko requested a review from And1sS August 14, 2025 07:01
@And1sS
Copy link
Collaborator

And1sS commented Aug 15, 2025

Closed in favor of #4127.
Basically, my colleague took your pr and polished it.

@wi101
Copy link

wi101 commented Aug 22, 2025

Hi @And1sS
Thanks! I saw the PR has been merged.
So could we close this one?

@And1sS
Copy link
Collaborator

And1sS commented Aug 22, 2025

Hi @And1sS

Thanks! I saw the PR has been merged.

So could we close this one?

Yes

@wi101
Copy link

wi101 commented Aug 22, 2025

Awesome, thanks for your answer
Could you help me to close it? because I don't have access

@osulzhenko osulzhenko added the do not merge Not the time for merging yet label Sep 4, 2025
@Net-burst Net-burst closed this Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do not merge Not the time for merging yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants