Add the json-deserializer middleware package #57

matt-jenner · 2021-07-25T13:17:47Z

This implements a json-deserializer middleware, it expects an object extending ApiGatewayProxyEvent and returns an ApiGatewayProxyObjectEvent which replaces the string body for an object of type Record<string, unknown>.

It introduces the concept of a RequestBodyNotJsonError to be used to catch and return an appropriate 400 error back if a non json payload is passed.

If no body or a non-json header is passed it will pass null as the body property on the event passed to the underlying handler.

Closes #54

Checklist:

I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

I'm sorry for the delays in supplying this PR, but things got crazily busy at work, and then summer (and a large DIY project - building a garden office now my work has decided no-one is going back to the office full-time) happened which I'm afraid took most of my spare time and I like to code when fully able to commit myself to it.

Although I mostly got this done within a few hours work. I ran into an issue with the 100% code-coverage requirements which caused me to delay submiting. While I do have full coverage in TS (and I always like to see with libraries I expect someone to depend upon), there was one issue where the super(message) line in the RequestBodyNotJsonError class would NOT mark itself as covered despite definately hitting the line in my tests.

Jest reports the line as not-covered due to the coverage being applied to ES5 code rather than the TS itself. I've confirmed coverage is 100% as if you switch the output to ES6 it reports 100% coverage.

After reading quite a bit around the issue, it seems to relate to the fact that when the TS line:
super(message)

is transpiled down to ES5, it comes out as:
var _this = _super.call(this, message) || this;

While the former part of the or statement is covered, it doesn't cover all the cases of the transpiled statement fully.

I've tried various approaches to resolve this including a suggestion of adding https://www.npmjs.com/package/ts-es5-istanbul-coverage but eventually thought it best to submit it cleanly without this and see your thoughts.

See here for more information:

Otherwise, hopefully the PR should be otherwise mostly self-explanatory, the logic itself is very simple and is the precursor for my other feature request logged in #55.

…r package This implements a json-deserializer middleware, it expects an object extending ApiGatewayProxyEvent and returns an ApiGatewayProxyObjectEvent which replaces the string body for an object of type Record<string, unknown>. It introduces the concept of a RequestBodyNotJsonError to be used to return an appropriate 400 error back if passed a non json payload is passed. If no body or a non-json header is passed it will pass null as the body property on the event passed to the underlying handler.

CLAassistant · 2021-07-25T13:17:52Z

All committers have signed the CLA.

dbartholomae

Won't be able to do a full review until I'm back from vacation, but here are already some observations

packages/json-deserializer/src/JsonDeserializer.test.ts

packages/json-deserializer/src/JsonDeserializer.ts

dbartholomae · 2021-07-25T14:40:28Z

Concerning the coverage, just adding a comment explaining it and an Istanbul ignore next might well suffice

Worked around issue with code-coverage in RequestBodyNotJsonError.ts by adding istanbul ignore next comment and an explanation of why as-agreed.

dbartholomae

Thanks for waiting! I've added comments to make it easy to add the status code to the error. I'll also check why CI isn't running on this PR. There might need to run rush change to also add a changelog. But CI should tell us.

packages/json-deserializer/src/JsonDeserializer.test.ts

packages/json-deserializer/src/customErrors/RequestBodyNotJsonError.ts

dbartholomae · 2021-08-30T20:24:55Z

