feat: add in i18n support for validator #161

willfarrell · 2018-04-22T18:22:09Z

Includes: ajv i18n support (Default: en)

Also includes the ability to customize error format

- revert how error was returned to pass tests - lint code

codecov · 2018-04-22T18:47:42Z

Codecov Report

Merging #161 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #161   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          17     17           
  Lines         366    379   +13     
  Branches       75     78    +3     
=====================================
+ Hits          366    379   +13

Impacted Files	Coverage Δ
src/middlewares/validator.js	`100% <100%> (ø)`	⬆️

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 c24ee61...a6e97bd. Read the comment docs.

lmammino

Great proposal @willfarrell, this would be really useful. Thanks a million for it!

Anyway there are some details in the current implementation on which I am not completely sold and I would prefer to reuse features from existing middlewares rather than adding more features to the validator one. Please refer to the specific comments for more details.

Aside from that, once we finalise the implementation, we should make sure we updated Docs and Typings.

lmammino · 2018-04-22T18:54:29Z

package.json

@@ -13,7 +13,7 @@
  ],
  "types": "./index.d.ts",
  "scripts": {
-    "test:lint": "eslint --ignore-pattern='node_modules/' --ignore-pattern='coverage/' --ignore-pattern='docs/' .",
+    "test:lint": "eslint --fix --ignore-pattern='node_modules/' --ignore-pattern='coverage/' --ignore-pattern='docs/' .",


Why are you adding --fix here?

IMHO, the test command is meant to only check for errors, not fixing them. You should run eslint --fix either on save in your IDE (automatically), or manually before commit.

Sorry, ment to remove this before commit. Habit from a pattern I use where my setup auto formats and lint on commit. No IDE settings or remembering to run lint before commit. Check out lint-staged and husky.

Will be removed in next commit.

lmammino · 2018-04-22T18:57:26Z

package.json

@@ -66,7 +66,9 @@
  "dependencies": {
    "@types/aws-lambda": "^8.10.1",
    "@types/http-errors": "^1.6.1",
+    "accept-language": "^3.0.18",


Middy already has negotiator, which should take care of negotiating languages. Do we really need an extra dependency?

Totally didn't see that other module, will refactor to use negotiator.

lmammino · 2018-04-22T19:00:52Z

src/middlewares/validator.js

+}
+
+module.exports = ({ inputSchema, outputSchema, ajvOptions, errorFormat = errors => errors }) => {
+  const acceptLanguage = require('accept-language')


Instead re-doing this here, can we delegate somehow the work of processing the languages to the already available (and configurable) HTTP Content Negotiation middleware?

If we do that, we should just assume that the event object will contain a property called preferredLanguage.

I thought about that, leveraging another middleware to do the parsing. Opted for having it included within due to the allowed languages being different than what the overall API may support. Plus allows for the middleware to be stand alone. I've refactored to leverage negotiator for now, but further discussion around this should continue.

lmammino · 2018-04-22T19:02:54Z

src/middlewares/validator.js

+  $data: true // required for ajv-keywords
+}
+
+module.exports = ({ inputSchema, outputSchema, ajvOptions, errorFormat = errors => errors }) => {


On one side, I like the idea of the errorFormat function. On the other, I would prefer to keep this middleware simpler and encourage people to combine it with the usage of the HTTP Error Handler middleware.

I've been writing a jsonapi middleware that essentially replaces httpErrorHandler. At the time of writing it made sense to do the formatting here, there have been changes since that might make more sense to move it to another middleware. I've done some testing, we're good to remove.

willfarrell · 2018-04-22T23:39:00Z

Thanks for the great feedback @lmammino. Changes pushed, ready for next round.

lmammino

I will spend a bit of time playing with your implementation and see if we can generalise some parts. This way I should be able to do a more accurate review 😉

lmammino · 2018-04-24T21:02:52Z

src/middlewares/validator.js

+    'sk',
+    'sv',
+    'zh'
+  ])


Still not sure it's a good approach to list the languages here and not having any opportunity to configure the selection

…e/validator_i18n

…n combination with the content-negotiation middleware

lmammino · 2018-04-24T21:40:58Z

Hey @willfarrell and others. Please check my suggested implementation (a296341) and let me know what you think.

I added few comments in the PR to better describe the changes, but here follows the main ones:

I am assuming here that you will use the middleware in combination with the Content Negotiation Middleware, which, if attached before, should initialize the field event.preferredLanguage.
I am also supporting the possibility to redefine the default language if needed and normalising the Portuguese language.

If you like this approach, I'll make sure to update the documentation and the typings

lmammino · 2018-04-24T21:42:13Z

src/middlewares/validator.js

-  ])
-  if (!results.length) return 'en'
-  return results[0]
+const availableLanguages = Object.keys(ajvLocalize)


This saves us the trouble of having a hardcoded list of languages

lmammino · 2018-04-24T21:43:57Z

src/middlewares/validator.js

+  'pt_BR': 'pt-BR',
+  'pt_br': 'pt-BR'
+}
+


Makes Portuguese less ambiguous and it will work with all possible variations.
Honestly, I am not sure if it's a good idea to go down the loophole of dealing with this kind of things. So far, looking at ajv-i18n, only Portuguese was the outlier, so I thought it was easy enough to support all the possible variations.

lmammino · 2018-04-24T21:44:43Z

src/middlewares/validator.js

-        console.log(chooseLanguage(handler.event))
-        const locale = chooseLanguage(handler.event)
-        ajvLocalize[locale](validateInput.errors)
+        const language = chooseLanguage(handler.event, options.defaultLanguage)


the default language can now be configured with options.defaultLanguage

vladholubiev · 2018-04-25T21:02:49Z

src/middlewares/validator.js

-const defaults = {v5: true, $data: true, allErrors: true}
+const defaults = {
+  v5: true,
+  coerceTypes: 'array', // important for query string params


Hmm, isn't this a breaking change? coerceTypes is false by default

From a code standpoint, yes. From an end-user standpoint, no. Since this middleware is paired with a json schema, having coerceTypes enabled would make requests more forgiving when a body is passed in and enable the ability to parse query strings making them more usable. There are no cases where existing code would fail where they once passed. For example; if someone is using the current defaults their schema would have "type":"string" for all their query string params, in this case, no coercion would happen. coerceTypes is only applied if the types don't match. I added this option to support the use case when you expect an array from the query string, but only one item was passed in.

I feel including this as our default will be doing a service to younger developers, allowing their APIs to be more forgiving out of the gate. However, I do understand why if you'd like to default to something more strict. I'm good either way due to I'm always passing in custom options anyway. Let me know which way you want to go.

The background behind coerceTypes: 'array' can be found here: ajv-validator/ajv#158

For example; if someone is using the current defaults their schema would have "type":"string" for all their query string params, in this case, no coercion would happen.

Totally makes sense for me. And thanks for adding this, I see how it will be useful in my code already!

@vladgolubev Along these lines, you may also be interested in this PR: ajv-validator/ajv-keywords#64
I just had a new keyword added, transform, that allows to you to do some sanitization on strings being passed in. We're drafting up next steps for it here: ajv-validator/ajv-keywords#66

willfarrell · 2018-04-26T12:54:13Z

@lmammino I will have time Friday or Saturday to review your suggested changes. At first glance, they look good.

willfarrell · 2018-04-27T16:35:36Z

@lmammino Looks good! The last thing we need is to update the README and I think this PR is good to go. Let me know if you'd like me to assist with that.

lmammino · 2018-04-30T12:54:13Z

@willfarrell well done. I will try to get this one completed and merged tonight 😉

willfarrell added 2 commits April 22, 2018 12:19

feat: add in i18n support for validator

d0a8f96

Also includes the ability to customize error format

fix: add tests to cover i18n and errorFormat

6713689

- revert how error was returned to pass tests - lint code

lmammino requested review from lmammino, vladholubiev and dkatavic April 22, 2018 18:52

lmammino assigned willfarrell Apr 22, 2018

lmammino added enhancement feature request labels Apr 22, 2018

lmammino requested changes Apr 22, 2018

View reviewed changes

willfarrell added 3 commits April 22, 2018 17:26

fix: pr change requests

26c0775

fix: remove errorFormat

6586d1b

chore: lint and update tests

781030b

lmammino reviewed Apr 24, 2018

View reviewed changes

lmammino added 2 commits April 24, 2018 22:08

Merge branch 'master' of https://github.com/middyjs/middy into featur…

9d1a4a5

…e/validator_i18n

Proposed alternative implementation that assumes you are using this i…

a296341

…n combination with the content-negotiation middleware

lmammino reviewed Apr 24, 2018

View reviewed changes

vladholubiev reviewed Apr 25, 2018

View reviewed changes

lmammino added 2 commits April 30, 2018 20:04

Updated documentation

5316c67

version bump

a6e97bd

lmammino approved these changes Apr 30, 2018

View reviewed changes

lmammino merged commit 1874cfa into middyjs:master Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add in i18n support for validator #161

feat: add in i18n support for validator #161

willfarrell commented Apr 22, 2018 •

edited

Loading

codecov bot commented Apr 22, 2018 •

edited

Loading

lmammino left a comment

lmammino Apr 22, 2018

willfarrell Apr 22, 2018

lmammino Apr 22, 2018

willfarrell Apr 22, 2018

lmammino Apr 22, 2018

willfarrell Apr 22, 2018 •

edited

Loading

lmammino Apr 22, 2018

willfarrell Apr 22, 2018 •

edited

Loading

willfarrell commented Apr 22, 2018

lmammino left a comment •

edited

Loading

lmammino Apr 24, 2018

lmammino commented Apr 24, 2018 •

edited

Loading

lmammino Apr 24, 2018 •

edited

Loading

lmammino Apr 24, 2018 •

edited

Loading

lmammino Apr 24, 2018

vladholubiev Apr 25, 2018

willfarrell Apr 26, 2018 •

edited

Loading

vladholubiev Apr 29, 2018

willfarrell Apr 30, 2018

willfarrell commented Apr 26, 2018

willfarrell commented Apr 27, 2018

lmammino commented Apr 30, 2018

feat: add in i18n support for validator #161

feat: add in i18n support for validator #161

Conversation

willfarrell commented Apr 22, 2018 • edited Loading

codecov bot commented Apr 22, 2018 • edited Loading

Codecov Report

lmammino 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

willfarrell Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willfarrell Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

willfarrell commented Apr 22, 2018

lmammino left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmammino commented Apr 24, 2018 • edited Loading

lmammino Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

lmammino Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willfarrell Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willfarrell commented Apr 26, 2018

willfarrell commented Apr 27, 2018

lmammino commented Apr 30, 2018

willfarrell commented Apr 22, 2018 •

edited

Loading

codecov bot commented Apr 22, 2018 •

edited

Loading

willfarrell Apr 22, 2018 •

edited

Loading

willfarrell Apr 22, 2018 •

edited

Loading

lmammino left a comment •

edited

Loading

lmammino commented Apr 24, 2018 •

edited

Loading

lmammino Apr 24, 2018 •

edited

Loading

lmammino Apr 24, 2018 •

edited

Loading

willfarrell Apr 26, 2018 •

edited

Loading