Skip to content
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

feat: improve fake api gateway request events creation #91

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

MatheusBaldi
Copy link
Contributor

No description provided.

Copy link
Contributor

@onebytegone onebytegone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatheusBaldi Thanks for submitting this! On first pass, a few observations:

  1. This is a fairly large MR that is fundamentally changing how the test events are generated in this project. Instead of changing everything, would it be possible to just change the API Gateway test event in this initial PR and then submit a follow up PR for the ALB events? This way the initial pattern can be set while keeping the set of changes small.
  2. By convention we lean towards the OR operator (||) over the null coalescing operator (??). Eventually we want to reconsider that stance, but for now can we keep with || to maintain consistency.
  3. This PR is adding a bunch of types which basically mirror the AWS provided types. The goal of these "generator" functions is to make developer's lives better. Rather than taking a 1-to-1 input from the event, can we abstract the internal workings of the events some? For example:
// in tests/samples.ts...

interface MakeAPIGatewayRequestParams {
   // Only defining the overrides that are needed
   httpMethod?: string;
   headers?: StringMap;
}

function makeAPIGatewayRequest(params: MakeAPIGatewayRequestParams): APIGatewayRequestEvent {
   return {
      // ... omitted for brevity ...
      httpMethod: params.httpMethod || 'GET',
      requestContext: makeAPIGatewayRequestContext({
         httpMethod: params.httpMethod,
      }),
      headers: {
         // ...
         ...(params.headers || {})
      },
      multiValueHeaders: {
         // ...
         ...Object.fromEntries(Object.entries(params.headers || {}).map(([ key, value ]), () => {
            return [ key, [ value ] ];
         }))
      },
      // ...
   };
}

interface MakeAPIGatewayRequestContextParams {
   httpMethod?: string;
}

function makeAPIGatewayRequestContext(params: MakeAPIGatewayRequestContextParams): APIGatewayEventRequestContext {
   return {
      // ...
      httpMethod: params.httpMethod || 'GET',
      // ...
   };
};

@MatheusBaldi
Copy link
Contributor Author

MatheusBaldi commented Dec 6, 2024

@onebytegone Thank you for your observations.

  1. Sounds good to me, I'll do that. Would you recommend closing the PR and referencing it on the others or just make this the first one?
  2. Hmmmm, ok. It'll be necessary to add a verification for undefined though, something like someAttribute: params.someAttribute === undefined ? 'default' : params.someAttribute,. Otherwise we could end up falling in the default case when we actually intend to use a falsy value. This may even be better since allows us to input null if we want. I see you also removed the optional chaining operator, is it avoided as well?
  3. I understand the motivation, but since it will probably be a standard, I believe it's worth discussing this a bit further. Here are some things I've considered:
    1. By using Partial<...> as the type definition for the params input, all fields become optional, so the developer will only be demanded to follow the type definition in case he wants to specify a value for an attribute, and only for that attribute
    2. By having a local type that mimics the original from AWS, if there is a change in their type definition, we will get a type error while testing, which is good. This type check would happen when calling the system under test. But it even seems to me that defining a local type is not really necessary, since we can use partial and we'll get type check anyways, so maybe we could just use the AWS types;
    3. I consider specific implementation inside a generator function a bit dangerous depending on how it's done. The expected behavior I imagine for a generator function is that the developer will receive a dummy object by calling it, and, if he specifies a value for an attribute through the input param, that value will be exactly what the developer passed. If we have implementation changing the input inside the function, the developer will not know what is happening, he will be demanded to check the inner workings of the function. And it's even possible that the way the function behaves doesn't fulfill the need of what this developer wants to mock. In this situation this developer has some options: change the generator function (may impact all other tests that uses it, it hurts when it happens); create a new generator function (bad tendency to the codebase); define the object in the specific test (could be a solution, but loses DRY and the benefit of having a common base object for all tests). All this can be avoided if the generator function doesn't change its input before returning it. For example:
