docs: add API design overview #16

sf-clouder · 2019-12-04T13:24:21Z

No description provided.

afaq-sf

@sf-clouder Your plan and it looks reasonable to me. Thank you for sharing the early draft via PR.

Please proceed with the test.

afaq-sf · 2019-12-11T02:44:45Z

api-design-overview.md

+
+## Questions
+
+1. Do we need to secure the API? (API key, lambda authorizer, something else?)


Great question. Please secure the API with an authorizer lambda, if time allows.

afaq-sf · 2019-12-11T02:47:07Z

api-design-overview.md

+
+1. Do we need to secure the API? (API key, lambda authorizer, something else?)
+
+2. Should the API generate the unique user ID? Or does the user request the User ID, and the API determines it's uniqueness?


I would ask the same question back to you: what would you recommend and why?

afaq-sf · 2019-12-11T02:48:56Z

api-design-overview.md

+
+2. Should the API generate the unique user ID? Or does the user request the User ID, and the API determines it's uniqueness?
+
+3. Do we want to store the data exactly as shown in the model? (i.e. address as a simple string). Or do we want to break address into an object with attributes like street, city, state, zip, etc?


The model shown is only for the sake of simplicity.

However, we would welcome further improvements into an object model based on what you might deem appropriate. Address as an object definitely makes sense to me, in that regard.

…reate and update lambdas

…nt and api use

sf-clouder · 2020-01-03T20:55:54Z

Hello Superformula! Please review this submission for the cloud native backend test. I have modified README.md with details about the project and how to deploy it. Thank You!

afaq-sf

@sf-clouder I have left some questions on the PR. I would like to get your thoughts on them as much as time allows.

afaq-sf · 2020-01-14T00:28:54Z

README.md


-These may be used for further challenges. You can freely skip these if you are not asked to do them; feel free to try out if you feel up to it.
+## Create a User


Have you ever used API Gateway export to dynamically generate API documentation?

https://docs.aws.amazon.com/cli/latest/reference/apigateway/get-export.html

Thank you for your review, Afaq. Yes, the ability to export was a primary reason behind defining the methods and validation within API Gateway instead of proxying and handling it within Lambda.

afaq-sf · 2020-01-14T00:30:05Z

.prettierrc

+ "tabWidth": 4,
+ "useTabs": false,
+ "semi": true
+}


I noticed some of your files lack newlines at the end of the file.
have you considered using editorconfig or similar tool to auto-add line endings?

This is a great suggestion. I read up on why an EOF newline can be helpful. Prior, I knew about it as an option, but didn't understand the reason behind it until you pointed it out. Thank you.

afaq-sf · 2020-01-14T00:34:20Z

lib/UsersStack.js

+ partitionKey: { name: 'pk', type: dynamodb.AttributeType.STRING },
+ sortKey: { name: 'sk', type: dynamodb.AttributeType.STRING },
+ billingMode: dynamodb.BillingMode.PAY_PER_REQUEST,
+ removalPolicy: cdk.RemovalPolicy.DESTROY, // allow table delete


Do you foresee any issues in real-world applications if the stack allows the table to be removed?

For sure. This line is not desired in a production setting as it would allow a delete of the table. I left it there only so you could fully remove the stack upon a 'cdk destroy' during this demo.

afaq-sf · 2020-01-14T00:47:00Z

lib/UsersStack.js

+ code: new lambda.AssetCode(crudAsset),
+ handler: crudHandler,
+ runtime: lambda.Runtime.NODEJS_12_X,
+ memorySize: 1024,


How would you recommend memorySize configurable?
For example for different configurations between deployable environments.

This could be fed in with environment variables, or computed in the CDK stack itself. A variation could look to AWS SSM to avoid putting the values in the codebase. Another option is to bundle config files with each lambda with definitions for each environment. Let me know if you have any other techniques or interesting ideas here.

afaq-sf · 2020-01-14T01:01:00Z

src/lambdas/createUser/createUser.js

+ const item = typeof event.body === 'object' ? event.body : JSON.parse(event.body);
+
+ // Get UUID for user id from the lambda request id
+ const userId = context.awsRequestId;


Interesting choice here. Out of curiosity, what is the reasoning for using aws request ID as the UUID of the new user?

In the past I've used the 'uuid' package to generate ids. However, I recently learned that the Lambda request ID is itself a V4 UUID, so I thought it would be good to take advantage of it and save the import. This is similar, though not exact, to '$util.autoId()' that is available in VTL.

afaq-sf · 2020-01-14T01:18:53Z

src/lambdas/createUser/createUser.js

+ }
+
+ Log.error('User Create Error', new Error(err));
+ return httpResponse(400, err);


Is 400 a reasonable default error status?

For example, what if the failure is because of inability to reach Dynamo or code-related programmer error?

Yes, good point. I was using 400 was a catch all here, but I agree that more meaningful status codes in the 4xx and 5xx range with appropriate error messages should be returned.

afaq-sf · 2020-01-14T01:19:05Z

src/lambdas/createUser/createUser.js

