[TEX-537] Refactor HTTP request and response representations #554
Conversation
timdawborn
changed the title
| request: { | ||
| method: string; | ||
| path: string; | ||
| headers: Headers; | ||
| body: Buffer; | ||
| }; | ||
| response: { | ||
| status: { | ||
| code: number; | ||
| }; | ||
| headers: Headers; | ||
| body: Buffer; | ||
| }; |
There was a problem hiding this comment.
Moved to HttpRequest and HttpReponse in http.ts.
|
|
||
| /** | ||
| * Headers of a request or response. | ||
| */ | ||
| export interface Headers { | ||
| [headerName: string]: string | string[] | undefined; | ||
| } |
There was a problem hiding this comment.
Moved into http.ts and renamed to HttpHeaders.
| export interface Request { | ||
| host?: string; | ||
| method: string; | ||
| path: string; | ||
| headers: Headers; | ||
| body: Buffer; | ||
| } | ||
|
|
||
| export interface RequestWithHost extends Request { | ||
| host: string; | ||
| } |
There was a problem hiding this comment.
Moved into HttpRequest in http.ts.
| requestMethod: string, | ||
| requestPath: string, | ||
| requestHeaders: Headers, | ||
| requestBody: Buffer, |
There was a problem hiding this comment.
Just pass around a HttpRequest object instead of all of the individual components of a request unencapsulated.
| "deep-diff": "^1.0.2", | ||
| "fs-extra": "^11.1.1", | ||
| "js-yaml": "^4.1.0", | ||
| "query-string": "^6.13.6", |
There was a problem hiding this comment.
Removed this super outdated library and replaced it with the builtin querystring library provided by NodeJS. https://nodejs.org/api/querystring.html
| // Compare the query parameters. | ||
| const parsedQueryParameters = parseQueryParameters(request.path); | ||
| const parsedCompareToQueryParameters = parseQueryParameters( | ||
| compareTo.request.path, | ||
| ); | ||
| const differencesQueryParameters = countObjectDifferences( | ||
| parsedQueryParameters, | ||
| parsedCompareToQueryParameters, | ||
| rewriteBeforeDiffRules, | ||
| ); | ||
|
|
||
| // Compare the cleaned headers. | ||
| const cleanedHeaders = stripExtraneousHeaders(request.headers); | ||
| const cleanedCompareToHeaders = stripExtraneousHeaders( | ||
| compareTo.request.headers, | ||
| ); | ||
| return ( | ||
| countObjectDifferences( | ||
| parsedQuery, | ||
| parsedCompareToQuery, | ||
| rewriteBeforeDiffRules, | ||
| ) + | ||
| countObjectDifferences(headers, compareToHeaders, rewriteBeforeDiffRules) + | ||
| countBodyDifferences( | ||
| serialisedRequestBody, | ||
| serialisedCompareToRequestBody, | ||
| rewriteBeforeDiffRules, | ||
| ) | ||
| const differencesHeaders = countObjectDifferences( | ||
| cleanedHeaders, | ||
| cleanedCompareToHeaders, | ||
| rewriteBeforeDiffRules, | ||
| ); | ||
|
|
||
| // Compare the bodies. | ||
| const differencesBody = countBodyDifferences( | ||
| request, | ||
| compareTo.request, | ||
| rewriteBeforeDiffRules, | ||
| ); | ||
|
|
||
| return differencesQueryParameters + differencesHeaders + differencesBody; |
There was a problem hiding this comment.
This is the same logic as before, it's just been re-ordered slightly.
| const contentType = contentType1.type; | ||
| if (contentType === "application/json") { | ||
| return countBodyDifferencesApplicationJson( | ||
| request1, | ||
| contentType1, | ||
| request2, | ||
| contentType2, | ||
| rewriteBeforeDiffRules, | ||
| ); | ||
| } else if (contentType.startsWith("text/")) { | ||
| return countBodyDifferencesText( | ||
| request1, | ||
| contentType1, | ||
| request2, | ||
| contentType2, | ||
| rewriteBeforeDiffRules, | ||
| ); | ||
| } else { | ||
| // No more special cases to consider. Assume binary data for all other content types. | ||
| return countBodyDifferencesBinary(request1, request2); | ||
| } |
There was a problem hiding this comment.
This is some slightly different logic to before.
Up until now, we just blindly attempted to decode it as JSON. If that succeeded, we did a JS object comparison of the resulting JSON decoded values. If it failed to decode, we did a base64 string comparison.
Now, we inspect the content type of the HTTP requests, and attempt to do JSON decoding if they have application/json as their content type, do raw string comparison if they have text/<anything> as their content type, and base64 comparison for any other content type.
| const questionMarkPosition = path.indexOf("?"); | ||
| if (questionMarkPosition !== -1) { | ||
| return path.substr(0, questionMarkPosition); | ||
| return path.substring(0, questionMarkPosition); |
There was a problem hiding this comment.
This change fixes a deprecation warning. substr has been long deprecated in favour of substring.
| const questionMarkPosition = path.indexOf("?"); | ||
| if (questionMarkPosition !== -1) { | ||
| return queryString.parse(path.substr(questionMarkPosition)); | ||
| return parseQueryString(path.substring(questionMarkPosition + 1)); |
There was a problem hiding this comment.
The + 1 addition here fixes a long standing bug that I found while swapping the query string parsing functionality from the long deprecated library to the builtin NodeJS library. Before, the substring returned included in the ? at the start of the string as the slice was starting at the index of the ?. Apparently the old library accepted this without issue, but the new NodeJS one did not.
| @@ -54,13 +54,14 @@ | |||
| "brotli": "^1.3.3", | |||
| "chalk": "^4.1.0", | |||
| "commander": "^10.0.1", | |||
| "content-type": "^1.0.5", | |||
There was a problem hiding this comment.
Added a HTTP content-type parsing library. This is used in similarity.ts to start adding some smarts in how to compare HTTP request bodies of different content types.
| /** | ||
| * Headers of a request or response. | ||
| */ | ||
| export interface HttpHeaders { | ||
| [headerName: string]: string | string[] | undefined; | ||
| } |
There was a problem hiding this comment.
This has been moved from tape.ts.
| /** | ||
| * The common fields of a HTTP request. | ||
| */ | ||
| export interface HttpRequest { | ||
| host?: string; | ||
| method: string; | ||
| path: string; | ||
| headers: HttpHeaders; | ||
| body: Buffer; | ||
| } | ||
|
|
||
| export interface HttpRequestWithHost extends HttpRequest { | ||
| host: string; | ||
| } |
There was a problem hiding this comment.
These have been moved from sender.ts.
| /** | ||
| * The common fields of a HTTP response. | ||
| */ | ||
| export interface HttpResponse { | ||
| status: { | ||
| code: number; | ||
| }; | ||
| headers: HttpHeaders; | ||
| body: Buffer; | ||
| } |
There was a problem hiding this comment.
This has been moved from tape.ts.
This PR does a bunch of initial refactoring and cleaning up of the representations of HTTP request and response objects throughout the codebase, ahead of making some larger changes to support gRPC-web request comparisons.
The changes in this PR are mostly code structure in nature rather than functional. There is one functionality change in the decision on how to compare two HTTP request bodies, which I've called out in comments.