function makeAPIGatewayRequest(params?: MakeAPIGatewayRequestParams): APIGatewayRequestEvent {
   const defaultHttpMethod = params.httpMethod || 'GET';
   const defaultHeader = makeAPIGatewayRequestHeader(params.headers);
   return {
      // ... omitted for brevity ...
      httpMethod: defaultHttpMethod,

      // if requestContext not specified, define a default as we want, otherwise, use the input
      requestContext: params?.requestContext === undefined
         ? makeAPIGatewayRequestContext({ // this function will know what other fields to populate by default
            httpMethod: defaultHttpMethod,
         })
         : params.requestContext,

      headers: params?.headers === undefined
         ? defaultHeader
         : params.headers,
      
      multiValueHeaders: params?.multiValueHeaders === undefined
         ? makeAPIGatewayRequestMultivalueHeader(defaultHeader) // 
         : params.multiValueHeaders,
      // ...
   };
}

Also, we could have other helper functions that builds parts of the object before passing them to the generator, in that way, the responsibility of being specific goes to where the generator function is called.

Now two more questions

  • I see some places where the object we are mocking is called "Event", and other places it's called "Request". But aren't they only events, since they represent the event object received by the lambda and have not been through new Request() yet?
  • Is it a problem using the input function as optional in this case? It would prevent the developer from having to call the function with an empty object makeFunction({}) when he doesn't need changing any attibute.

@onebytegone
Copy link
Contributor

1 Feel free to update this PR
2/3. Could we have a helper like this?

interface MakeAPIGatewayRequestEventParams {
   httpMethod?: string | null;
}

function makeAPIGatewayRequestEvent(params?: MakeAPIGatewayRequestEventParams): APIGatewayRequestEvent {
  const httpMethod = withDefault(params?.httpMethod, 'GET');

   return {
      // ... omitted for brevity ...
      httpMethod,
      requestContext: makeAPIGatewayRequestContext({
         httpMethod,
      }),
   };
}

function withDefault<T>(value: T, defaultValue: T): T | undefined {
   if (isUndefined(value)) {
       return defaultValue;
   }

   if (value === null) {
      return undefined;
   }

   return value;
}
  1. Let's go with *RequestEvent, that's more accurate. Good catch.
  2. Yeah, the params should be optional too.

@MatheusBaldi MatheusBaldi force-pushed the MatheusBaldi/improve-fake-events-creation branch 2 times, most recently from 23bd78e to 06ef2c0 Compare December 19, 2024 18:28
@coveralls
Copy link

coveralls commented Dec 19, 2024

Coverage Status

coverage: 98.663%. remained the same
when pulling 623ffe8 on MatheusBaldi:MatheusBaldi/improve-fake-events-creation
into cc79c46 on silvermine:master.

tests/samples.ts Outdated
Comment on lines 222 to 256
export const makeAPIGatewayRequestEvent = (params?: MakeAPIGatewayRequestEventParams): APIGatewayRequestEvent => {
const httpMethod = withDefault(params?.httpMethod, 'GET'),
path = withDefault(params?.path, '/echo/asdf/a'),
headers = withDefault(params?.headers, makeAPIGatewayRequestEventHeaders());

const multiValueHeaders = withDefault(
params?.multiValueHeaders,
makeAPIGatewayRequestEventMultiValueHeader()
);

return {
path,
httpMethod,
body: null,
isBase64Encoded: false,
resource: '/{proxy+}',
pathParameters: { proxy: path },
stageVariables: null,
requestContext: makeAPIGatewayRequestContext({
httpMethod,
}),
headers,
multiValueHeaders,
queryStringParameters: {
'foo[a]': 'bar b',
x: '2',
y: 'z',
},
multiValueQueryStringParameters: {
'foo[a]': [ 'bar b', 'baz c' ],
x: [ '1', '2' ],
y: [ 'z' ],
},
};
};
Copy link
Contributor Author

@MatheusBaldi MatheusBaldi Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also thought of another approach for handling default values by using the object behavior of making keys that comes at last overwrite the preceding ones:

// say params is:
const params = {
   path: '/another/path/a',
   httpMethod: 'POST',
};
export const makeAPIGatewayRequestEvent = (params?: MakeAPIGatewayRequestEventParams): APIGatewayRequestEvent => {
   // still treating these values because in the default case they must be reflected in other
   // parts of the object
   const httpMethod = withDefault(params?.httpMethod, 'GET'),
         path = withDefault(params?.path, '/echo/asdf/a');

   return {
      path,
      httpMethod,
      body: null,
      isBase64Encoded: false,
      resource: '/{proxy+}',
      pathParameters: { proxy: path },
      stageVariables: null,
      requestContext: makeAPIGatewayRequestContext({
         httpMethod,
      }),
      headers: makeAPIGatewayRequestEventHeaders(),
      multiValueHeaders: makeAPIGatewayRequestEventMultiValueHeader(),
      queryStringParameters: {
         'foo[a]': 'bar b',
         x: '2',
         y: 'z',
      },
      multiValueQueryStringParameters: {
         'foo[a]': [ 'bar b', 'baz c' ],
         x: [ '1', '2' ],
         y: [ 'z' ],
      },
      
      ...params,
   };
};

