Remove invalid user EIDs and UIDs from bid request #3891

Pubmatic-Supriya-Patil · 2024-08-28T06:08:01Z

This PR solves issue #3859

Pubmatic-Supriya-Patil · 2024-09-04T03:56:48Z

Hi @bsardo @bretg
Please review this PR and let me know if any changes are required.

SyntaxNode · 2024-11-01T03:26:26Z

errortypes/code.go

@@ -35,6 +35,8 @@ const (
 	InvalidBidResponseDSAWarningCode
 	SecCookieDeprecationLenWarningCode
 	SecBrowsingTopicsWarningCode
+	InvalidUserEIDsWarningCode
+	InvalidUserUIDsWarningCode


Do these new warning codes match with PBS-Java? We try to keep in sync.

Can you please help me to get those error codes. Do we have any document or PR to check new warning codes match with PBS-Java.

As per prebid-server-java/pull/3465, I understand that the same 999 warning code is being used when removing eids arrays. In the Prebid Server Go repository, 999 corresponds to UnknownErrorCode in the error codes reference. Are we intending to use UnknownErrorCode here as well?

In this PR, I introduced InvalidUserEIDsWarningCode (10013) and InvalidUserUIDsWarningCode (10014). If we are okay with a generic error code, we could use UnknownErrorCode (999). However, please confirm whether we want separate error codes for InvalidUserEIDs and InvalidUserUIDs, or if using 999 would suffice.

Kindly provide your input on this.

As discussed offline, it is ok to have these error codes. No change needed.

SyntaxNode · 2024-11-01T03:27:01Z

endpoints/openrtb2/sample-requests/invalid-whole/user-ext-eids-uids-id-empty.json

@@ -1,42 +0,0 @@
-{


Instead of removing these tests cases, please modify them to prove the new behavior.

SyntaxNode · 2024-11-01T03:28:31Z

endpoints/openrtb2/auction.go

@@ -1296,8 +1296,14 @@ func (deps *endpointDeps) validateUser(req *openrtb_ext.RequestWrapper, aliases
 	// Check Universal User ID
 	eids := userExt.GetEid()
 	if eids != nil {
-		eidsValue := *eids
-		for eidIndex, eid := range eidsValue {
+


Nitpick; Please remove the leading empty line.

SyntaxNode · 2024-11-01T03:30:17Z

endpoints/openrtb2/auction.go

+
+		if len(eidErrors) > 0 {
+			errL = append(errL, eidErrors...)
+		}


Likely fine to keep this logic here for now. We want to separate validation from fixing in the future. We can refactor this new logic along with the rest later. Thoughts @bsardo?

SyntaxNode · 2024-11-01T03:32:18Z

endpoints/openrtb2/auction.go

+			validEIDs = append(validEIDs, eid)
+		} else {
+			errorsList = append(errorsList, &errortypes.Warning{
+				Message:     fmt.Sprintf("Removed EID with empty UIDs (source: %s)", eid.Source),


Please keep with the established error message format. Perhaps something like:

request.user.ext.eids[0] (source: %s) contains only empty uids and is removed from the request

SyntaxNode · 2024-11-01T03:37:56Z

endpoints/openrtb2/auction_test.go

+		expectedErrorMessages []string
+	}{
+		{
+			name: "Valid EID with non-empty UID",


Go test names cannot contain spaces so they are replaced with a dash character. This makes it hard to locate the failing test based on its name. Recommend using simpler test names like:

one-eid-one-uid-valid

one-eid-one-uid-empty

many-mixed

one-eid-many-uid-empty

The parent test name is within scope so it's easy to locate in source. Please apply this guidance to other tests added in this PR.

SyntaxNode · 2024-11-01T03:41:27Z

endpoints/openrtb2/auction_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			validEIDs, errorsList := validateEIDs(tc.input)
+
+			if len(validEIDs) != len(tc.expected) {


Please use testify assert statements where possible. This can be written as follows which will compare the contents instead of just the array lengths, yielding more concrete tests.

assert.ElementsMatch(e, tc.expected, validEIDs)

SyntaxNode · 2024-11-01T03:42:36Z

endpoints/openrtb2/auction_test.go

+			expectedValidUIDs: nil,
+			expectedErrors:    nil,
+		},
+	}


Please add a test case for a nil input.

bsardo · 2024-11-04T16:40:16Z

Hi @Pubmatic-Supriya-Patil, can you please merge with master (no rebase) and resolve conflicts? Thanks!

SyntaxNode · 2024-11-05T16:30:23Z

Thank you for contributing this feature Please address the comments and we will re-review.

…ver into Fix_3859

Pubmatic-Supriya-Patil · 2024-12-02T09:22:23Z

Hi @Pubmatic-Supriya-Patil, can you please merge with master (no rebase) and resolve conflicts? Thanks!

HI @bsardo I have updated branch and addressed review comments.

endpoints/openrtb2/auction.go

VeronikaSolovei9 · 2024-12-10T04:32:25Z

endpoints/openrtb2/auction.go

+			for uidIndex, uid := range eid.UIDs {
+				if uid.ID == "" {
+					return append(errL, fmt.Errorf("request.user.eids[%d].uids[%d] missing required field: \"id\"", eidIndex, uidIndex))
+				}


At this time we will not have empty eid.ID, correct? there is a check for this already in line 1334.

Also do we need to set "clean" EIDs back to req.User.EIDs?
Something like req.User.EIDs = validEids after the for loop?

Please correct me if I'm wrong. We don't want to fail the entire request if there is at least one invalid EID, correct? Or do we want to drop invalid ones and continue the execution?
If we want to continue the execution - please add this fix and json tests where we validate "clean" user.EIDs and error messages.

Can we remove if len(eid.UIDs) == 0 { validation also from validateUser function. This is also not possible because validateEids wil return error for emtpy uids. Please provide your suggestions @VeronikaSolovei9

No no, I didn't ask to delete if len(eid.UIDs) == 0 { - it is needed here. I only asked about if we want to add valid aids to the request. And you fixed this by adding this line: req.User.EIDs = validEids

Okay. Thanks. Please review PR

…ver into Fix_3859

ShriprasadM · 2025-01-13T15:24:31Z

@Pubmatic-Supriya-Patil : Please respond to this comments.

bsardo · 2025-01-15T18:37:03Z

@Pubmatic-Supriya-Patil can you please merge with master? That should resolve the failing validate-merge check.

…ver into Fix_3859

VeronikaSolovei9 · 2025-01-17T08:59:35Z

endpoints/openrtb2/auction.go

+		for eidIndex, eid := range validEids {
+			if eid.Source == "" {
+				return append(errL, fmt.Errorf("request.user.eids[%d] missing required field: \"source\"", eidIndex))
+			}

-		for uidIndex, uid := range eid.UIDs {
-			if uid.ID == "" {
-				return append(errL, fmt.Errorf("request.user.eids[%d].uids[%d] missing required field: \"id\"", eidIndex, uidIndex))
+			if len(eid.UIDs) == 0 {
+				return append(errL, fmt.Errorf("request.user.eids[%d].uids must contain at least one element or be undefined", eidIndex))


This code fails the request if eids look like this:

"eids": [ { "source": "uidapi.com", "uids": [ { "id": "" }, { "id": "id111111" } ] }, { "source": "liveramp.com", "uids": [ { "id": "id22222" } ] }, { "source": "", <<< this is the problem "uids": [ { "id": "123" }, { "id": "1234" } ] } ]

All eids are added to the valid list, because they have at least one valid id. The last eid doesn't have a source and instead of removing this eid from the list of valid aids - we return fatal error and fail the request with unexpected message:

Invalid request: request.user.eids[0].uids[0] contains empty ids and is removed from the request Invalid request: request.user.eids[2] missing required field: "source"

Possible fix: move these 2 checks to the validateEIDs function after validateUIDs.
Add message to the error list and don't add this aid to the list of valid eids.

As per my understanding, the logic for removing invalid IDs is specific to eids. We have retained the existing logic for checking the source. However, it is unclear whether we need to check for an empty source and remove that entry while allowing other valid eids. @VeronikaSolovei9, could you please confirm this scenario and clarify the expected behavior in this case?

I will ask team about the expected behavior.

My take is that EID.source is required. IMO, makes sense to remove empty sources.

The team discussed offline. We're in agreement that if EID.source is invalid we should simply remove the corresponding EID without failing the request. We should also add a warning message.

Thanks for the update. I’ll incorporate the required changes.

…ver into Fix_3859

…stead of errors

VeronikaSolovei9

Thank you for the changes, approved!

bsardo · 2025-02-21T20:13:53Z

endpoints/openrtb2/auction.go

+		for eidIndex, eid := range validEids {
+			if len(eid.UIDs) == 0 {
+				return append(errL, fmt.Errorf("request.user.eids[%d].uids must contain at least one element or be undefined", eidIndex))
+			}
+		}


As discussed offline, we can remove this error.

bsardo · 2025-02-25T18:05:36Z

errortypes/code.go

@@ -35,6 +35,8 @@ const (
 	InvalidBidResponseDSAWarningCode
 	SecCookieDeprecationLenWarningCode
 	SecBrowsingTopicsWarningCode
+	InvalidUserEIDsWarningCode
+	InvalidUserUIDsWarningCode


As discussed offline, it is ok to have these error codes. No change needed.

bsardo · 2025-02-27T17:28:11Z

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

@@ -35,6 +35,6 @@
      }]
    }
  },
-  "expectedReturnCode": 400,
-  "expectedErrorMessage": "Invalid request: request.user.eids[0].uids[0] missing required field: \"id\"\n"
+  "expectedReturnCode": 200,


Can we edit this test on lines 31-336 so that there are no uid objects:

"user": { "eids": [{ "source": "source1", "uids": [] }] }

bsardo · 2025-02-27T17:33:01Z

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

Can we add a new JSON test user-eids-empty-uids-removed.json that is similar to the example in the corresponding issue?
[{"source":"a","uids":[{"id":""}, {"id":"id-a"}]}, {"source":"b","uids":[{"id":"id-b"}]}, {"source":"c","uids":[{"id":""},{"id":""}]}]

bsardo · 2025-02-27T17:45:14Z

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

The JSON tests here now return 200s so they should be moved out of the invalid-whole directory to some other valid request directory.

Moved to valid-whole

bsardo · 2025-02-27T17:47:34Z

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

-  "expectedReturnCode": 400,
-  "expectedErrorMessage": "Invalid request: request.user.eids[0] missing required field: \"source\"\n"
+  "expectedReturnCode": 200,
+  "expectedErrorMessage": "request.user.eids[0] missing required field: source"


Since all of these JSON test errors are now reported as warnings and since the endpoint JSON test framework does not allow both expectedBidResponse and expectedErrorMessage in a given JSON test, it would be better to use the following approach:

Add expectedBidResponse object

Specify the expected warnings in the expectedBidResponse object under ext.warnings

bsardo · 2025-02-27T18:01:13Z

endpoints/openrtb2/auction_test.go

+		numErrors             int
+		expectedErrorMessages []string
+	}{
+		{


Please add the following test cases:

eid-nil

eid-uid-nil

source-missing

Added test cases for given scenarios

bsardo · 2025-02-28T21:34:06Z

endpoints/openrtb2/auction_test.go

+			assert.ElementsMatch(t, tc.expectedValidUIDs, validUIDs, "Valid UIDs mismatch")
+			assert.ElementsMatch(t, tc.expectedErrors, errorsList, "Errors mismatch")


Nitpick: you can delete the last parameter in these assert statements. t.Run will give the appropriate context should one of these assertions fail.

Removed last parameter

bsardo · 2025-02-28T21:35:31Z

endpoints/openrtb2/auction.go

 		if eid.Source == "" {
-			return append(errL, fmt.Errorf("request.user.eids[%d] missing required field: \"source\"", eidIndex))
+			errorsList = append(errorsList, &errortypes.Warning{
+				Message:     fmt.Sprintf("request.user.eids[%d] missing required field: source", eidIndex),


Nitpick: can we change the error message a bit so that all of the messages in this function have similar wording?
request.user.eids[%d] removed due to missing source

bsardo · 2025-02-28T21:36:05Z

endpoints/openrtb2/auction.go

+			validEIDs = append(validEIDs, eid)
+		} else {
+			errorsList = append(errorsList, &errortypes.Warning{
+				Message:     fmt.Sprintf("request.user.eids[%d] (source: %s) contains only empty uids and is removed from the request", eidIndex, eid.Source),


Nitpick: can we change the error message a bit so that all of the messages in this function have similar wording and so it is a bit more concise?
request.user.eids[%d] (%s) removed due to empty uids

bsardo · 2025-02-28T21:36:37Z

endpoints/openrtb2/auction.go

+			validUIDs = append(validUIDs, uid)
+		} else {
+			uidErrors = append(uidErrors, &errortypes.Warning{
+				Message:     fmt.Sprintf("request.user.eids[%d].uids[%d] contains empty ids and is removed from the request", eidIndex, uidIndex),


Nitpick: can we change the error message a bit so that all of the messages in this function have similar wording and so it is a bit more concise?
request.user.eids[%d].uids[%d] removed due to empty ids

bsardo · 2025-02-28T21:56:14Z

endpoints/openrtb2/auction_test.go

+		expectedEids []openrtb2.EID
+	}{
+		{
+			description: "Valid user with Geo accuracy",


Nitpick: use name instead of description and replace all test case name spaces with underscores

…est cases for missing source field

…erver into Fix_3859

Remove invalid user EIDs and UIDs from bid request

0f955e9

Pubmatic-Supriya-Patil mentioned this pull request Aug 28, 2024

Drop the User EID object if the all the ID values are empty #3859

Open

bsardo self-assigned this Oct 21, 2024

SyntaxNode reviewed Nov 1, 2024

View reviewed changes

Merge branch 'master' of github.com:Pubmatic-Supriya-Patil/prebid-ser…

9352604

…ver into Fix_3859

bsardo assigned VeronikaSolovei9 Dec 4, 2024

VeronikaSolovei9 reviewed Dec 10, 2024

View reviewed changes

endpoints/openrtb2/auction.go Outdated Show resolved Hide resolved

VeronikaSolovei9 reviewed Dec 10, 2024

View reviewed changes

Merge branch 'master' of github.com:Pubmatic-Supriya-Patil/prebid-ser…

541cbed

…ver into Fix_3859

Removed redundant check for uid.id validation

22e6b21

Merge branch 'master' of github.com:Pubmatic-Supriya-Patil/prebid-ser…

db31c94

…ver into Fix_3859

Pubmatic-Supriya-Patil requested review from VeronikaSolovei9 and SyntaxNode January 16, 2025 04:27

VeronikaSolovei9 reviewed Jan 17, 2025

View reviewed changes

Pubmatic-Supriya-Patil added 2 commits February 4, 2025 17:14

Merge branch 'master' of github.com:Pubmatic-Supriya-Patil/prebid-ser…

0f45f16

…ver into Fix_3859

Refactor EID validation to issue warnings for missing source field in…

5514eae

…stead of errors

Pubmatic-Supriya-Patil requested review from bsardo and VeronikaSolovei9 February 4, 2025 15:28

VeronikaSolovei9 previously approved these changes Feb 12, 2025

View reviewed changes

bsardo reviewed Feb 27, 2025

View reviewed changes

Remove validation for empty UIDs in user EIDs

74d9c26

Pubmatic-Supriya-Patil dismissed VeronikaSolovei9’s stale review via 74d9c26 February 28, 2025 08:50

Merge branch 'prebid:master' into Fix_3859

5d56a61

Pubmatic-Supriya-Patil requested a review from bsardo February 28, 2025 11:56

bsardo reviewed Feb 28, 2025

View reviewed changes

Pubmatic-Supriya-Patil and others added 3 commits March 4, 2025 09:19

Merge branch 'prebid:master' into Fix_3859

5c45e53

Update EID validation to remove entries with empty UIDs and add new t…

4dcd6c9

…est cases for missing source field

Merge branch 'Fix_3859' of github.com:Pubmatic-Supriya-Patil/prebid-s…

675bd31

…erver into Fix_3859

Pubmatic-Supriya-Patil requested a review from bsardo March 7, 2025 09:45

		assert.ElementsMatch(t, tc.expectedValidUIDs, validUIDs, "Valid UIDs mismatch")
		assert.ElementsMatch(t, tc.expectedErrors, errorsList, "Errors mismatch")

Remove invalid user EIDs and UIDs from bid request #3891

Are you sure you want to change the base?

Remove invalid user EIDs and UIDs from bid request #3891

Conversation

Pubmatic-Supriya-Patil commented Aug 28, 2024

Pubmatic-Supriya-Patil commented Sep 4, 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

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

bsardo commented Nov 4, 2024

SyntaxNode commented Nov 5, 2024

Pubmatic-Supriya-Patil commented Dec 2, 2024

VeronikaSolovei9 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

VeronikaSolovei9 Dec 10, 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

ShriprasadM commented Jan 13, 2025

bsardo commented Jan 15, 2025

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 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Dec 10, 2024 •

edited

Loading

VeronikaSolovei9 Dec 10, 2024 •

edited

Loading