Make a smaller param for ard, and fix order of params #14019
Conversation
| export function getCalloutParam(callout) { | ||
| try { | ||
| const parsedUrl = parseUrl(callout); | ||
| callout = (parsedUrl.hostname + parsedUrl.pathname).substr(0, 50); |
There was a problem hiding this comment.
The 50 character limit should be doc'd
| try { | ||
| const parsedUrl = parseUrl(callout); | ||
| callout = (parsedUrl.hostname + parsedUrl.pathname).substr(0, 50); | ||
| } catch (e) {} |
There was a problem hiding this comment.
What sort of errors are we trying to catch here?
We should generally be doc'ing what sort of errors lead to us just returning the input unmodified.
| @@ -225,7 +246,8 @@ function sendRtcCallout_( | |||
| return {rtcTime, callout}; | |||
There was a problem hiding this comment.
should callout here be trimmed?
There was a problem hiding this comment.
Changed where we do the trimming so we only need to do it once.
| @@ -225,7 +246,8 @@ function sendRtcCallout_( | |||
| return {rtcTime, callout}; | |||
| } | |||
| const response = tryParseJson(text); | |||
| return response ? {response, rtcTime, callout} : | |||
| return response ? {response, rtcTime, | |||
| callout: getCalloutParam(callout)} : | |||
| buildErrorResponse_( | |||
| RTC_ERROR_ENUM.MALFORMED_JSON_RESPONSE, callout, rtcTime); | |||
There was a problem hiding this comment.
what about callout here?
There was a problem hiding this comment.
buildErrorResponse calls getCalloutParam from within it
| {response: rtcCalloutResponses[1], | ||
| callout: getCalloutParam(urls[1]), rtcTime: 10}, | ||
| {callout: getCalloutParam(urls[2]), | ||
| error: RTC_ERROR_ENUM.INSECURE_URL, rtcTime: 10}, | ||
| ]; | ||
| return executeTest({ | ||
| urls, inflatedUrls: urls, rtcCalloutResponses, calloutCount, |
There was a problem hiding this comment.
You also need explicit tests of getCalloutParam: I think these tests would all pass if it just always returned foo.
| function rtcEntry(response, url, error, isVendor) { | ||
| // If this is an entry for a vendor, then the callout is just | ||
| // the vendor name passed in to url here. | ||
| const callout = !!isVendor ? url : getCalloutParam_(url); |
There was a problem hiding this comment.
minor: is that !! actually doing anything?
There was a problem hiding this comment.
I suppose not, I just put it there for sanity sometimes as I'm going
| @@ -152,22 +171,22 @@ export function inflateAndSendRtc_(a4aElement, url, seenUrls, promiseArray, | |||
| if (Object.keys(seenUrls).length == MAX_RTC_CALLOUTS) { | |||
| return buildErrorResponse_( | |||
| RTC_ERROR_ENUM.MAX_CALLOUTS_EXCEEDED, | |||
| opt_vendor || url, undefined, true); | |||
| callout, undefined, true); | |||
There was a problem hiding this comment.
Let's always report errors... the only place where I don't see that we do is for malformed json and I can't see why we wouldn't. Just remove opt_log and then you can remove all of these undefined portions for opt_rtcTime
We had been passing the entire url that we call out to, now instead we shorten it. So instead of a doubleclick request including, for example:
...&ard=https://www.example.com/endpoint.php?a=1&b=2&c=3&d=4
it would now include
&ard=www.example.com/endpoint.php