It seems to have a similar behavior and it is easier for developers to add more of the dynamic fields in the future, since they only need to update the interface if no further treatment is needed like in the cases of path and httpMethod. What do you think?

@MatheusBaldi
Copy link
Contributor Author

MatheusBaldi commented Dec 19, 2024

@onebytegone the amount of changed files increased because the refactorings demanded some other files that used the old apiGatewayRequest to be updated, but they're all related to api gateway only. If needed, I could separate the feature commits from the refactoring ones.

@MatheusBaldi MatheusBaldi changed the title feat: improve fake events creation feat: improve fake api gateway request events creation Dec 19, 2024
tests/samples.ts Outdated
Comment on lines 12 to 25
}
interface MakeAPIGatewayRequestEventHeadersParams {
[name: string]: string | undefined;
}

interface MakeAPIGatewayRequestEventMultiValueHeadersParams {
[name: string]: string[] | undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
interface MakeAPIGatewayRequestEventHeadersParams {
[name: string]: string | undefined;
}
interface MakeAPIGatewayRequestEventMultiValueHeadersParams {
[name: string]: string[] | undefined;
}
}
interface MakeAPIGatewayRequestEventHeadersParams {
[name: string]: string | undefined;
}
interface MakeAPIGatewayRequestEventMultiValueHeadersParams {
[name: string]: string[] | undefined;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be a good idea to add it as a lint rule? This behavior exists for const, let and var in the padding-line-between-statements rule

tests/samples.ts Outdated
accountId: '123456789012',
apiId: 'someapi',
authorizer: null,
httpMethod: params?.httpMethod ?? 'GET',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See prior note about the null coalescing operator.

Suggested change
httpMethod: params?.httpMethod ?? 'GET',
httpMethod: params?.httpMethod || 'GET',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected with the withDefault function

tests/samples.ts Outdated
Comment on lines 180 to 181
export const makeAPIGatewayRequestEventHeaders =
(params?: MakeAPIGatewayRequestEventHeadersParams): APIGatewayRequestEvent['headers'] => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For functions, we prefer the function declaration style. The other functions in this file were from before we had a set standard.

Suggested change
export const makeAPIGatewayRequestEventHeaders =
(params?: MakeAPIGatewayRequestEventHeadersParams): APIGatewayRequestEvent['headers'] => {
export function makeAPIGatewayRequestEventHeaders(params?: MakeAPIGatewayRequestEventHeadersParams): APIGatewayRequestEvent['headers'] {

tests/samples.ts Outdated
Comment on lines 182 to 203
return withDefault<APIGatewayRequestEvent['headers']>(
params,
{
Accept: '*/*',
'CloudFront-Forwarded-Proto': 'https',
'CloudFront-Is-Desktop-Viewer': 'true',
'CloudFront-Is-Mobile-Viewer': 'false',
'CloudFront-Is-SmartTV-Viewer': 'false',
'CloudFront-Is-Tablet-Viewer': 'false',
'CloudFront-Viewer-Country': 'US',
Host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
'User-Agent': 'curl/7.54.0',
Via: '2.0 4ee511e558a0400aa4b9c1d34d92af5a.cloudfront.net (CloudFront)',
'X-Amz-Cf-Id': 'xn-ohXlUAed-32bae2cfb7164fd690ffffb87d36b032==',
'X-Amzn-Trace-Id': 'Root=1-4b5398e2-a7fbe4f92f2e911013cba76b',
'X-Forwarded-For': '8.8.8.8, 2.3.4.5',
'X-Forwarded-Port': '443',
'X-Forwarded-Proto': 'https',
Referer: 'https://en.wikipedia.org/wiki/HTTP_referer',
Cookie: 'uid=abc; ga=1234; foo=bar; baz=foo%5Ba%5D; obj=j%3A%7B%22abc%22%3A123%7D; onechar=j; bad=j%3A%7Ba%7D',
}
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the developer only wants to edit a single header?

Suggested change
return withDefault<APIGatewayRequestEvent['headers']>(
params,
{
Accept: '*/*',
'CloudFront-Forwarded-Proto': 'https',
'CloudFront-Is-Desktop-Viewer': 'true',
'CloudFront-Is-Mobile-Viewer': 'false',
'CloudFront-Is-SmartTV-Viewer': 'false',
'CloudFront-Is-Tablet-Viewer': 'false',
'CloudFront-Viewer-Country': 'US',
Host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
'User-Agent': 'curl/7.54.0',
Via: '2.0 4ee511e558a0400aa4b9c1d34d92af5a.cloudfront.net (CloudFront)',
'X-Amz-Cf-Id': 'xn-ohXlUAed-32bae2cfb7164fd690ffffb87d36b032==',
'X-Amzn-Trace-Id': 'Root=1-4b5398e2-a7fbe4f92f2e911013cba76b',
'X-Forwarded-For': '8.8.8.8, 2.3.4.5',
'X-Forwarded-Port': '443',
'X-Forwarded-Proto': 'https',
Referer: 'https://en.wikipedia.org/wiki/HTTP_referer',
Cookie: 'uid=abc; ga=1234; foo=bar; baz=foo%5Ba%5D; obj=j%3A%7B%22abc%22%3A123%7D; onechar=j; bad=j%3A%7Ba%7D',
}
);
return compact({
Accept: '*/*',
'CloudFront-Forwarded-Proto': 'https',
'CloudFront-Is-Desktop-Viewer': 'true',
'CloudFront-Is-Mobile-Viewer': 'false',
'CloudFront-Is-SmartTV-Viewer': 'false',
'CloudFront-Is-Tablet-Viewer': 'false',
'CloudFront-Viewer-Country': 'US',
Host: 'b5gee6dacf.execute-api.us-east-1.amazonaws.com',
'User-Agent': 'curl/7.54.0',
Via: '2.0 4ee511e558a0400aa4b9c1d34d92af5a.cloudfront.net (CloudFront)',
'X-Amz-Cf-Id': 'xn-ohXlUAed-32bae2cfb7164fd690ffffb87d36b032==',
'X-Amzn-Trace-Id': 'Root=1-4b5398e2-a7fbe4f92f2e911013cba76b',
'X-Forwarded-For': '8.8.8.8, 2.3.4.5',
'X-Forwarded-Port': '443',
'X-Forwarded-Proto': 'https',
Referer: 'https://en.wikipedia.org/wiki/HTTP_referer',
Cookie: 'uid=abc; ga=1234; foo=bar; baz=foo%5Ba%5D; obj=j%3A%7B%22abc%22%3A123%7D; onechar=j; bad=j%3A%7Ba%7D',
...params,
});

Copy link
Contributor Author

@MatheusBaldi MatheusBaldi Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onebytegone I like this approach. I've also left a question about applying it for the base object, what do you think?

I didn't understand the compact function here. Why would we define falsy values if they're going to be removed? Wouldn't these values be expected to be returned, even as falsy? Is it the same compact from the silvermine/toolbox? It seems to me that it is meant for arrays.

After applying the suggested changes, the easiest way for the developer to completely override those values, if needed, will be to use the spread operator or the _.extends function in the level of the parent object.

tests/samples.ts Outdated
Comment on lines 225 to 230
headers = withDefault(params?.headers, makeAPIGatewayRequestEventHeaders());

const multiValueHeaders = withDefault(
params?.multiValueHeaders,
makeAPIGatewayRequestEventMultiValueHeader()
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach we're putting the burden of ensuring headers and multiValueHeaders are in sync on the developer writing the tests. Do we have tests which need to override only multiValueHeaders? If so, what if multiValueHeaders is normally generated dynamically from headers but can have overrides provided via multiValueHeaders. If we go that route MakeAPIGatewayRequestEventParams will need a comment clearly explaining that headers should normally be used but multiValueHeaders can be used to selectively override multi-value header values.

Copy link
Contributor Author

@MatheusBaldi MatheusBaldi Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onebytegone If we implement it to generate multiValueHeaders from headers, how things should work when the developer only need to think about multiValueHeaders? If he tries changing it through the maker function params, using the multiValueHeaders, which is what interests him, he'll need to check implementation details of the function. Do you consider it a problem?

tests/samples.ts Outdated
import withDefault from './test-utils/withDefault';

interface MakeAPIGatewayRequestEventContextParams {
httpMethod?: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the aws-lambda types allow null for httpMethod.

Suggested change
httpMethod?: string | null;
httpMethod?: string;

Comment on lines 495 to 497
evt = _.extend(makeAPIGatewayRequestEvent(), {
multiValueHeaders: { 'Referer': [ referer ] },
headers: { 'Referer': referer },
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the purpose of makeAPIGatewayRequestEvent is to make an event, allowing specific values to be overridden, should we still be using _.extend here? There are a number of places this is happening in the in PR, only commenting in this one.

Copy link
Contributor Author

@MatheusBaldi MatheusBaldi Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't, thanks.

@MatheusBaldi MatheusBaldi force-pushed the MatheusBaldi/improve-fake-events-creation branch 2 times, most recently from 6e4ad20 to 637362a Compare December 26, 2024 14:31
@MatheusBaldi MatheusBaldi force-pushed the MatheusBaldi/improve-fake-events-creation branch from 637362a to f1507e2 Compare December 26, 2024 14:59
Comment on lines 24 to 25
const makeRequestEvent = (path: string, base?: RequestEvent, method?: string): RequestEvent => {
return _.extend(base || apiGatewayRequest(), { path: path, httpMethod: (method || 'GET') });
return _.extend(base, { path: path, httpMethod: (method || 'GET') });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if makeRequestEvent doesn't have a base provided? If we should always provide base, can we remove the optional flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, thanks. This is going to be removed when the alb makers are introduced in a follow up PR, the places where it is used now are providing a base object, but, if not provided, that wouldn't work properly.

Comment on lines 394 to 395
evt.headers['X-Forwarded-Proto'] = 'http';
evt.multiValueHeaders['X-Forwarded-Proto'] = [ 'http' ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the X-Forwarded-Proto header be passed into makeAPIGatewayRequestEvent instead of modifying the event after creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This evt is actually not even used here. I'll remove it.

@@ -250,7 +250,7 @@ describe('Request', () => {

it('parses proper values - APIGW', () => {
_.each(testCases, (testCase) => {
let evt: RequestEvent = apiGatewayRequest(),
let evt: RequestEvent = makeAPIGatewayRequestEvent(),
req;

evt.headers.Host = testCase.host;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these headers be passed into makeAPIGatewayRequestEvent?

Copy link
Contributor Author

@MatheusBaldi MatheusBaldi Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @onebytegone I tried to find some similar cases and found one in the test 'does not throw an error when bad values supplied', around line 539. It is changing queryStringParameters and multiValueQueryStringParameters. Do you think the maker function should allow these attributes to be changed as well?

tests/samples.ts Outdated
Comment on lines 30 to 43
headers?: MakeAPIGatewayRequestEventHeadersParams;
multiValueHeaders?: MakeAPIGatewayRequestEventMultiValueHeaderParams;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Able to add comments explaining why using headers is preferred, but multiValueHeaders can be used to set multiple values for specific headers?

@MatheusBaldi MatheusBaldi force-pushed the MatheusBaldi/improve-fake-events-creation branch from f1507e2 to 623ffe8 Compare January 31, 2025 16:10
receives a value in its first parameter `value` and a default value in its second
parameter `defaultValue`. If `value` is undefined, returns `defaultValue`, otherwise
returns `value`
- makeAPIGatewayRequestEvent accepts an input object, allowing the developer to change
values of desired fields when needed, while apiGatewayRequest would always return the
same object
- makeAPIGatewayRequestEvent accepts an input object, allowing the developer to change
values of desired fields when needed, while apiGatewayRequest would always return the
same object
- makeAPIGatewayRequestEvent accepts an input object, allowing the developer to change
values of desired fields when needed, while apiGatewayRequest would always return the
same object
@MatheusBaldi MatheusBaldi force-pushed the MatheusBaldi/improve-fake-events-creation branch from 623ffe8 to b9fecef Compare February 11, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants