Remove invalid user EIDs and UIDs from bid request #3891
Conversation
| @@ -35,6 +35,8 @@ const ( | |||
| InvalidBidResponseDSAWarningCode | |||
| SecCookieDeprecationLenWarningCode | |||
| SecBrowsingTopicsWarningCode | |||
| InvalidUserEIDsWarningCode | |||
| InvalidUserUIDsWarningCode | |||
There was a problem hiding this comment.
Do these new warning codes match with PBS-Java? We try to keep in sync.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -1,42 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
Instead of removing these tests cases, please modify them to prove the new behavior.
| @@ -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 { | |||
|
|
|||
There was a problem hiding this comment.
Nitpick; Please remove the leading empty line.
|
|
||
| if len(eidErrors) > 0 { | ||
| errL = append(errL, eidErrors...) | ||
| } |
There was a problem hiding this comment.
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?
endpoints/openrtb2/auction.go
Outdated
| validEIDs = append(validEIDs, eid) | ||
| } else { | ||
| errorsList = append(errorsList, &errortypes.Warning{ | ||
| Message: fmt.Sprintf("Removed EID with empty UIDs (source: %s)", eid.Source), |
There was a problem hiding this comment.
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
endpoints/openrtb2/auction_test.go
Outdated
| expectedErrorMessages []string | ||
| }{ | ||
| { | ||
| name: "Valid EID with non-empty UID", |
There was a problem hiding this comment.
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-validone-eid-one-uid-emptymany-mixedone-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.
endpoints/openrtb2/auction_test.go
Outdated
| t.Run(tc.name, func(t *testing.T) { | ||
| validEIDs, errorsList := validateEIDs(tc.input) | ||
|
|
||
| if len(validEIDs) != len(tc.expected) { |
There was a problem hiding this comment.
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)
| expectedValidUIDs: nil, | ||
| expectedErrors: nil, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Please add a test case for a nil input.
|
Hi @Pubmatic-Supriya-Patil, can you please merge with master (no rebase) and resolve conflicts? Thanks! |
|
Thank you for contributing this feature Please address the comments and we will re-review. |
…ver into Fix_3859
HI @bsardo I have updated branch and addressed review comments. |
This PR solves issue #3859