Native template fixes perbid by william-harris · Pull Request #30 · yieldlab/Prebid.js

william-harris · 2026-08-28T07:22:16Z

No description provided.

NOTE: one test is deliberately RED and fails on this commit - "a publisher using the standard mediaTypes.native shorthand gets a usable native bid" It expresses the desired behaviour rather than the current one. The following commit adds the facet-type mapping fix and re-scopes it. Suite here is 50 passed / 1 failed; that failure is the bug, not a broken test.

Fixes the native regression documented by the tests in the previous commit.

github-actions · 2026-08-28T07:25:49Z

This PR introduces changes that may not work on all browsers. According to Babel, the following polyfills may be needed, and they are not automatically included:

Changes to modules/yieldlabBidAdapter.js may need:
- es.iterator.filter
- esnext.iterator.filter
Changes to test/spec/modules/yieldlabBidAdapter_spec.js may need:
- es.array.push
- es.iterator.for-each
- es.iterator.map
- esnext.iterator.for-each
- esnext.iterator.map

The best way to address this is to provide good test coverage, as normal PR checks run unit tests on older browsers.

nkloeber

Overall, I think the migration is good. Before upstreaming, I would address the correctness/compatibility issues from the review, especially the single-media-type requirement, floor handling, customParams, native reconciliation and request-id handling.

For the final public-PR cleanup, could we also:

update modules/yieldlabBidAdapter.md for the new /ortb flow and its compatibility constraints, and link any docs.prebid.org PR in the description
add and export types for the public bidder params (adslotId, supplyId, targeting, extId, customParams)
squash 9cb47c1a5 with 54c9c0708 -> the former deliberately leaves one test red and the latter fixes it, so the public history shouldn't contain a failing commit
rebase onto current upstream/master and rerun the adapter tests

I'd also keep the usual Yieldlab Bid Adapter: prefix -> the last two commits currently use Yieldlab:.

nkloeber · 2026-08-31T05:55:27Z

+ * Rewrite the DSP's native asset ids onto the ids the publisher asked for.
+ *
+ * The adserver builds its outgoing native request from the adslot's configured
+ * native template and deliberately ignores `imp.native.request` (YL-6463), so the


Remove the ticket number before the public PR.

Keep the reason for the code, for example:

The endpoint may return native asset ids that do not match the requested ids, so assets are matched by facet before validation.

nkloeber · 2026-08-31T06:06:23Z

+      expect(res[0]).to.not.have.property('dealId');
+    });
+  });
+  describe('facet-type mapping as a candidate fix for the native id mismatch', () => {


I would remove this test.

It tests a local helper and Prebid core behaviour, not the adapter.

The PR already has proper adapter tests under describe('native asset-id contract with the adserver native template').

The useful conclusion can go into the PR description instead, i.e.:

Response-side mapping cannot create a required native asset that is missing from the template.

nkloeber · 2026-08-31T06:31:31Z

 * @returns {Object}                  The finalized OpenRTB `imp` object.
 */
 function impFn(buildImp, bidRequest, context) {
  const imp = buildImp(bidRequest, context);


I think we need to select the media type before calling buildImp().

Our /ortb endpoint only accepts one of banner, video, native or audio per imp. Since context.mediaType is currently unset, the ORTB converter includes every format declared on a multi-format ad unit.

For example, banner + video becomes an imp containing both imp.banner and imp.video. The backend drops that imp before adslot resolution and returns 204 if it was the only one.

The converter already supports this explicitly: its context.mediaType JSDoc says that setting it disables imp generation for the other media types and also passes the selected type to getFloor().

Could we therefore set it before building the imp?

context.mediaType = derivePrimaryMediaType(bidRequest); const imp = buildImp(bidRequest, context);

That would make the generated imp and its floor use the same media type.

nkloeber · 2026-08-31T06:40:26Z

+      expect(ortb.imp[0]).to.have.property('bidfloorcur', 'EUR');
+    });
+
+    it('prefers banner over video on a multiformat ad unit', () => {


I think this test should also assert the resulting imp shape.

The important endpoint constraint is that an imp must contain exactly one media type. Right now this test confirms that the floor prefers banner, but the generated imp still contains both banner and video.

Could we assert that only the selected media type is present on the imp?

This depends on the context.mediaType change I suggested at line 707

nkloeber · 2026-08-31T06:46:07Z

+    return;
+  }

  if (floor && floor.currency === CURRENCY_CODE && Number.isFinite(floor.floor)) {


I think we should allow USD here as well.

The /ortb backend supports top-level bidfloor in EUR and USD and converts USD internally. With the current check, an unconverted USD floor from Prebid gets dropped completely.

Could we keep EUR/USD floors unchanged and only drop other currencies?

nkloeber · 2026-08-31T08:14:47Z

 * Sanitize video asset fields on a bid.
 *
 * - Only runs for `mediaType: 'video'`
 * - Trims strings and clears blanks for `vastUrl` / `vastXml`


Small docs nit: this comment says the function trims strings, but the implementation doesn't seem to do that anymore.

nkloeber · 2026-08-31T08:18:04Z

 }

 /**
 * Apply `netRevenue` from the bid response `seatbid[].bid[].ext.netRevenue`.


Small typo in the JSDoc: the field is ext.netrevenue, not ext.netRevenue.

nkloeber · 2026-08-31T08:19:20Z


  let parent;
  if (ortb.app) {
    ortb.app = ortb.app || {};


Is this still needed? I don't see anything added to ortb.app afterwards, so this looks dead now.

nkloeber · 2026-08-31T08:23:57Z


 /**
 * Add advertiser domains fallback and pass DSA if provided.
 * (Used in bidResponse and interpretResponse.)


This docblock says Used in bidResponse and interpretResponse., but the direct call in interpretResponse is gone, the only call site now is bidResponseFn

nkloeber · 2026-08-31T14:15:41Z

      ortb.tmax = (bidderReq?.timeout != null) ? bidderReq.timeout : ortb.tmax;

      if (bidderReq?.auctionId) {
        ortb.id = bidderReq.auctionId;


Do we need to override the request id with auctionId here?

The converter already provides $.id, while Prebid treats the auction identifier separately as $.source.tid. bidderRequest.auctionId resolves to that TID, so this copies the auction TID into the request-id field and changes the meaning of $.id depending on whether TIDs are enabled.

Our /ortb backend only requires id to be non-blank and echoes it back, and the legacy adapter didn't send the auction id either.

I think we can remove this override and keep the converter-generated id.

William Harris added 2 commits August 26, 2026 09:21

Yieldlab: reconcile native asset ids by facet type

54c9c07

Fixes the native regression documented by the tests in the previous commit.

nkloeber requested changes Aug 31, 2026

View reviewed changes

Navigation Menu

Uh oh!

Native template fixes perbid - #30

Native template fixes perbid#30
william-harris wants to merge 2 commits into
Yieldlab_Bid_Adapter_Switch_To_Post_Open_RTB_Converterfrom
native-template-fixes-perbid

william-harris commented Aug 28, 2026

Uh oh!

github-actions Bot commented Aug 28, 2026

Uh oh!

nkloeber left a comment

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

nkloeber Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

william-harris commented Aug 28, 2026

Uh oh!

github-actions Bot commented Aug 28, 2026

Uh oh!

nkloeber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants