Skip to content
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

only do video caching if we don't already have a videoCacheKey #2143

Merged

Conversation

snapwich
Copy link
Collaborator

Type of change

  • Bugfix

Description of change

addBidResponse should not cache video response if adapter already returns a videoCacheKey (indicating that it already cached).

src/auction.js Outdated
} else {
});
} else if (!bidResponse.vastUrl) {
bidResponse.vastUrl = getCacheUrl(bidResponse.videoCacheKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

We may not need line 249. When videoCacheKey exists, publisher is not using cache.url, publisher should use either Prebid Server cache with cacheMarkup (videoCacheKey = cache_id, vastUrl = cache_url) or its own cache server (videoCacheKey = imp_id or anything client side adapter wants, vastUrl = creative_depot_url or anything client side adapter wants). And getCacheUrl method is using cache.url to build the Url.

Copy link
Collaborator Author

@snapwich snapwich Feb 13, 2018

Choose a reason for hiding this comment

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

I know we're not using this line (because we return vastUrl with videoCacheKey), but I'm not sure if other's might use it. Is there no scenario where an adapter would return the videoCacheKey but expect that key to work against the url set with cache.url?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And if that scenario is not possible, then this line should probably be substituted with some kind of errors or warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, we can throw errors or warnings here. videoCacheKey and vastUrl should be well defined in all cache scenarios.

@snapwich snapwich assigned snapwich and matthewlane and unassigned snapwich Feb 15, 2018
@jaiminpanchal27 jaiminpanchal27 merged commit 448d4db into prebid:master Feb 22, 2018
mizmaar3 added a commit to widespace-os/Prebid.js that referenced this pull request Feb 28, 2018
* master:
  Update Vertoz adapter for Prebid 1.0 (prebid#2104)
  Add multiple bids request  (prebid#2136)
  [NEW Adapter] RTBHouseBidAdapter (prebid#2184)
  Update Innity Adapter to Prebid.js v1.0 (prebid#2180)
  Update Adyoulike Adapter to prebid 1.0 (prebid#2077)
  Change bidderCode for DAN Marketplace Bid Adapter (prebid#2183)
  only count bid timeouts if bidder didn't call done. fixes prebid#2146 (prebid#2154)
  [Edit BidAdapter] rxrtb adapter for Perbid.js 1.0 (prebid#2182)
  Update NasmediaAdmixer adapter (prebid#2164)
  only do video caching if we don't already have a videoCacheKey (prebid#2143)
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
…d#2143)

* only do video caching if we don't already have a videoCacheKey

* log error for missing vastUrl when videoCacheKey specified
@robertrmartinez robertrmartinez deleted the only-cache-without-videoCacheKey branch July 5, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants