-
Notifications
You must be signed in to change notification settings - Fork 774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove invalid user EIDs and UIDs from bid request #3891
base: master
Are you sure you want to change the base?
Changes from 7 commits
0f955e9
9352604
541cbed
22e6b21
db31c94
0f45f16
5514eae
74d9c26
5d56a61
5c45e53
4dcd6c9
675bd31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1277,23 +1277,69 @@ func (deps *endpointDeps) validateUser(req *openrtb_ext.RequestWrapper, aliases | |
} | ||
|
||
// Check Universal User ID | ||
for eidIndex, eid := range req.User.EIDs { | ||
if len(req.User.EIDs) > 0 { | ||
|
||
validEids, eidErrors := validateEIDs(req.User.EIDs) | ||
|
||
if len(eidErrors) > 0 { | ||
errL = append(errL, eidErrors...) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
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)) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, we can remove this error. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed |
||
req.User.EIDs = validEids | ||
} | ||
|
||
return errL | ||
} | ||
|
||
func validateEIDs(eids []openrtb2.EID) ([]openrtb2.EID, []error) { | ||
var errorsList []error | ||
validEIDs := make([]openrtb2.EID, 0, len(eids)) | ||
|
||
for eidIndex, eid := range eids { | ||
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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: can we change the error message a bit so that all of the messages in this function have similar wording? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
WarningCode: errortypes.InvalidUserEIDsWarningCode, | ||
}) | ||
continue | ||
} | ||
validUIDs, uidErrors := validateUIDs(eid.UIDs, eidIndex) | ||
errorsList = append(errorsList, uidErrors...) | ||
|
||
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)) | ||
if len(validUIDs) > 0 { | ||
eid.UIDs = validUIDs | ||
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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
WarningCode: errortypes.InvalidUserEIDsWarningCode, | ||
}) | ||
} | ||
} | ||
|
||
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)) | ||
} | ||
return validEIDs, errorsList | ||
} | ||
|
||
func validateUIDs(uids []openrtb2.UID, eidIndex int) ([]openrtb2.UID, []error) { | ||
var validUIDs []openrtb2.UID | ||
var uidErrors []error | ||
|
||
for uidIndex, uid := range uids { | ||
if uid.ID != "" { | ||
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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
WarningCode: errortypes.InvalidUserUIDsWarningCode, | ||
}) | ||
} | ||
} | ||
|
||
return errL | ||
return validUIDs, uidErrors | ||
} | ||
|
||
func validateRegs(req *openrtb_ext.RequestWrapper, gpp gpplib.GppContainer) []error { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick; Please remove the leading empty line.