-
Notifications
You must be signed in to change notification settings - Fork 748
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
SetUID Error Handling Refactor #3050
SetUID Error Handling Refactor #3050
Conversation
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.
Local testing with Prometheus metrics looks good. Code makes sense.
@@ -135,16 +119,14 @@ func NewSetUIDEndpoint(cfg *config.Configuration, syncersByBidder map[string]use | |||
tcf2Cfg := tcf2CfgBuilder(cfg.GDPR.TCF2, account.GDPR) | |||
|
|||
if shouldReturn, status, body := preventSyncsGDPR(gdprRequestInfo, gdprPermsBuilder, tcf2Cfg); shouldReturn { | |||
w.WriteHeader(status) | |||
w.Write([]byte(body)) |
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.
Why did we remove this line? In order for the refactored version to be completely equivalent, shouldn't we keep it?
121 if shouldReturn, status, body := preventSyncsGDPR(gdprRequestInfo, gdprPermsBuilder, tcf2Cfg); shouldReturn {
+ w.Write([]byte(body))
122 var metricValue metrics.SetUidStatus
123 switch status {
124 case http.StatusBadRequest:
125 metricValue = metrics.SetUidBadRequest
126 case http.StatusUnavailableForLegalReasons:
127 metricValue = metrics.SetUidGDPRHostCookieBlocked
128 }
129 handleBadStatus(w, status, metricValue, errors.New(body), metricsEngine, &so)
130 return
131 }
endpoints/setuid.go
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.
So after looking into it more, this body
value that preventSyncsGDPR
is essentially a custom error message. So I don't think there's much of a difference between w.Write([]byte(body))
and w.Write([]byte(errors.New(body))
which is how it's written in handleBadStatus
.
So I'd advocate for keeping the code how it is, and keeping it a bit cleaner. However, it is a very small difference, so I'm not totally against changing it either. What do you think?
I'm ok to approve it again. Gus, what do you think it should be? |
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.
LGTM
I noticed in the
/setuid
endpoint, that the error handling seemed repetitive and could be consolidated. So I wrote a function to simplify theNewSetUidEndpoint()
functions error handling.