Request validation upgrade to 2.6 locations #3906

VeronikaSolovei9 · 2024-09-08T22:05:21Z

No description provided.

bsardo · 2024-09-09T17:47:58Z

endpoints/openrtb2/auction.go

@@ -823,7 +827,7 @@ func (deps *endpointDeps) validateRequest(account *config.Account, httpReq *http
 		}
 	}

-	if err := mapSChains(req); err != nil {
+	if err := mapSChains(req); err != nil { // do we need this?


You can get rid of this. It should now be handled in moveSupplyChainFrom24To25 from convert_up.go.

…Regs.GDPR

VeronikaSolovei9 · 2024-09-11T22:06:50Z

endpoints/openrtb2/auction.go

-func validateSChains(sChains []*openrtb_ext.ExtRequestPrebidSChain) error {
-	_, err := schain.BidderToPrebidSChains(sChains)
-	return err
-}


Not sure if this needs to be deleted

We need to keep this. I believe the source.ext.schain 2.5 field is what is being migrated to source.schain in 2.6. This logic refers to a different schain feature that has no 2.6 equivalent where bidder-specific schain nodes may be specified at source.ext.prebid.schains.

hhhjort

LGTM

bsardo · 2024-09-17T19:42:17Z

endpoints/openrtb2/auction.go

-func validateSChains(sChains []*openrtb_ext.ExtRequestPrebidSChain) error {
-	_, err := schain.BidderToPrebidSChains(sChains)
-	return err
-}


We need to keep this. I believe the source.ext.schain 2.5 field is what is being migrated to source.schain in 2.6. This logic refers to a different schain feature that has no 2.6 equivalent where bidder-specific schain nodes may be specified at source.ext.prebid.schains.

bsardo · 2024-09-17T20:43:27Z

endpoints/openrtb2/auction.go

-// mapSChains maps an schain defined in an ORTB 2.4 location (req.ext.schain) to the ORTB 2.5 location
-// (req.source.ext.schain) if no ORTB 2.5 schain (req.source.ext.schain, req.ext.prebid.schains) exists.


I'm wondering if we need to update the upconvert logic for schain to only perform an upgrade from 2.4 to 2.5 if req.ext.prebid.schains does not exist. req.ext.prebid.schains is a 2.5 location so it should take precedence.

Discussed offline. I'll take a look to see if this is a problem.

The order of precedence from highest to lowest is as follows:
req.ext.prebid.schains
req.source.schain (2.6)
req.source.ext.schain (2.5)
req.ext.schain (2.4)

We're upconverting right after we parse the request which will handle the ORTB standard locations resulting in a value in the 2.6 location if an schain exists in the 2.4, 2.5 or 2.6 location. Downstream we will use the schain writer to potentially overwrite the 2.6 location with a value from the req.ext.prebid.schains.

No additional change is needed to the upconvert logic.

bsardo · 2024-09-17T20:51:06Z

endpoints/openrtb2/auction.go

+	if len(sChain.Ver) == 0 {
+		return errors.New("request.source.schain.ver cannot be empty")
+	}
+	if len(sChain.Nodes) == 0 {
+		return errors.New("request.source.schain.nodes cannot be empty")
+	}
+	for i, node := range sChain.Nodes {
+		if len(node.ASI) == 0 {
+			return fmt.Errorf("request.source.schain.nodes[%d].asi cannot be empty", i)
+		}
+		if len(node.SID) == 0 {
+			return fmt.Errorf("request.source.schain.nodes[%d].sid cannot be empty", i)
+		}
+	}


Did these validation rules come from https://github.com/InteractiveAdvertisingBureau/openrtb/blob/main/supplychainobject.md? There are a couple of other fields marked as required in this spec.
I don't think we had these validation rules before so I'm wondering if there is a reason for that. I'll look through the closed issues to see if I can figure it out.

I found this in OpenRTB 2.6 spec, paragraph 3.2.25 and 3.2.26.

You are probably referring to this required field: Complete int8. This field bus not a pointer and will always present in this struct. Specification also says it can 0 = no, 1 = yes. Do we want to validate this value is 0 or 1 only? I missed this part.

Let's remove this validation. PBS's approach is to just validate the schema (attributes & datatypes) for fields we do not use.

bsardo · 2024-09-17T21:19:19Z

endpoints/openrtb2/auction.go

+		eidsValue := req.User.EIDs
 		for eidIndex, eid := range eidsValue {


Nitpick: I think you can iterate over req.User.EIDs directly and get rid of eidsValue since we are just reading the eids from the slice.

bsardo · 2024-09-17T21:23:42Z

endpoints/openrtb2/auction.go

+	if device.SUA != nil {
+		if len(device.SUA.Browsers) > 0 {
+			for i, browser := range device.SUA.Browsers {
+				if len(browser.Brand) == 0 {
+					return fmt.Errorf("request.device.sua.browsers[%d].brand cannot be empty", i)
+				}
+			}
+		}
+		if device.SUA.Platform != nil {
+			if len(device.SUA.Platform.Brand) == 0 {
+				return errors.New("request.device.sua.platform.brand cannot be empty")
+			}
+		}
+	}


Where did this validation logic come from? Is this new 2.6 behavior?

This is from the OpenRTB 2.6 specification, paragraph 3.2.30.
device.SUA.Browser[].Brand is required if device.SUA.Browser[] is present.
device.SUA.Platform.Brand is required if device.SUA.Platform is present.

We discussed offline that we can omit validation for the fields we don't use in PBS core, so I'm open to delete this.

Let's remove this validation. PBS's approach is to just validate the schema (attributes & datatypes) for fields we do not use.

bsardo · 2024-09-18T16:23:09Z

endpoints/openrtb2/sample-requests/invalid-whole/user-eids-source-empty.json

@@ -1,5 +1,5 @@
 {
-  "description": "Bid request where a request.user.ext.eids.uids array element is missing its id field",
+  "description": "Bid request with user.eids array element array element that does not contain source field",


typo: array element array element

bsardo · 2024-09-18T16:26:55Z

endpoints/openrtb2/auction_test.go

@@ -2889,183 +2889,6 @@ func TestValidateSourceTID(t *testing.T) {
 	assert.NotEmpty(t, req.Source.TID, "Expected req.Source.TID to be filled with a randomly generated UID")
 }

-func TestSChainInvalid(t *testing.T) {


We should restore the prebid specific per-bidder schain logic as mentioned in another comment so this test should be restored as well.

Initial request validation upgrade to 2.6 locations

c4fcd13

bsardo reviewed Sep 9, 2024

View reviewed changes

VeronikaSolovei9 added 2 commits September 9, 2024 21:33

Initial request validation upgrade to 2.6 locations - req.BidRequest.…

10e1faf

…Regs.GDPR

Clean up

a8e5a44

bsardo added the do not port label Sep 11, 2024

Clean up and unit tests

590e56b

VeronikaSolovei9 marked this pull request as ready for review September 11, 2024 22:06

VeronikaSolovei9 commented Sep 11, 2024

View reviewed changes

VeronikaSolovei9 changed the title ~~[DRAFT] Request validation upgrade to 2.6 locations~~ Request validation upgrade to 2.6 locations Sep 11, 2024

bsardo assigned bsardo and hhhjort Sep 12, 2024

hhhjort approved these changes Sep 16, 2024

View reviewed changes

bsardo reviewed Sep 18, 2024

View reviewed changes

VeronikaSolovei9 added 2 commits September 18, 2024 18:27

Code review fixes

3a3c492

Redundant validation revert

04e460f

bsardo approved these changes Sep 25, 2024

View reviewed changes

bsardo merged commit 664f2ff into ortb26 Sep 25, 2024

bsardo pushed a commit to bsardo/prebid-server that referenced this pull request Oct 10, 2024

Request validation upgrade to 2.6 locations (prebid#3906)

8a10a7c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request validation upgrade to 2.6 locations #3906

Request validation upgrade to 2.6 locations #3906

VeronikaSolovei9 commented Sep 8, 2024

bsardo Sep 9, 2024

VeronikaSolovei9 Sep 11, 2024

bsardo Sep 17, 2024

VeronikaSolovei9 Sep 19, 2024

hhhjort left a comment

bsardo Sep 17, 2024

bsardo Sep 17, 2024

bsardo Sep 18, 2024

bsardo Sep 23, 2024

bsardo Sep 17, 2024

VeronikaSolovei9 Sep 19, 2024

bsardo Sep 24, 2024

VeronikaSolovei9 Sep 24, 2024

bsardo Sep 17, 2024

bsardo Sep 17, 2024

VeronikaSolovei9 Sep 19, 2024 •

edited

Loading

bsardo Sep 24, 2024

VeronikaSolovei9 Sep 24, 2024

bsardo Sep 18, 2024

bsardo Sep 18, 2024

VeronikaSolovei9 Sep 19, 2024

		// mapSChains maps an schain defined in an ORTB 2.4 location (req.ext.schain) to the ORTB 2.5 location
		// (req.source.ext.schain) if no ORTB 2.5 schain (req.source.ext.schain, req.ext.prebid.schains) exists.

		eidsValue := req.User.EIDs
		for eidIndex, eid := range eidsValue {

Request validation upgrade to 2.6 locations #3906

Request validation upgrade to 2.6 locations #3906

Conversation

VeronikaSolovei9 commented Sep 8, 2024

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

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

VeronikaSolovei9 Sep 19, 2024 • edited Loading

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

VeronikaSolovei9 Sep 19, 2024 •

edited

Loading