Skip to content

OMS: add video support #3779

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.

Sign up for GitHub

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

Merged
CTMBNara merged 7 commits into from
Apr 17, 2025
Merged

OMS: add video support #3779

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d87670e
OMS: Video support #3770
przemkaczmarek Feb 24, 2025
6459db5
fix comments
przemkaczmarek Mar 5, 2025
f32d656
fix comments
przemkaczmarek Mar 10, 2025
ed5230c
fix comments
przemkaczmarek Mar 11, 2025
057d343
fix comments
przemkaczmarek Mar 12, 2025
2d95af3
fix comments
przemkaczmarek Mar 19, 2025
cef4cb6
fix comments
przemkaczmarek Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 18 additions & 26 deletions src/main/java/org/prebid/server/bidder/oms/OmsBidder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,8 @@ public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest request
if (!request.getImp().isEmpty()) {
try {
final ExtImpOms impExt = parseImpExt(request.getImp().getFirst());
final String publisherId = impExt.getPid() == null
&& impExt.getPublisherId() != null
&& impExt.getPublisherId() > 0
? String.valueOf(impExt.getPublisherId())
: impExt.getPid();
final String publisherId = impExt.getPublisherId() != null && impExt.getPublisherId() > 0
? String.valueOf(impExt.getPublisherId()) : null;
Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025

Choose a reason for hiding this comment

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

why did you change it? I believe in the last commit it was correct

Copy link
Collaborator Author

@przemkaczmarek przemkaczmarek Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

because U said that pid is deprecated

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025

Choose a reason for hiding this comment

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

deprecated doesn't mean removed

Copy link
Collaborator Author

@przemkaczmarek przemkaczmarek Mar 10, 2025

Choose a reason for hiding this comment

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

so i should add only annotation @deprecated on pid filed ?

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 11, 2025

Choose a reason for hiding this comment

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

the initial point was about using publisherId in the integration test instead of pid, that's it, everything else was fine, the pid is still supported

Copy link
Collaborator Author

@przemkaczmarek przemkaczmarek Mar 11, 2025

Choose a reason for hiding this comment

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

In the integration test I used only publisherId.
I used pid only in unit test.
Should it look like this?

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 11, 2025

Choose a reason for hiding this comment

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

yes

final String url = "%s?publisherId=%s".formatted(endpointUrl, publisherId);
return Result.withValue(BidderUtil.defaultRequest(request, url, mapper));
} catch (PreBidException e) {
Expand Down Expand Up @@ -91,37 +88,32 @@ private static List<BidderBid> bidsFromResponse(BidResponse bidResponse) {
.map(SeatBid::getBid)
.filter(Objects::nonNull)
.flatMap(Collection::stream)
.map(bid -> BidderBid.builder()
.bid(bid)
.type(getBidType(bid))
.bidCurrency(bidResponse.getCur())
.videoInfo(videoInfo(bid))
.build())
.map(bid -> {
final BidType bidType = getBidType(bid);
return BidderBid.builder()
.bid(bid)
.type(bidType)
.bidCurrency(bidResponse.getCur())
.videoInfo(videoInfo(bidType, bid))
.build();
})
.toList();
}

private static BidType getBidType(Bid bid) {
final Integer markupType = bid.getMtype();
return switch (markupType) {
case 1 -> BidType.banner;
case 2 -> BidType.video;
case null, default -> BidType.banner;
};
return Objects.equals(bid.getMtype(), 2) ? BidType.video : BidType.banner;
}

private static ExtBidPrebidVideo videoInfo(Bid bid) {
if (!Integer.valueOf(2).equals(bid.getMtype())) {
private static ExtBidPrebidVideo videoInfo(BidType bidType, Bid bid) {
if (bidType != BidType.video) {
return null;
}
final List<String> cat = bid.getCat();
final Integer duration = bid.getDur();

final boolean catNotEmpty = CollectionUtils.isNotEmpty(cat);
final boolean durationValid = duration != null && duration > 0;
return catNotEmpty || durationValid
? ExtBidPrebidVideo.of(
durationValid ? duration : null,
catNotEmpty ? cat.getFirst() : null)
: null;
return ExtBidPrebidVideo.of(
duration != null ? duration : 0,
Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 11, 2025

Choose a reason for hiding this comment

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

ObjectUtils.defaultIfNull

CollectionUtils.isNotEmpty(cat) ? cat.getFirst() : ""
Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025

Choose a reason for hiding this comment

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

StringUtils.EMPTY

);
}
}
1 change: 0 additions & 1 deletion src/main/java/org/prebid/server/proto/openrtb/ext/request/omx/ExtImpOms.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
@Value(staticConstructor = "of")
public class ExtImpOms {

String pid;
Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025

Choose a reason for hiding this comment

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

do not need to remove, since it's present in Go

@JsonProperty("publisherId")
Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025

Choose a reason for hiding this comment

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

add an empty line

Integer publisherId;
Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 5, 2025

Choose a reason for hiding this comment

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

add JsonProperty

also I would change the integration test payload to cover publisherId parameter

Copy link
Collaborator Author

@przemkaczmarek przemkaczmarek Mar 5, 2025

Choose a reason for hiding this comment

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

I have cheanged pid to publisherId but IMO its indifferent

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025

Choose a reason for hiding this comment

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

the pid is declared as deprecated

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I meant, it makes sense to add a publisherId to the test instead of deprecated pid, but pid is still can be used, it's deprecated, not removed, please revert this change

}
18 changes: 1 addition & 17 deletions src/test/java/org/prebid/server/bidder/oms/OmsBidderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,26 +57,10 @@ public void makeHttpRequestsShouldReturnErrorWhenRequestHasInvalidImpression() {
assertThat(result.getErrors()).hasSize(1).first().isEqualTo(badInput("Invalid ext. Imp.Id: 123"));
}

@Test
public void makeHttpRequestsShouldCreateExpectedUrl() {
// given
final ExtImpOms impExt = ExtImpOms.of("otherTagId", 12345);
final BidRequest bidRequest = givenBidRequest(impCustomizer -> impCustomizer.ext(givenImpExt(impExt)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);

// then
assertThat(result.getErrors()).isEmpty();
assertThat(result.getValue()).hasSize(1)
.extracting(HttpRequest::getUri)
.containsExactly("https://randomurl.com?publisherId=otherTagId");
}

@Test
public void makeHttpRequestsShouldCreateExpectedUrlWithPublisherId() {
// given
final ExtImpOms impExt = ExtImpOms.of(null, 12345);
final ExtImpOms impExt = ExtImpOms.of(12345);
final BidRequest bidRequest = givenBidRequest(impCustomizer -> impCustomizer.ext(givenImpExt(impExt)));

// when
Expand Down
Loading