pubmatic: return error for banner with no sizes and no format - #4643
Open
Aditya-9-6 wants to merge 3 commits into
Open
Aditya-9-6 wants to merge 3 commits into
Aditya-9-6 wants to merge 3 commits into
Conversation
Port of Go fix from prebid-server#5008 (prebid/prebid-server#5008). In Go, assignBannerSize() returns a BadInput error when banner.Format is empty and no explicit W/H are present, preventing a panic. The Java implementation had the same logical gap: assignSizesIfMissing() silently returned the banner unchanged when format was empty, forwarding a dimensionless banner to PubMatic. This commit: - Renames assignSizesIfMissing -> fillBannerSizeFromFormat to clarify its role (only copies dims from Format[0]; does not validate) - Adds a post-enrichment check in modifyImp() that throws PreBidException when the resulting banner still has no W/H after both fillBannerSizeFromFormat and enrichWithAdSlotParameters have run - This preserves the valid case where adSlot (e.g. slot@300x250) supplies dimensions even when format is empty Closes #5002 (see also prebid/prebid-server#5002)
Per OpenRTB 2.6 spec, banner sizes are optional for interstitial impressions (imp.instl=1), as interstitials fill the full screen. The previous fix incorrectly discarded these legitimate impressions. Skip the no-sizes PreBidException when the imp has instl=1, even if both W/H are absent and format is empty.
|
The new check only rejects when both w and h are missing. With w=300, h=null, no format, and an adSlot without size, the banner still passes through. Could the check reject either missing dimension? |
Update banner dimensions validation to check if either width or height is missing ((w == null || h == null)), preventing banners with only one dimension set from passing through.
Author
|
Good catch! Updated the check to (resultBanner.getW() == null || resultBanner.getH() == null)\ so it rejects if either dimension is missing when no format/adSlot dimensions are available, and added a unit test for \w=300, h=null. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is a Java port of the Go fix from prebid/prebid-server#5008, as requested in issue prebid/prebid-server#5002 (comment by @osulzhenko).
Problem
PubmaticBidder.assignSizesIfMissing()silently returned the banner unchanged whenformatwas empty and no explicitW/Hwere set, forwarding a dimensionless banner to PubMatic — leading to invalid requests.Go's equivalent (
assignBannerSize) was already fixed to return aBadInputerror in such cases. Java was missing the same guard.What is different in Java vs Go
The call order in Java differs from Go:
fillBannerSizeFromFormat()(formerlyassignSizesIfMissing) is called beforeenrichWithAdSlotParameters()adSlotcontains@WxH(e.g.slot@300x250),enrichWithAdSlotParameterssetsW/Hon the imp builder after the format-fill stepSo we cannot throw eagerly in
fillBannerSizeFromFormat— a valid imp with emptyformatbutadSlot-derived dimensions would be incorrectly rejected.Fix
assignSizesIfMissing→fillBannerSizeFromFormatto clarify its single responsibility (copying dims fromformat[0]when W/H are absent)modifyImp(): if the resulting banner still has noW/Hafter both steps, throwPreBidException("No sizes provided for Banner")Tests added
makeHttpRequestsShouldReturnErrorIfBannerHasNoSizesAndNoFormat— banner with empty format and adSlot without@→ BadInput errormakeHttpRequestsShouldAllowBannerWithNoFormatWhenAdSlotProvidesSize— banner with empty format butadSlot = slot@300x250→ W=300, H=250 from adSlot, no errorFixes #5002 (cross-repo issue from prebid/prebid-server#5002)