OpenRTB: Fix request model types and preserve missing fields - #4626
pavel-ptashyts wants to merge 4 commits into
Conversation
CTMBNara
left a comment
CTMBNara
left a comment
There was a problem hiding this comment.
Check whether you need to add any logic to BidRequestOrtbVersionConverter.
Also, update ProtobufRequestUtils if needed
There was a problem hiding this comment.
Removed OpenRtbRequestModelTest as requested. The protobuf mapping changes are covered in the existing ProtobufRequestUtilsTest, and content preservation scenarios were added to the existing OrtbConverterSpec.
There was a problem hiding this comment.
Removed this file. The field/version explanations remain in the model JavaDoc and PR description.
|
Thanks for the review. I updated I also checked both The requested document and generic DTO test class are removed. I added scenarios to Validation: all 55 focused protobuf/version-converter Java tests pass, Checkstyle passes, and the Groovy scenarios compile. I attempted the new scenarios with a rebuilt server Docker image, but the local run stalled during global Testcontainers setup while copying files into MySQL, before any scenarios executed; I stopped that run rather than reporting it as passed. |
|
Hello, @pavel-ptashyts . Thanks for your contribution. There is a caveat: we made a conscious decision not to support those deprecated OpenRTB 2.5 fields. This is aligned between PBS-Java and PBS-Go. Could you please remove them from the PR? Thanks in advance. |
|
Thanks for clarifying, @Net-burst. I removed all six deprecated fields ( The PR now keeps only the OpenRTB 2.6 changes: string pod IDs for audio/video, the missing content/data fields, and the corrected Validation: all 52 focused protobuf and version-converter tests pass, as does Checkstyle. Production sources and Java/Groovy test sources compile. The new functional scenarios have not been verified at runtime because the previous local run stalled during Testcontainers setup. |
|
Hi @Net-burst could you review PR now. |
| Integer realtime | ||
| Integer firstbroadcast |
There was a problem hiding this comment.
Please change the types of realtime and firstbroadcast to specific enums, so their meaning is clear without additional context:
enum FirstBroadcast {
NOT_FIRST_BROADCAST(0), FIRST_BROADCAST(1)
@JsonValue
final Integer value
FirstBroadcast(Integer value) {
this.value = value
}
}
enum Realtime {
REPLAY(0), REAL_TIME(1)
@JsonValue
final Integer value
Realtime(Integer value) {
this.value = value
}
}
They should be located it: org.prebid.server.functional.model.request.auction
There was a problem hiding this comment.
also update field firstbroadcast with camelCase strategy name:
@JsonProperty("firstbroadcast")
FirstBroadcast firstbroadcast
There was a problem hiding this comment.
Updated the functional Content model to use Realtime and FirstBroadcast enums with numeric @jsonvalue mappings. I also renamed the Groovy property to firstBroadcast and kept the wire name via @JsonProperty("firstbroadcast"). Verified JSON serialization and deserialization for all four combinations of the two flags, including zero values.
| def "PBS should preserve content metadata for #distributionChannel when bidder supports ortb #ortbVersion"() { | ||
| given: "Content with genre taxonomy, broadcast flags and extended content identifiers" | ||
| def content = Content.defaultContent.tap { | ||
| gtax = 1 | ||
| genres = ["genre-001", "genre-002"] | ||
| realtime = realtimeFlag | ||
| firstbroadcast = firstbroadcastFlag | ||
| data = [new Data(id: "provider-id", cids: ["content-001", "00042"])] | ||
| } | ||
| def bidRequest = BidRequest.getDefaultBidRequest(distributionChannel).tap { | ||
| it[distributionChannel.value].content = content | ||
| } | ||
| def service = ortbVersion == "2.5" ? prebidServerServiceWithElderOrtb : prebidServerServiceWithNewOrtb | ||
|
|
||
| when: "Requesting a PBS auction" | ||
| service.sendAuctionRequest(bidRequest) | ||
|
|
||
| then: "The bidder receives the content metadata unchanged" | ||
| def actualContent = bidder.getBidderRequest(bidRequest.id)[distributionChannel.value].content | ||
| verifyAll(actualContent) { | ||
| gtax == content.gtax | ||
| genres == content.genres | ||
| realtime == realtimeFlag | ||
| firstbroadcast == firstbroadcastFlag | ||
| data == content.data | ||
| } | ||
|
|
||
| where: | ||
| distributionChannel | ortbVersion | realtimeFlag | firstbroadcastFlag | ||
| SITE | "2.5" | 0 | 1 | ||
| SITE | "2.6" | 1 | 0 | ||
| APP | "2.5" | 1 | 0 | ||
| APP | "2.6" | 0 | 1 | ||
| DOOH | "2.5" | 0 | 1 | ||
| DOOH | "2.6" | 1 | 0 | ||
| } |
There was a problem hiding this comment.
We shouldn't use magic numbers or logical operations directly inside test cases, as they make the test harder to understand.
Also, please replace hardcoded data with random values where possible, so it is clear that those values are not important for the test case.
Finally, please split this into separate test cases depending on the ORTB configuration, like this:
def "PBS should preserve content metadata for #distributionChannel with ORTB 2.5"() {
given: "Content with genre taxonomy, broadcast flags, and extended content identifiers"
def content = Content.defaultContent.tap {
it.gtax = PBSUtils.getRandomNumber()
it.genres = [PBSUtils.randomString, PBSUtils.randomString]
it.realtime = PBSUtils.getRandomEnum(Realtime)
it.firstbroadcast = PBSUtils.getRandomEnum(FirstBroadcast)
it.data = [new Data(id: PBSUtils.randomString, cids: [PBSUtils.randomString, PBSUtils.randomString])]
}
def bidRequest = BidRequest.getDefaultBidRequest(distributionChannel).tap {
it[distributionChannel.value].content = content
}
when: "Sending auction request to PBS"
def bidResponse = prebidServerServiceWithElderOrtb.sendAuctionRequest(bidRequest)
then: "PBS responds without errors or warnings"
assert !bidResponse.ext.errors
assert !bidResponse.ext.warnings
and: "PBS forwards the content metadata to the bidder unchanged"
def bidderRequest = bidder.getBidderRequest(bidRequest.id)
def actualContent = (bidderRequest[distributionChannel.value].content as Content)
verifyAll(actualContent) {
it.gtax == content.gtax
it.genres == content.genres
it.realtime == content.realtime
it.firstbroadcast == content.firstbroadcast
it.data == content.data
}
where:
distributionChannel << DistributionChannel.values()
}
def "PBS should preserve content metadata for #distributionChannel with ORTB 2.6"() {
given: "Content with genre taxonomy, broadcast flags, and extended content identifiers"
def content = Content.defaultContent.tap {
it.gtax = PBSUtils.getRandomNumber()
it.genres = [PBSUtils.randomString, PBSUtils.randomString]
it.realtime = PBSUtils.getRandomEnum(Realtime)
it.firstbroadcast = PBSUtils.getRandomEnum(FirstBroadcast)
it.data = [new Data(id: PBSUtils.randomString, cids: [PBSUtils.randomString, PBSUtils.randomString])]
}
def bidRequest = BidRequest.getDefaultBidRequest(distributionChannel).tap {
it[distributionChannel.value].content = content
}
when: "Sending auction request to PBS"
def bidResponse = prebidServerServiceWithNewOrtb.sendAuctionRequest(bidRequest)
then: "PBS responds without errors or warnings"
assert !bidResponse.ext.errors
assert !bidResponse.ext.warnings
and: "PBS forwards the content metadata to the bidder unchanged"
def bidderRequest = bidder.getBidderRequest(bidRequest.id)
def actualContent = (bidderRequest[distributionChannel.value].content as Content)
verifyAll(actualContent) {
it.gtax == content.gtax
it.genres == content.genres
it.realtime == content.realtime
it.firstbroadcast == content.firstbroadcast
it.data == content.data
}
where:
distributionChannel << DistributionChannel.values()
}
There was a problem hiding this comment.
Split this into separate ORTB 2.5 and 2.6 tests, each covering DistributionChannel.values(). Both now use randomized metadata and enum values, call the corresponding service directly, and assert that the response has no errors or warnings before checking the forwarded content. Checkstyle, compilation of the Java/Groovy sources and all 52 focused protobuf/version-converter Java tests pass. The Docker-backed functional scenarios have not been verified at runtime; the previous local run stalled during Testcontainers setup.
4 participants
Type of changes
What's the context?
The shared request model rejects alphanumeric OpenRTB 2.6 pod IDs and drops several content fields during JSON round trips. For example,
video.podid: "pod-001"cannot be decoded as an Integer, andcontent.genresandcontent.data[].cidsdisappear from the serialized request.Change
Video.podidandAudio.podidto String. AddContent.gtax,genres,realtime,firstbroadcast, andData.cids. Update thelivestreamJavaDoc to distinguish scheduled broadcasts from real-time events.Rationale behind the change
Deprecated OpenRTB 2.5 fields (
banner.wmax/hmax/wmin/hmin,video.protocol, andcontent.videoquality) remain unsupported, consistent with the existing PBS-Java/PBS-Go behavior. They are ignored during JSON decoding and are not preserved by the shared model or protobuf mapper.IAB OpenRTB 2.6 specifies string pod IDs for both Video and Audio.
The Go implementation made the same Video correction in prebid/openrtb#2. Both Video and Audio use string PodID.
The content attributes are defined in the IAB Content and Data tables.
Numeric JSON pod IDs are still accepted through existing Jackson coercion and are serialized as strings. Java consumers must update Integer pod ID builder arguments/getter usages to String and recompile. The separate video endpoint's numeric pod IDs are unchanged.
The 2.5/2.6 converters were checked: these fields introduce no new
ext-to-root relocation. Their existing behavior of retaining newer content and media fields for older adapters is preserved.Test plan
Focused run on JDK 25: 52 tests passed in
ProtobufRequestUtilsTest, both request version-converter test classes andBidRequestOrtbVersionConverterFactoryTest. Checkstyle passed; production, Java test and Groovy functional test sources compiled.Existing functional pod-ID cases use strings. New
OrtbConverterSpecscenarios cover content metadata anddata.cidsin site/app/DOOH for 2.5 and 2.6 adapters, using randomized metadata and typed broadcast flags, with separate tests for each adapter version and checks for response errors and warnings. These scenarios compile. A standalone check using the functional JSON mapper passed serialization and deserialization for all four broadcast flag combinations, including zero values and the firstbroadcast wire name. A local Docker run using the rebuilt server image was attempted but stopped during global Testcontainers setup: the JVM waited inCopyArchiveToContainerCmdwhile copying files into MySQL, before any scenarios ran. No functional pass is claimed.The generic DTO round-trip test class and separate compatibility document were removed following review.
Quality check
Follow project code style guidelines.
Breaking Java model API change is documented above.
No debug logging or temporary code added.
The existing JaCoCo configuration excludes
com/iab/openrtb/**; no DTO coverage percentage is claimed.