+ } catch (err) {
+ // dynamo did not store because the record already exists
+ if (err.code === 'ConditionalCheckFailedException') {
+ return httpResponse(409, 'a user with this ID already exists');


Can you please share your thoughts on the pros/cons of the approach you've taken here for checking if the user already exists.

Especially when weighted against doing an initial check against the table.

In DynamoDB, when doing a put to an existing primary key, it will overwrite an existing item, which may not be the desired effect. By adding the conditional check, verifying that a particular attribute does not exist, it blocks the overwrite. My approach to using the conditional write was to avoid having to first do a 'get' to read any potential existing item to see if it exists, and then having to follow it up with a 'put'.

Thank you for calling attention to this section as it made me realize my check was against the sort key 'sk' and should have been against the partition key 'pk'. Earlier in the design I was storing the UUID as the sort key, but then changed to store the UUID as the partition key instead and forgot to update the condition expression. This code should be updated so that the conditional check is against the partition key: 'attribute_not_exists(pk)'.

Edit: Sorry, it's been a couple weeks since I submitted the PR. Looking closer at my code it doesn't matter whether the condition expression check is against the pk or the sk, since it's simply checking for the existence of the attribute, not it's value. Checking for the existence of either works equally well in this case.

afaq-sf · 2020-01-14T01:27:05Z

src/lambdas/deleteUser/deleteUser.js

+ const originalParams = { TableName: TABLE_NAME, Key: { pk: userId, sk: `USER` } };
+ const doc = await db.get(originalParams).promise();
+ const { Item } = doc;
+ if (!Item) throw 'user does not exist';


This is a reasonable check.
What are your thoughts on returning 200 rather than throwing an error here?

I can see 200 being an appropriate response in this situation as well since, like a successful delete, it is confirming that the user does not exist.

afaq-sf · 2020-01-14T01:29:47Z

src/lambdas/deleteUser/deleteUser.js

+ await db.put(backupParams).promise();
+
+ // delete the original
+ await db.delete(originalParams).promise();


How would you go about building an audit log of all delete requests and capturing who initiated it as well as other details?

I like the idea of monitoring the Dynamo stream for changes like this so we don't tie up the delete lambda with this extra work. We could also move the entire delete request to an SQS queue, immediately returning a 202 status code (Accepted), and then have a lambda process the queue and record the audit data.

afaq-sf · 2020-01-14T01:30:44Z

src/lambdas/deleteUser/deleteUser.js

+ }
+};
+
+const httpResponse = (statusCode, body) => {


Since this is common code repeated within lambdas, how would you recommend abstracting it to shared tooling?

Normally I would put code like this in a shared module and import it into each lambda. However, the CDK only supports deploying the lambda by specifying a folder or a ZIP file. So putting it in a common module would have required duplicating the module in each lambda's directory, or putting all lambda code in the same directory, which creates lambdas with unused code from all the other lambdas. Neither of which was ideal. Because of this, and since the HTTP response function was small, I simply chose to duplicate it in each lambda.

Eventually the AWS CDK is going to support efficiently bundling lambda code during synthesis of the CloudFormation (see here). Until then, I would use webpack or parcel to to solve this, or write a build script that creates a ZIP package for each Lambda as a pre-deploy step.

docs: add API design overview

d82182c

afaq-sf reviewed Dec 11, 2019

View reviewed changes

sf-clouder added 13 commits January 3, 2020 10:11

chore: project init

d3f03d0

style: install prettier and eslint

c0f870e

chore(cdk): install aws cdk core

a8e7a87

chore(cdk): install cdk packages for apig, dynamo, lambda, sqs

74cff4f

feat(stack): basic cdk stack with dynamo table

d3ed2eb

feat(dynamo): add gsi for sk-state-city-zip

fa3035a

feat(stack): add api gateway with resources for /users and /users/{id}

3e24cc3

chore(lambda): install clone, clean, merge and logging packages

8fb7ee0

feat(stack): add lambda layer to stack

8051c09

feat(lambda): add create, read, update, delete functions

72a48bf

feat(api): add user model to api for payload validation in front of c…

ef79d9b

…reate and update lambdas

docs: remove outdated api design overview

1d59d3b

docs(readme): modify readme.md with info about architecture, deployme…

e3154f8

…nt and api use

afaq-sf reviewed Jan 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add API design overview #16

docs: add API design overview #16

sf-clouder commented Dec 4, 2019

afaq-sf left a comment

afaq-sf Dec 11, 2019

afaq-sf Dec 11, 2019

afaq-sf Dec 11, 2019

sf-clouder commented Jan 3, 2020

afaq-sf left a comment

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020 •

edited

Loading

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020 •

edited

Loading

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020 •

edited

Loading

afaq-sf Jan 14, 2020

sf-clouder Jan 14, 2020


		## Questions

		1. Do we need to secure the API? (API key, lambda authorizer, something else?)


		1. Do we need to secure the API? (API key, lambda authorizer, something else?)

		2. Should the API generate the unique user ID? Or does the user request the User ID, and the API determines it's uniqueness?


		2. Should the API generate the unique user ID? Or does the user request the User ID, and the API determines it's uniqueness?

		3. Do we want to store the data exactly as shown in the model? (i.e. address as a simple string). Or do we want to break address into an object with attributes like street, city, state, zip, etc?


		These may be used for further challenges. You can freely skip these if you are not asked to do them; feel free to try out if you feel up to it.
		## Create a User

docs: add API design overview #16

Are you sure you want to change the base?

docs: add API design overview #16

Conversation

sf-clouder commented Dec 4, 2019

afaq-sf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sf-clouder commented Jan 3, 2020

afaq-sf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sf-clouder Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sf-clouder Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sf-clouder Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sf-clouder Jan 14, 2020 •

edited

Loading

sf-clouder Jan 14, 2020 •

edited

Loading

sf-clouder Jan 14, 2020 •

edited

Loading