Pass Through First Party Context Data #1479

SyntaxNode · 2020-09-04T17:48:13Z

PBS-Go does not yet support the First Party Data feature (#879), but there is a conflict with how we map bidder data and it's causing issues for some publishers. We'll need to fix this eventually, so why not now?

The First Party Data feature allows for context to be provided on an imp to be shared among all bidders, such as:

{
    imp: [
        ...
        ext: {
            context: {
                data: {
                    // everyone sees this data
                    ADUNIT SPECIFIC CONTEXT DATA
                }
            }
         }
    ]
}

However, PBS-Go expects all fields within the "ext" to either be bidder data or the prebid section, such as:

{
    imp: [
        ...
        ext: {
            appnexus: {
                        placementId: 1
            },
            context: {
                data: {
                    // everyone sees this data
                    ADUNIT SPECIFIC CONTEXT DATA
                }
            }
        }
    ]
}

We attempt to validate that context is a valid bidder and return an error. We already have a special case for the prebid section. This PR also adds a special case for the context section, to pass it down to all bid requests.

SyntaxNode · 2020-09-04T17:52:48Z

endpoints/openrtb2/auction_test.go

+			requestExt:   json.RawMessage(`{"prebid":{"bidder":{"appnexus":{"placement_id":555}}}}`),
+			expectedExt:  `{"prebid":{"bidder":{"appnexus":{"placement_id":555}}}}`,
+			expectedErrs: []error{},
+			// request.ext.prebid.bidder.{biddername} is only promoted/copied to request.ext.{biddername} if there is at least one disabled bidder.


We need to talk about this behavior. It makes no sense to me and looks like a bug.

I can understand removing a disabled bidder, but that's not happening for imp.ext.bidder definitions.

I have no idea why we're copying the bidder up a level here. That hoisting is already performed by the exchange during imp sanitization. Maybe this was added first and never removed?

I thought about this more and now consider it a bug. As a follow-up PR, I'd like to make the validate method only actually validate and move the disabled bidder logic to exchange.go, if it's not already there.

SyntaxNode · 2020-09-04T17:55:41Z

endpoints/openrtb2/sample-requests/first-party-data/valid-context-allowed.json

+                "appnexus": {
+                    "placementId": 12883451
+                },
+                "context": {


This throws a validation exception currently.

SyntaxNode · 2020-09-04T17:56:22Z

exchange/exchangetest/first-party-data-multiple-bidders.json

+                            "bidder": {
+                                "placementId": 1
+                            },
+                            "context": {


This is considered a bidder today and causes all sorts of panics currently (when the validation now allows it).

SyntaxNode · 2020-09-04T17:57:07Z

exchange/utils.go

 // It will not mutate the input imp.
 // This function will write the new imps to the output map passed in
 func sanitizedImpCopy(imp *openrtb.Imp,
 	bidderExts map[string]json.RawMessage,
 	rawPrebidExt json.RawMessage,
+	firstPartyDataContext json.RawMessage,


Yes, this is hacky. This method is in need of a refactor. I wanted to keep my "new feature' hat on for now and limited the scope of this PR from a full on refactor.

SyntaxNode · 2020-09-04T17:57:49Z

exchange/utils.go

-			errs = append(errs, err)
+		// Remove the entire bidder field. We will already have the content we need in bidderExts. We
+		// don't want to include other demand partners' bidder params in the sanitized imp.
+		if _, hasBidderField := prebidExt["bidder"]; hasBidderField {


Well, except for this slight optimization. No need to marshal if there was no bidder section to remove.

hhhjort · 2020-09-09T18:15:33Z

endpoints/openrtb2/auction.go

+	// ExtRequestFirstPartyDataContext is a special case for the first party data context section
+	// and is not considered a bidder.
+
+	return bidder != openrtb_ext.PrebidExtKey && bidder != openrtb_ext.ExtRequestFirstPartyDataContext


Should it be RequestFirstPartyDataContextExtKey to match the PrebidExtKey pattern?

I'll rename it to FirstPartyDataContextExtKey. Push incoming.

hhhjort

It is a bit hacky, but doing better does require a major refactor which is better off in its own PR.

mansinahar · 2020-09-10T15:34:05Z

endpoints/openrtb2/auction_test.go

-		hardcodedResponseIPValidator{response: true},
+
+	for _, test := range testCases {
+		deps := &endpointDeps{


I'd suggest moving the deps assignment outside the for loop as it is common for all test cases.

mansinahar · 2020-09-10T15:38:23Z

exchange/utils.go

 			errList = append(errList, errs...)
 		}
 	}

 	return splitImps, nil
 }

-// sanitizedImpCopy returns a copy of imp with its ext filtered so that only "prebid" and bidder params exist.
+// sanitizedImpCopy returns a copy of imp with its ext filtered so that only "prebid", "comtext", and bidder params exist.


Typo: "context"

I cna't spell. Fixed.

mansinahar · 2020-09-10T15:46:47Z

openrtb_ext/bidders_test.go

@@ -61,3 +61,8 @@ func TestBidderListDoesNotDefineGeneral(t *testing.T) {
 	bidders := BidderList()
 	assert.NotContains(t, bidders, BidderNameGeneral)
 }
+
+func TestBidderListDoesNotDefineContext(t *testing.T) {


If the BidderNameContext is only being used in this test, can we just define that as a constant here in this specific test file? The reason being it can be otherwise easily confused with FirstPartyDataContextExtKey which is what we're actually using when looking at the ext to separate it out from the bidders

I included BidderNameContext alongside the other bidder name definitions to act as documentation of expected cases while also providing a BidderName instance (FirstPartyDataContextExtKey is a string). I used the same pattern for BidderNameGeneral for the same reason.

The variables also refer to slightly different things:
BidderNameContext = A reserved bidder name due to a conflict with another imp.ext use case. Will eventually go away when we move entirely to imp.ext.prebid.bidder.xxx

FirstPartyDataContextExtKey = Name of the first party data context imp.ext field. Likely to be used more in the future when building out the first party data feature.

SyntaxNode added 3 commits September 4, 2020 13:39

Pass Through First Party Context Data

43bd697

Remove Manual Test Harness

df0fbc9

Extract Bidder Special Case Handling To Method

dd0c680

SyntaxNode commented Sep 4, 2020

View reviewed changes

Fixed Wrong Naming

715bfa1

SyntaxNode mentioned this pull request Sep 4, 2020

Major protocol change -- move bidders to imp[].ext.prebid.bidder #1478

Closed

Better Exchange Test File Names

079426e

SyntaxNode requested review from mansinahar and hhhjort September 8, 2020 14:25

SyntaxNode assigned mansinahar and hhhjort Sep 8, 2020

robertrmartinez mentioned this pull request Sep 8, 2020

fix GPT Pre-Auction PBS path prebid/Prebid.js#5650

Merged

1 task

hhhjort reviewed Sep 9, 2020

View reviewed changes

Renamed ExtRequestFirstPartyDataContext

a8c66ed

hhhjort previously approved these changes Sep 10, 2020

View reviewed changes

mansinahar reviewed Sep 10, 2020

View reviewed changes

Code Review Feedback

1bc35a4

SyntaxNode dismissed hhhjort’s stale review via 1bc35a4 September 10, 2020 16:23

mansinahar previously approved these changes Sep 10, 2020

View reviewed changes

SyntaxNode added 2 commits September 10, 2020 12:33

Replace Placeholder Description

15f4a9c

Added Validation Test Coverage

7e1a7e6

SyntaxNode dismissed mansinahar’s stale review via 7e1a7e6 September 10, 2020 16:39

hhhjort approved these changes Sep 10, 2020

View reviewed changes

mansinahar approved these changes Sep 10, 2020

View reviewed changes

SyntaxNode merged commit 42e6765 into prebid:master Sep 10, 2020

SyntaxNode deleted the fpd_allowcontext branch September 10, 2020 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass Through First Party Context Data #1479

Pass Through First Party Context Data #1479

SyntaxNode commented Sep 4, 2020

SyntaxNode Sep 4, 2020

SyntaxNode Sep 9, 2020

SyntaxNode Sep 4, 2020

SyntaxNode Sep 4, 2020

SyntaxNode Sep 4, 2020

SyntaxNode Sep 4, 2020

hhhjort Sep 9, 2020

SyntaxNode Sep 9, 2020

hhhjort left a comment

mansinahar Sep 10, 2020

SyntaxNode Sep 10, 2020

mansinahar Sep 10, 2020

SyntaxNode Sep 10, 2020

mansinahar Sep 10, 2020

SyntaxNode Sep 10, 2020

Pass Through First Party Context Data #1479

Pass Through First Party Context Data #1479

Conversation

SyntaxNode commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment