feat: add type coercion #1370

jannyHou · 2018-05-29T03:45:45Z

connect to #750

This is the first PR for story #750 that

Implements a coercion function called in parseOperationArgs that converts the http raw data into a JS type data according to its OpenAPI schema
Implements helper functions to easily create tests for more edge cases
Adds unit test + acceptance test
More PRs for edge cases + validation rules are coming next.

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

bajtos

Please don't start yet another type coercion implementation. As was mentioned in #750, we should leverage strong-remoting or AJV to perform the coercion.

Note that we already have some type coercion code in @loopback/respository here, but a type system is out of scope of ~~this work~~ #750 - see #1319.

Since we are using OpenAPI spec to describe parameters and OpenAPI spec is mostly the same as JSON Schema, I think we should start by exploring the ways how to leverage AJV, because it promises the least amount of code to write to support more features that strong-remoting supports ATM, especially when it comes to advanced validation.

shimks · 2018-05-29T14:30:53Z

@bajtos I thought we had agreed on coming up with our own implementation of coercion from the result of #1057. I believe the reason had to do with having complete control over our LoopBack type deserializer as well as its interaction between the validator.

jannyHou · 2018-05-29T16:02:03Z

@bajtos Issue #750 focuses on "primitive type coercion", so I didn't use AJV, I thought it's just coercions from string to a JS run-time type for those OpenAPI 3.0.0 primitive types. My understanding is AJV is used for coercion/validating complex schemas.

@bajtos @shimks I think people have different opinions on the implementation, maybe let's clarify the issue from the perspective of expected behaviours first:

Despite the module we leverage to do the coercion, are my test cases valid and on the right track? Are they the coercion we aim to accomplish in issue #750 ?

raymondfeng · 2018-05-29T16:26:46Z

FYI: https://github.com/epoberezkin/ajv/blob/master/COERCION.md. I don't think the built-in coercion from ajv can deal with OpenAPI types (especially around format).

bajtos · 2018-05-30T08:46:38Z

I see. Looks like I have somehow missed the decision to build our own type coercion. Considering Raymond's comment about the limitations of AJV's built-in coercion, it makes sense to not use AJV.

Sorry for the confusion I created!

Let's discuss the best plan for rolling our own coercion then. I have few concerns:

How to limit the amount of rework & code churn

Eventually, we would like to leverage @loopback/types in the REST layer. However, that module is not available yet (see #947 and #1319). I am concerned that we may end up with an implementation in REST that will be difficult to port to @loopback/types and as a result we will throw away a lot of code.

At minimum, I think we should design the implementation and tests for conversion for various types to be decoupled from parseParams action as much as possible.

Correct handling of edge cases & good test coverage

I consider a comprehensive test suite that's easy to read & maintain as even more important than implementation details. When I was reworking coercion algorithm in strong-remoting 3.x, I wrote a very extensive test suite [1] that is unfortunately not that easy to understand and maintain. I think we should preserve the broad coverage of that existing test suite, but design the tests in a better way for LB4.

Incremental development

Let's work incrementally in vertical baby steps please, it will make pull requests smaller, which means they will be easier to review and faster to get landed.

I am proposing to start with a single type number and figure out all aspects of coercion:

Interaction between parseParams action and the coercion implementation
Testing strategy - what to test via unit tests, what to test via integration tests
Treatment of string-sources (e.g. query string, HTTP headers) and typed-source (JSON body), assuming this task is going to cover parameters read from JSON body. For example, assuming count parameter of type number: when a string value "42" was received from a query string, we want to convert it to a number 42. BUT: when a string valud "42" was received from JSON {"count": "42"}, we should reject this value as invalid.
Design of tests - using strong-remoting test suite [1] as a base, we will need to write about 1000 test cases, where most test cases are the same behavior-wise, but differ by their parameters. To keep such large test suite maintainable, we need to think hard about a design that will allow us to minimize the amount of code needed for each test case.
How to make tests decoupled from the fact that they are living inside REST repository now. Ideally, we should not need to completely rewrite 1000 tests when moving coercion to @loopback/types, instead it should be enough to update a small number of shared helper functions.

Once we figure out how to touch all layers, adding support for other types becomes an easy task that can be even spread out among multiple people.

[1] Link to the existing test suite I mentioned several times in my comment: https://github.com/strongloop/strong-remoting/tree/master/test/rest-coercion.

This comment is no longer valid.

jannyHou · 2018-05-30T13:36:47Z

No worries at all, any opinions are welcomed 👍

At minimum, I think we should design the implementation and tests for conversion for various types to be decoupled from parseParams action as much as possible.

@bajtos Do you mean implement the coercion as a separate action?
Yesterday when I discussed with @shimks , I am aware that we plan to have two steps for the type coercion:

http layer-->JS run-time
JS run-time-->LB types.
I am still finding the original discussion related, while based on that design, this PR only focuses on the first step, I also tend to implement the second step as a separate action, but hesitate on the first one. IMO there could be two approaches:
as what I did now: do the type coercion inside buildOperationArguments function
do the type coercion according to the parameter spec after generate the input array

And another thing I would like to confirm: IIRC this PR(#750 ) only does the coercion for parameters NOT requestBody right?

jannyHou · 2018-05-30T13:55:31Z

packages/rest/test/unit/parser-type-coercion.unit.ts

+  parseOperationArgs,
+} from '../..';
+
+describe('operationArgsParser', () => {


add acceptance test + refactor the todo example test

bajtos · 2018-05-30T14:25:06Z

One thing I really like about this pull request: it allows us to keep almost all tests at unit-test level 👍

@jannyHou and I had a quick call to discuss this pull request, the scope and the direction.

According to #750, the scope of this work is to coerce parameters coming from string sources only (query string, path and request headers), therefore the part of my previous comment regarding typed (JSON) source is not relevant.

In that light, I think the changes presented in this pull request are reasonable and we may take a different approach to incremental steps - start with a wide implementation that covers multiple types but does not handle all edge cases - at least that's how this pull request seems to me. I don't mind either way as long as we can iterate quickly and avoid a pull request that will take weeks to land.

Regarding the test suite, I still think it's crucial to simplify the test code and remove as much redundancy as possible. The test suite in strong-remoting is doing a good job at that, but at the cost of difficult troubleshooting - when a test fails, it produces very descriptive error message, but does not point to the source code line where the failing test case was defined. It would be great if we could fix that in LB4.

Here is a mockup showing some ideas to explore:

describe('path parameters', () => {
  describe('number type', () => {
    const def: ParameterObject = {name: 'p', type: 'number', in: 'path'};

    // this is rejected by tsc: Expected 1-2 arguments, but got 0 or more.
    // it(...verifyParamCoercion(def, '0', 0));

    it.apply(it, verifyParamCoercion(def, '0', 0));
    it.apply(it, verifyParamCoercion(def, '-1', -1));
    it.apply(
      it,
      verifyParamCoercion(def, '123456789123456789123435678', ERROR),
    );
  });
});

function verifyParamCoercion<T>(
  def: ParameterObject,
  input: string,
  expectedOutput: T,
): [string, Function] {
  // in case Mocha does not print the line where `it()` was called,
  // but only a stack trace starting with `verifyParamCoercion`
  const lineThatCalledUs = new Error().stack!.split(/\n/)[1];
  
  const prettyInput = JSON.stringify(input);
  return [
    `converts ${prettyInput} to ${expectedOutput} (see ${lineThatCalledUs})`,
    function testCoercionCase() {
      // mocha test
      // expect(something)
    },
  ];
}

Besides the comprehensive unit-test suite covering all different cases, we were talking with @jannyHou about writing three acceptance tests to verify how individual building blocks fit together. One test case for each input source (query string, path, headers).

bajtos · 2018-05-30T14:30:51Z

http layer-->JS run-time
JS run-time-->LB types.

IMO, there should be no distinction between JS primitive types and LB types. A number will always be represented as number at runtime. The difference is in the additional information we have. In JS, there is only a single type number to hold all numeric values. In LB (and OpenAPI Spec too), we distinguish between number, integer, etc.

I agree this PR should deal with conversion from the HTTP layer (strings received from Express Request object) into JavaScript primitives.

I am still finding the original discussion related, while based on that design, this PR only focuses on the first step, I also tend to implement the second step as a separate action, but hesitate on the first one. IMO there could be two approaches:

as what I did now: do the type coercion inside buildOperationArguments function

do the type coercion according to the parameter spec after generate the input array

My (strong) opinion is that buildOperationArguments is responsible for returning an array of values that are matching the types and constraints described by OpenAPI ParameterObjects. Any coercion and/or validation needed to achieve that outcome should remain as an internal implementation detail of buildOperationArguments.

And another thing I would like to confirm: IIRC this PR(#750 ) only does the coercion for parameters NOT requestBody right?

I think so.

bajtos · 2018-05-30T14:40:21Z

packages/rest/src/parser.ts

@@ -103,13 +105,14 @@ function buildOperationArguments(
    const spec = paramSpec as ParameterObject;
    switch (spec.in) {
      case 'query':
-        paramArgs.push(request.query[spec.name]);
+        paramArgs.push(paramCoerce(request.query[spec.name], spec.schema));


The bit paramArgs.push(paramCoerce is duplicated many times and it's getting rather long. Could you please clean up this code to avoid repetition please?

I think we should move the switch (spec.in) statement to a new function that will return the value from the request, and then keep paramArgs.push(paramCoerce) in a single place here.

const spec = paramSpec as ParameterObject; const rawValue = getParamFromRequest(request, spec.in); const value = paramCoerce(rawValue, spec.schema); paramArgs.push(value);

bajtos · 2018-05-30T14:40:45Z

packages/rest/src/parser.ts

@@ -122,3 +125,50 @@ function buildOperationArguments(
  if (requestBodyIndex > -1) paramArgs.splice(requestBodyIndex, 0, body);
  return paramArgs;
 }
+
+export function paramCoerce(data: string, schema?: SchemaObject | ReferenceObject): any {


Let's move this function to a new file please. I am expecting it to grow in size as we will deal with edge cases.

Rename it to coerceParam?

+1 for coerceParam, I would even call it coerceParameter. It's our convention for function/method names to start with a verb.

shimks

Looks good!

shimks · 2018-06-01T16:10:44Z

packages/rest/src/coercion/coerce-parameter.ts

+ */
+
+function isTrue(data: string): boolean {
+  const isTrueSet = ['true', '1', true, 1];


are 1, true, '1' reachable here? Or is this implemented for the purpose of using @loopback/types later on?

Can we make boolean coercion more strict please and reject unknown values (throw an error)?

For example, both values "TRUE" and "ok" are interpreted as false right now, which is very confusing!

IMO, we should ignore case here and treat TRUE the same way as true.

I am proposing to reject any value that's not true, false, 1, or 0.

An edge case to figure out: how to treat an empty string? I think we should convert it to undefined and let validation handle it later.

This is the behavior we have in LB 3.x, see https://github.com/strongloop/strong-remoting/blob/032e51e1f5bf69a91bc1b89ec73f734a26c29ad2/test/rest-coercion/urlencoded-boolean.suite.js#L42-L45

@shimks @bajtos This PR would not be the full implementation for boolean type coercion, that's why I didn't write all test cases and handle the validations. I think we agree to create PRs incrementally add edge cases.

from @bajtos

I think the changes presented in this pull request are reasonable and we may take a different approach to incremental steps - start with a wide implementation that covers multiple types but does not handle all edge cases - at least that's how this pull request seems to me. I don't mind either way as long as we can iterate quickly and avoid a pull request that will take weeks to land.

And this PR won't be the end of story #750, the story includes all edge cases for all types we support 💪.

I think what I miss is @bajtos 's comment in #1370 (comment), I should pick one type and

start with a single type number and figure out all aspects of coercion

Will do it in the next commit.

@shimks @bajtos FYI I will track the coming PRs in #750 (comment)

@jannyHou sounds good! I am fine to leave out edge cases from this PR as long as we are keeping track of missing items (a todo list).

Could you please move the todo list from a comment in #750 to the issue description at the top of #750?

raymondfeng · 2018-06-01T22:12:52Z

packages/rest/src/coercion/coerce-parameter.ts

+    case 'password':
+    // serizlize will be supported in next PR
+    case 'serialize':
+      break;


Typo: serizlize -> serialize.

jannyHou · 2018-06-06T19:07:13Z

packages/rest/test/unit/coercion/utils.ts

+}
+
+// tslint:disable-next-line:no-any
+export function test(t: any[]) {


A screenshot of the error report:

It contains the caller's information in stack.

It contains the caller's information in stack.

Awesome!

Let's use properly typed arguments instead of any[] array please.

Also can we please generate a descriptive test name showing the input value and the expected output, removing t[0] as the test name along the way? Most test cases are setting t[0] to the same value as t[2], which is unnecessary duplication to me.

Let's use properly typed arguments instead of any[] array please.

Here is what I mean:

function test<T>(spec: SchemaObject, input: string, expectedOutput: T);

If you think there are test cases where a hand-written test name is better than the auto-generated one (converts ${input} to ${expectedOutput}), then I am proposing to add a new optional parameter for test case name at the end of the arguments list.

function test<T>(spec: SchemaObject, input: string, expectedOutput: T, testName?: string);

Thoughts?

raymondfeng · 2018-06-06T19:46:46Z

packages/rest/src/coercion/coerce-parameter.ts

+    debug(
+      'The parameter with schema %s is not coerced since schema' +
+        'dereferrence is not supported yet.',
+      schema,


We should use https://github.com/BigstickCarpet/swagger-parser now as it supports both Swagger 2.0 and OAS 3.0.

That's cool!

Let's leave integration with swagger-parser out of scope of this pull request though.

@raymondfeng Good point to use the swagger-parser 👍 while I think the validation for whole OpenAPI spec should happen before we parse the parameter, which means if the generated OpenAPI spec is invalid, the app shouldn't be able to start at all.
So the default clause here is actually infeasible.
I am going to remove the error throw.

raymondfeng · 2018-06-06T19:51:23Z

packages/rest/src/coercion/coerce-parameter.ts

+ */
+function isTrue(data: string): boolean {
+  const isTrueSet = ['true', '1'];
+  return isTrueSet.includes(data);


We should promote isTrueSet outside of the function or simply use ['true', '1'].includes(data).

shimks · 2018-06-06T21:03:09Z

packages/rest/src/coercion/validator.ts

+ * Validator class provides a bunch of functions that perform
+ * validations on the request parameters and request body.
+ */
+export class Validator {


I thought we were using AJV for the validation portion of the epic. Is this catered for interweaving coercion and validation together?

I think I may get a better understanding if I was to see how the code here would be used.

My initial thought:

Can we leave Validation out of scope of this initial pull request please?

As I understand our plan, the scope of #750 is only coercion. Validation will be covered by #118.

Even if we think that #750 should cover basic validation, I would prefer to keep this initial pull request as small as possible so that we can land it sooner.

On the second though, it's difficult to implement coercion without basic validation, because we need some way to handle string values that cannot be converted to the target type.

Would it make sense to (temporarily?) simplify this part and let coerceParameter throw an error when the string value cannot be converted to the target type? My idea is to reject only values that cannot be converted to target type. Any validation rules beyond that would be left for the validation framework to implement and handle.

As for required flag, I think that's an example of a validation rule that's out of scope of coercion. When the request does not provide any value for the argument (or provides an empty string), the coercion should return undefined and let the rest of the framework to take care of that.

Thoughts?

A code snippet to illustrate what I meant.

Now:

case 'number': validator.validateParamBeforeCoercion('number', data); coercedResult = data ? Number(data) : undefined; validator.validateParamAfterCoercion('number', coercedResult);

My proposal:

// handle empty string for all types at the top case 'number': // we can assume "data" is not an empty string here coercedResult = Number(data); if (isNaN(coercedResult)) { // report coercion error - data is not a number }

@bajtos I think the problem is it's not possible to ignore the requirement check if I add all edge cases for a type.
AJV only validates the schema spec, but not the parameter spec, which means it's still our responsibility to do the check for required.

Would it make sense to (temporarily?) simplify this part and let coerceParameter throw an error when the string value cannot be converted to the target type? My idea is to reject only values that cannot be converted to target type.

That's the tricky part...required affects how we define "values that cannot be converted to target type", by saying that, I mean:

required: empty string cannot be converted

not required: empty string can be converted

IMO, requirement check should be part of the basic validation.

// handle empty string for all types at the top case 'number': // we can assume "data" is not an empty string here coercedResult = Number(data); if (isNaN(coercedResult)) { // report coercion error - data is not a number }

^ That looks good to me within this PR 👍While I still want to apply those checking functions through a validator class. The reason why I define the validator(does basic validation) as a class is

It's easy to extend

easy to share the context by a class constructor

I think the problem is it's not possible to ignore the requirement check if I add all edge cases for a type.
AJV only validates the schema spec, but not the parameter spec, which means it's still our responsibility to do the check for required.

Good point about AJV capabilities. Even if we don't use AJV to validate parameter spec, my opinion is that validation is out of scope of #750.

That's the tricky part...required affects how we define "values that cannot be converted to target type", by saying that, I mean:

required: empty string cannot be converted
not required: empty string can be converted
IMO, requirement check should be part of the basic validation.

I have a different view. When the spec says that a parameter is of type number, I consider undefined as a valid value signaling there was no parameter value provided by the client. Whether a parameter can be undefined (is optional vs. required) is another matter that can be handled by different piece of code.

Having said that, I have re-read the Parameter Object from OpenAPI-Specification and the way how to spec is laid out, I agree with you it makes sense to handle required flag as part of parameter coercion.

shimks · 2018-06-06T21:16:59Z

packages/rest/test/acceptance/coercion/coercion.acceptance.ts

+
+  it('coerces parameter in header from string to number', async () => {
+    const spy = sinon.spy(MyController.prototype, 'createNumberFromHeader');
+    await client.get('/create-number-from-header').set({num: 100});


The parameter here should be in string if I understand how http headers work correctly

I believe the HTTP client we are using (supertest/superagent) has to convert the value to string during transport, so it should not really matter whether we use a number or a string here.

Having said that, I don't mind either way.

@shimks

I believe the HTTP client we are using (supertest/superagent) has to convert the value to string during transport,

Yep, see this edge case for type number, there is a conversion done by http client before the original query data reaches our paramParser, and that's why whatever data type you provide in the query/path/header, the paramParser always receive it as a string.

I use a number type in this test to mock a real use case. The unit tests are written in a way that the raw data is always a string.

shimks · 2018-06-06T21:17:07Z

packages/rest/test/acceptance/coercion/coercion.acceptance.ts

+    const spy = sinon.spy(MyController.prototype, 'createNumberFromQuery');
+    await client
+      .get('/create-number-from-query')
+      .query({num: 100})


same as above

shimks · 2018-06-06T21:19:04Z

packages/rest/test/unit/coercion/paramStringToBoolean.unit.ts

+  /*tslint:disable:max-line-length*/
+  test(['false', BOOLEAN_SCHEMA, 'false', false]);
+  test(['true', BOOLEAN_SCHEMA, 'true', true]);
+  test(['undefined', BOOLEAN_SCHEMA, undefined, undefined]);


Don't we need another test case for random strings (exception path)? Same thing for other type coercions based on different schemas

@shimks that will be in the coming PR :) see list in https://github.com/strongloop/loopback-next/pull/1370/files#diff-66e4e0e78968649919599eb83a809326R62

bajtos

Good progress, I like how the new test design allows us to group test cases to context blocks 👍

I have few more suggestions how to improve and simplify this further, see below.

bajtos · 2018-06-07T06:52:18Z

packages/rest/src/coercion/coerce-parameter.ts

+    switch (format) {
+      case 'byte':
+        OAIType = 'byte';
+        break;


Can you simplify this method by returning immediately as you determine the type (return value), instead of using a mutable variable?

function getOAIPrimitiveType(type?: string, format?: string) { if (type === 'object' || type === 'array') return 'serialize'; if (type === 'string') { switch (format) { case: 'byte': return 'byte'; // ... } } // etc. return 'unknownType'; }

I think it's also better to return undefined instead of a magic string unknownType.

bajtos · 2018-06-07T06:55:51Z

packages/rest/src/coercion/validator.ts

+ * Validator class provides a bunch of functions that perform
+ * validations on the request parameters and request body.
+ */
+export class Validator {


My initial thought:

Can we leave Validation out of scope of this initial pull request please?

As I understand our plan, the scope of #750 is only coercion. Validation will be covered by #118.

Even if we think that #750 should cover basic validation, I would prefer to keep this initial pull request as small as possible so that we can land it sooner.

On the second though, it's difficult to implement coercion without basic validation, because we need some way to handle string values that cannot be converted to the target type.

Would it make sense to (temporarily?) simplify this part and let coerceParameter throw an error when the string value cannot be converted to the target type? My idea is to reject only values that cannot be converted to target type. Any validation rules beyond that would be left for the validation framework to implement and handle.

As for required flag, I think that's an example of a validation rule that's out of scope of coercion. When the request does not provide any value for the argument (or provides an empty string), the coercion should return undefined and let the rest of the framework to take care of that.

Thoughts?

bajtos · 2018-06-07T06:59:16Z

packages/rest/test/unit/coercion/utils.ts

+}
+
+// tslint:disable-next-line:no-any
+export function test(t: any[]) {


It contains the caller's information in stack.

Awesome!

Let's use properly typed arguments instead of any[] array please.

Also can we please generate a descriptive test name showing the input value and the expected output, removing t[0] as the test name along the way? Most test cases are setting t[0] to the same value as t[2], which is unnecessary duplication to me.

jannyHou · 2018-06-07T23:32:39Z

@raymondfeng @shimks @bajtos thanks for all the feedback!
Applied them in commit 6
And improved error message in commit 8

@bajtos tends to simplify the basic validations, I don't have very strong opinion on that since it's still early to determine a good abstract for them. I explained why I abstract the validator as a class in comment, and commit 7 simplified the check for invalid parameter as @bajtos suggested. More opinions are welcomed:

if a validator class is good, I can revert commit 7
if the simplified check looks good, then keep commit 7 as it is
if we decide to abandon the validator class due to its complexity, I can create another commit to simplify the required check like commit 7

Which options among the ^ 3 ones would you like?

bajtos

To move this work forward, I'll try to focus on important and/or higher-level issues and leave implementation details up to you, these details are often subjective.

I don't have a strong opinion about the Validator class, I think we don't yet have enough knowledge to be able to come up with a good design that won't change later on. There isn't much code in Validator yet, it will be easy to refactor and/or move it around if needed. I am ok with keeping it.

bajtos · 2018-06-08T06:58:38Z

packages/rest/src/coercion/coerce-parameter.ts

-      validator.validateParamAfterCoercion('number', coercedResult);
+      if (coercedResult === undefined) break;
+      if (isNaN(coercedResult))
+        throw new HttpErrors['400'](


Let's use a named constructor please, see https://www.npmjs.com/package/http-errors#list-of-all-constructors

throw new HttpErrors.BadRequest(...)

bajtos · 2018-06-08T07:00:29Z

packages/rest/src/coercion/coerce-parameter.ts

-          schema.format
-        } is not a valid OpenAPI schema`,
-      );
+      break;
  }
  return coercedResult;


I think you are missing call of validator.validateParamAfterCoercion(data)?

If that's not needed, then I would prefer to see all break statements replaced with return coercedResult. The next refactoring is to get rid of let coercedResult entirely.

@bajtos I only applied the isNaN(coercedResult) to type number in this PR, that's why it's only called for case 'number'

The invalid value check will definitely be added to other types, I didn't apply isNaN() for other types since I feel "is not a number" only represents invalid number but not other types...

bajtos · 2018-06-08T07:02:46Z

packages/rest/src/coercion/validator.ts

    opts?: ValidationOptions,
  ) {
    if (this.isAbsent(value)) {
      if (this.isRequired(opts)) {
-        throw new HttpErrors['400']();
+        const name = this.ctx.parameterSpec.name;
+        throw new HttpErrors['400'](HttpErrorMessage.MISSING_REQUIRED(name));


Ditto, use new HttpErrors.BadRequest(...)

bajtos · 2018-06-08T07:04:35Z

packages/rest/src/coercion/validator.ts

-  validateNumber(value: any) {
-    if (value === undefined) return;
-    if (isNaN(value)) throw new HttpErrors['400']();
+    return [''].includes(value);


I find this weirdly complex, value === '' is easier to comprehend and faster at runtime.

Should we handle the case where the value was not provided at all (e.g. the request header is missing)?

return value == null || value === '';

@bajtos After the raw data converted by http client, it's always '', see test case https://github.com/strongloop/loopback-next/pull/1370/files#diff-66e4e0e78968649919599eb83a809326R51, so null and '' are all converted to ''.

value === '' is easier to comprehend and faster at runtime.

👍

I think your test case is not good enough. I am talking about the following situation:

A parameter called X-Correlation-Id is defined as {in: 'header', schema: {type: 'number'}}

The HTTP request does not contain any X-Correlation-Id header.

Please add a test for this edge case and make sure it's handled correctly.

I am fine with leaving this test case out of scope of this initial pull request as long as it's done as part of the user story you are working on.

bajtos · 2018-06-08T07:05:23Z

packages/rest/test/unit/coercion/paramStringToBoolean.unit.ts

-  test(['false', BOOLEAN_SCHEMA, 'false', false]);
-  test(['true', BOOLEAN_SCHEMA, 'true', true]);
-  test(['undefined', BOOLEAN_SCHEMA, undefined, undefined]);
+  test<boolean>(BOOLEAN_PARAM, 'false', false);


Is it necessary to provide <boolean>? I would expect TypeScript compiler to automatically infer the generic type from the last (third) argument. Same comment applies to all other similar tests.

good catch! 😓

bajtos · 2018-06-08T07:11:25Z

packages/rest/test/unit/coercion/paramStringToNumber.unit.ts

-      ERROR_BAD_REQUEST,
-      true,
-    ]);
+      new HttpErrors['400'](errorMsg),


+1 for extracting the error message construction to a shared helper.

I am proposing to go one step further and move the creation of HttpError instances to the helper too.

Before:

const errorMsg = HttpErrorMessage.INVALID_DATA('text', NUMBER_PARAM.name); new HttpErrors.BadRequest(errorMsg);

After:

RestCoercionErrors.invalidData('text', NUMBER_PARAM.name);

Implementation:

export namespace RestCoercionErrors export function invalidData<T>(data: T, name: string) => { const msg = `Invalid value ${JSON.stringify(data)} for parameter ${name}!`; return new HttpErrors.BadRequest(msg); } // etc. }

Thoughts?

@bajtos Originally I was thinking of creating templates to unify the message for each kind of http error, not only for coercion functions, that's why I name it as http-error-message.ts. I should have moved the file one level up to be under src.

It makes more sense that one error message maps to one HttpError type, so your approach is better 👍

bajtos · 2018-06-08T07:13:08Z

packages/rest/test/unit/coercion/utils.ts

  const caller: string = new Error().stack!;
-  it(t[0] as string, async () => {
+  const DEFAULT_TEST_NAME = `convert request raw value ${rawValue} to ${expectedResult}`;


converts

(it converts request raw value ...)

bajtos · 2018-06-08T07:13:46Z

packages/rest/test/unit/coercion/utils.ts

-      expectedResult: t[3],
+      paramSpec,
+      rawValue,
+      expectedResult,
      caller,


Do we still need to pass caller? I thought you wrote the original stack trace already points to the actual test case?

@bajtos Yep we still need it. function test is called directly in a test file, so new Error().stack shows the caller.
test calls the testCoercion and wraps it with it('test name', () => { testCoercion()}), inside function testCoercion the error stack doesn't know the caller in a test file, so it still relies test passes the caller to it.

shimks

Nice work!

bajtos

LGTM, please consider addressing the comments below. No further review is necessary as far as I am concerned.

bajtos · 2018-06-12T07:34:54Z

packages/rest/src/coercion/coerce-parameter.ts

+      const coercedData = data ? Number(data) : undefined;
+      if (coercedData === undefined) return;
+      if (isNaN(coercedData))
+        throw HttpErrorMessage.invalidData(data, spec.name);


I find HttpErrorMessage misleading, the namespace is not for error messages anymore. How about CommonHttpErrors or RestHttpErrors?

Maybe RestErrors would be best?

Hmm..I prefer the name RestHttpErrors

bajtos · 2018-06-12T07:37:21Z

packages/rest/test/unit/coercion/paramStringToNumber.unit.ts

+
+  context('empty values trigger ERROR_BAD_REQUEST', () => {
+    // null, '' sent from request are converted to raw value ''
+    test<HttpErrors.HttpError>(


Is this explicit type required? Cannot TypeScript infer the type for us?

oops, forgot to remove it.

bajtos · 2018-06-12T07:38:53Z

packages/rest/test/unit/coercion/utils.ts

+        throw new Error("'parseOperationArgs' should throw error!");
+      } catch (err) {
+        expect(err).to.eql(config.expectedResult);
+      }


I think this block could be simplified using rejectedWith.

if (config.expectError) { expect(parseOperationArgs(req, route)) .to.be.rejectedWith(config.expectedResult); }

Not a big deal though, if my proposal does not work then I am ok with your current solution too.

@bajtos Thanks for the suggestion, I have also tried a lot of approaches to test the error for an async function, including rejectedWith, but none of them work, therefore try{...}catch(err){...} is used here.

E.g. by using rejectedWith:

expect(await parseOperationArgs(req, route)).to.be.rejectedWith(config.expectedResult);

It fails the test directly instead of compare the error message. While I would definitely like to try more to simplify the error msg test in the coming PRs, if time allows.

It's important to pass the promise returned by parseOperationArgs directly to expect and don't await it!

Your code which does not work:

expect(await parseOperationArgs(/*...*/)).to.be.rejectedWith(/*...*/);

Correct usage - let expect() handle the promise returned by parseOperationArgs, await the result of expect call:

await expect(parseOperationArgs(/*...*/)).to.be.rejectedWith(/*...*/);

A more verbose alternative:

expect(async () => { await parseOperationArgs(/*...*/); }) .to.be.rejectedWith(/*...*/); // a sync version for better understanding of the principle expect(() => { doSomethingSync(/*...*/); }) .to.throw(/*...*/);

bajtos · 2018-06-12T07:45:23Z

Your new code has insufficient test coverage compared to our current high standards. Please review coverage data and consider removing implementation bits that are not covered by tests, so that you can add them "properly" with tests in follow-up pull requests.

/packages/rest/src/coercion/validator.ts - test coverage 59%
/packages/rest/src/coercion/coerce-parameter.ts - test coverage 76%

Last but not least, please make sure that your code and commit messages are correctly formatted, Travis CI is currently failing in code-lint and commit-lint tasks.

jannyHou · 2018-06-12T17:49:56Z

@bajtos I applied most of the feedback and reduced the test coverage decrease from 0.4 to 0.02. 'binary', 'date-time' and 'password' will be handled in the coming PR, which will cover that 0.02 decrease.

b-admike · 2018-06-12T19:46:17Z

packages/rest/src/coercion/coerce-parameter.ts

+  if (!schema || isReferenceObject(schema)) {
+    debug(
+      'The parameter with schema %s is not coerced since schema' +
+        'dereferrence is not supported yet.',


typo: dereferrence -> dereference

good catch, fixed in f38b44d

b-admike · 2018-06-12T19:51:05Z

packages/rest/src/coercion/coerce-parameter.ts

+ * @param format The format in an OpenAPI schema specification
+ */
+function getOAIPrimitiveType(type?: string, format?: string) {
+  // serizlize will be supported in next PR


typo: serizlize -> serialize

@b-admike You may review a wrong commit? In the latest code it's already fixed, actually it was fixed long time(1-2 weeks) ago lol

jannyHou requested review from bajtos and raymondfeng as code owners May 29, 2018 03:45

jannyHou force-pushed the coercion/http-and-lb4 branch from f881494 to ba19079 Compare May 29, 2018 03:46

bajtos previously requested changes May 29, 2018

View reviewed changes

jannyHou self-assigned this May 29, 2018

jannyHou commented May 30, 2018

View reviewed changes

bajtos reviewed May 30, 2018

View reviewed changes

dhmlau mentioned this pull request May 31, 2018

Monthly Milestone - June 2018 #1375

Closed

27 tasks

jannyHou force-pushed the coercion/http-and-lb4 branch from ba19079 to e6127f0 Compare May 31, 2018 18:10

jannyHou changed the title ~~[WIP]feat: add type coercion~~ [feedback WIP]feat: add type coercion May 31, 2018

jannyHou force-pushed the coercion/http-and-lb4 branch 4 times, most recently from c74b096 to 444d933 Compare June 1, 2018 15:16

jannyHou changed the title ~~[feedback WIP]feat: add type coercion~~ feat: add type coercion Jun 1, 2018

jannyHou force-pushed the coercion/http-and-lb4 branch 4 times, most recently from 71987b0 to e3cd666 Compare June 1, 2018 16:02

shimks approved these changes Jun 1, 2018

View reviewed changes

jannyHou changed the title ~~feat: add type coercion~~ [improving tests]feat: add type coercion Jun 1, 2018

raymondfeng reviewed Jun 1, 2018

View reviewed changes

jannyHou changed the title ~~[WIP]feat: add type coercion~~ feat: add type coercion Jun 6, 2018

jannyHou force-pushed the coercion/http-and-lb4 branch from 1d7abb1 to b68bfd8 Compare June 6, 2018 18:15

jannyHou commented Jun 6, 2018

View reviewed changes

raymondfeng reviewed Jun 6, 2018

View reviewed changes

shimks reviewed Jun 6, 2018

View reviewed changes

bajtos suggested changes Jun 7, 2018

View reviewed changes

bajtos mentioned this pull request Jun 7, 2018

[WIP] feat(rest): add basic parameter type conversion #941

Closed

6 tasks

jannyHou force-pushed the coercion/http-and-lb4 branch from 8d50758 to 05bb648 Compare June 7, 2018 23:19

bajtos reviewed Jun 8, 2018

View reviewed changes

jannyHou force-pushed the coercion/http-and-lb4 branch from 1578ecf to f59d45f Compare June 8, 2018 17:12

shimks approved these changes Jun 8, 2018

View reviewed changes

jannyHou force-pushed the coercion/http-and-lb4 branch from 6ecf912 to d7f1904 Compare June 8, 2018 18:55

raymondfeng mentioned this pull request Jun 11, 2018

Cannot override query parameter default value in controller method #1408

Closed

bajtos approved these changes Jun 12, 2018

View reviewed changes

jannyHou force-pushed the coercion/http-and-lb4 branch 4 times, most recently from 8424f3c to 7ae633a Compare June 12, 2018 17:46

b-admike reviewed Jun 12, 2018

View reviewed changes

jannyHou force-pushed the coercion/http-and-lb4 branch from 7ae633a to f38b44d Compare June 12, 2018 20:46

b-admike approved these changes Jun 12, 2018

View reviewed changes

jannyHou force-pushed the coercion/http-and-lb4 branch from f38b44d to c770417 Compare June 12, 2018 20:53

feat: add type coercion

c770417

jannyHou merged commit 2b8d816 into master Jun 13, 2018

jannyHou deleted the coercion/http-and-lb4 branch June 13, 2018 16:36

jannyHou mentioned this pull request Jun 19, 2018

feat: coercion for other types #1435

Merged

15 tasks

feat: add type coercion #1370

feat: add type coercion #1370

Conversation

jannyHou commented May 29, 2018 • edited Loading

Checklist

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

shimks commented May 29, 2018

jannyHou commented May 29, 2018 • edited Loading

raymondfeng commented May 29, 2018

bajtos commented May 30, 2018

jannyHou commented May 30, 2018

Choose a reason for hiding this comment

bajtos commented May 30, 2018

bajtos commented May 30, 2018

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

shimks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jun 4, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

raymondfeng Jun 6, 2018 • edited by jannyHou 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

jannyHou Jun 7, 2018 • 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

shimks Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos 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

jannyHou commented Jun 7, 2018 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Jun 8, 2018 • 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

jannyHou Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

shimks left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

jannyHou commented May 29, 2018 •

edited

Loading

bajtos left a comment •

edited

Loading

jannyHou commented May 29, 2018 •

edited

Loading

jannyHou Jun 4, 2018 •

edited

Loading

jannyHou Jun 7, 2018 •

edited

Loading

raymondfeng Jun 6, 2018 •

edited by jannyHou

Loading

jannyHou Jun 7, 2018 •

edited

Loading

shimks Jun 6, 2018 •

edited

Loading

jannyHou commented Jun 7, 2018 •

edited

Loading

jannyHou Jun 8, 2018 •

edited

Loading

jannyHou Jun 8, 2018 •

edited

Loading

jannyHou Jun 8, 2018 •

edited

Loading

jannyHou Jun 12, 2018 •

edited

Loading