Please also rebase, this should run CI for the branch (I've just updated the main branch with a small config change).

…r package This implements a json-deserializer middleware, it expects an object extending ApiGatewayProxyEvent and returns an ApiGatewayProxyObjectEvent which replaces the string body for an object of type Record<string, unknown>. It introduces the concept of a RequestBodyNotJsonError to be used to return an appropriate 400 error back if passed a non json payload is passed. If no body or a non-json header is passed it will pass null as the body property on the event passed to the underlying handler.

Worked around issue with code-coverage in RequestBodyNotJsonError.ts by adding istanbul ignore next comment and an explanation of why as-agreed.

…enner/lambda-middleware into feature/json-deserializer

codecov · 2021-08-30T21:38:00Z

Codecov Report

Merging #57 (dfa8dbd) into main (30b8e5e) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #57   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           54        54           
  Lines          520       520           
  Branches        98        98           
=========================================
  Hits           520       520

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30b8e5e...dfa8dbd. Read the comment docs.

@dbartholomae

This alters the error response to return a statusCode property with a value of 400 to make it compatible with the error-handling middleware also included in the lambda-middleware package as suggested by @dbartholomae. Co-authored-by: Daniel Bartholomae <daniel@bartholomae.name>

Removed the regex used to identify a JSON mime-type as this was potentially vulnerable to 'Regular expression Denial of Service attacks' and making changes to make the regex more restrictive (but compliant with RFC 6838) did not pass tests for this issue online. This now matches based on the last segment of a mimetype being json or json; this should match all the expected mime types. Updated unit-tests with additional tests for a variety of compatible mime types.

matt-jenner · 2021-08-30T23:51:48Z

Thanks for the review, I hope you had a good holiday. I've added your suggestions and fixed one codeQL vulnerability relating to Regular expression denial of service (it was simpler to just remove the regex 🙂).

It's running the build now but for some reason it's not seeing my package in the build, it lists 13 packages not the 14 I see if I run rush test:integration or rush test:unit locally and the project is listed in rush.json so I would have thought it would have picked it up?

Any ideas on this front as I'm afraid I'm not a rush expert, thanks 🙂.

matt-jenner · 2021-09-01T12:28:07Z

Thanks for the approval - it will feel good to finally get this in, I think you need to merge this manually though as mergify is complaining that the workflow has changed. Once this is in, I can start on the next issue I logged to create the io-ts validator middleware before I'm due back in work next week, thanks 🙂.

dbartholomae · 2021-09-01T13:12:51Z

Thanks! I now figured out the problem: When I changed the settings to run the GitHub actions for this PR, I changed them in a way that lead to actions running on the main branch code instead of the PR branch code instead. I've just updated the main branch to fix this. If you could rebase/merge main one more time, it should be solved :)

…izer

dbartholomae · 2021-09-01T15:19:30Z

Just realized that you gave me access to the branch, so I did the merge of main myself and will merge this branch once CI is green.

The previous runtime version is no longer valid.

The old notation is deprecated now See https://www.serverless.com/framework/docs/deprecations/#SERVICE_OBJECT_NOTATION

Currently, an empty body in serverless will change the status code from 200 to 204

dbartholomae · 2021-09-01T19:00:13Z

There was also a slight problem with the integration test: it still used body instead of bodyObject. It's fixed, and version 1.1.0 is now available on npm.

matt-jenner · 2021-09-01T20:13:42Z

There was also a slight problem with the integration test: it still used body instead of bodyObject. It's fixed, and version 1.1.0 is now available on npm.

Thanks for fixing that, I really mustn't code quite so late 🙂 looking forward to switching my project over to use this now and start on the next middleware 😁

matt-jenner added 3 commits July 25, 2021 12:08

docs(json-deserializer): Added readme.md to json-deserializer

ed72ce7

docs: Added packages/json-deserializer to main readme.md

5165316

matt-jenner mentioned this pull request Jul 25, 2021

Add io-ts validator middleware #55

Closed

3 tasks

dbartholomae reviewed Jul 25, 2021

View reviewed changes

matt-jenner added 4 commits July 28, 2021 08:40

fix(json-deserializer): fixed code-coverage

0312425

Worked around issue with code-coverage in RequestBodyNotJsonError.ts by adding istanbul ignore next comment and an explanation of why as-agreed.

refactor(json-deserializer): remove should from unit-test statements

890e9dd

refactor(json-deserializer): replace let with const in jsonDeserializer

ab9a9ab

feat(json-deserializer): do not overwrite body use bodyObject

12f7383

dbartholomae requested changes Aug 30, 2021

View reviewed changes

packages/json-deserializer/src/JsonDeserializer.test.ts Show resolved Hide resolved

packages/json-deserializer/src/customErrors/RequestBodyNotJsonError.ts Show resolved Hide resolved

packages/json-deserializer/src/customErrors/RequestBodyNotJsonError.ts Show resolved Hide resolved

matt-jenner added 8 commits August 30, 2021 22:33

docs(json-deserializer): Added readme.md to json-deserializer

dee51cf

docs: Added packages/json-deserializer to main readme.md

c4bfa4e

fix(json-deserializer): fixed code-coverage

7d0ab74

Worked around issue with code-coverage in RequestBodyNotJsonError.ts by adding istanbul ignore next comment and an explanation of why as-agreed.

refactor(json-deserializer): remove should from unit-test statements

8b12ef5

refactor(json-deserializer): replace let with const in jsonDeserializer

42c6ce5

feat(json-deserializer): do not overwrite body use bodyObject

972a978

Merge branch 'feature/json-deserializer' of https://github.com/matt-j…

fe5e1b2

…enner/lambda-middleware into feature/json-deserializer

matt-jenner and others added 3 commits August 30, 2021 22:52

chore(json-deserializer): added changes after calling rush change

95d87dc

dbartholomae approved these changes Sep 1, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into feature/json-deserial…

f4c0c3c

…izer

dbartholomae added 8 commits September 1, 2021 18:48

fix: solve TypeScript problem in example

f54cd0d

chore: update serverless dependencies

a72b369

fix: update example to use bodyObject instead of body

deed126

chore: add changelog update

3fb29a3

chore: update serverless runtime

63db063

The previous runtime version is no longer valid.

chore: update serverless versions in template

7524415

chore: set service name directly in serverless.yml files

c016054

The old notation is deprecated now See https://www.serverless.com/framework/docs/deprecations/#SERVICE_OBJECT_NOTATION

fix: use example compatible with serverless

dfa8dbd

Currently, an empty body in serverless will change the status code from 200 to 204

mergify bot merged commit 4514449 into dbartholomae:main Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the json-deserializer middleware package #57

Add the json-deserializer middleware package #57

matt-jenner commented Jul 25, 2021

CLAassistant commented Jul 25, 2021 •

edited

Loading

dbartholomae left a comment

dbartholomae commented Jul 25, 2021

dbartholomae left a comment

dbartholomae commented Aug 30, 2021

codecov bot commented Aug 30, 2021 •

edited

Loading

matt-jenner commented Aug 30, 2021

matt-jenner commented Sep 1, 2021

dbartholomae commented Sep 1, 2021

dbartholomae commented Sep 1, 2021

dbartholomae commented Sep 1, 2021

matt-jenner commented Sep 1, 2021

Add the json-deserializer middleware package #57

Add the json-deserializer middleware package #57

Conversation

matt-jenner commented Jul 25, 2021

Checklist:

CLAassistant commented Jul 25, 2021 • edited Loading

dbartholomae left a comment

Choose a reason for hiding this comment

dbartholomae commented Jul 25, 2021

dbartholomae left a comment

Choose a reason for hiding this comment

dbartholomae commented Aug 30, 2021

codecov bot commented Aug 30, 2021 • edited Loading

Codecov Report

matt-jenner commented Aug 30, 2021

matt-jenner commented Sep 1, 2021

dbartholomae commented Sep 1, 2021

dbartholomae commented Sep 1, 2021

dbartholomae commented Sep 1, 2021

matt-jenner commented Sep 1, 2021

CLAassistant commented Jul 25, 2021 •

edited

Loading

codecov bot commented Aug 30, 2021 •

edited

Loading