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

[raymondfeng]

This PR fixes the console output from npm test:

WARNING: id property cannot be changed from 3 to 3 for model:Todo in 'before save' operation hook
WARNING: id property cannot be changed from 3 to 3 for model:Todo in 'loaded' operation hook

Checklist

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

[bajtos]

Cross-posting #947 (comment)

@raymondfeng could we please reduce the scope of this work to what's needed by our MVP? We have two similar stories in backlog, one is MVP the other is non-MVP:

• MVP: Router: validate input parameters Router: validate input parameters #118
• non-MVP: Router: coercion of complex parameters from string sources Router: coercion of complex parameters from string sources #100

As a team, we really need to focus on what's required by MVP and de-prioritize reviews of pull requests proposing changes out of scope of MVP. I don't mind to keep this pull request open as a long-term work, as long as our mutual expectation is that reviews will happen infrequently.

[jannyHou]

Hi @raymondfeng I am not sure how closely this PR is related to the swagger/openapi primitive types, in openapi3, they define data types as 3.0.0#data-types
And in my v3 PR, I add classes map to those primitive types (the code is a draft and still in progress at this moment), user can specify an openapi3 primitive type for a parameter by:

import {Integer, get} from '@loopback/openapi-v3';
class MyController {
  @get('/Foos')
  async function find(limit: Integer) {
     await getFoos({limit: limit});
  }
}

Then the v3 schema spec for that parameter is built as schema: {type: 'integer', format: 'int32'}

[raymondfeng]

@jannyHou The PR is for swagger 2.0 only. OpenAPI 3.0 probably needs to contribute its own parser/writer to convert from/to http values. We need to figure out how the parser/writer is plugged in.

[jannyHou]

@raymondfeng I see thanks, my pr contains the param/requestBody decorator upgrade, while I haven't investigated too much of the parser and writer. I will come up with some proposal and get the team involved for a discussion.

[raymondfeng]

Cross-posting #947 (comment)
@raymondfeng could we please reduce the scope of this work to what's needed by our MVP? We have two similar stories in backlog, one is MVP the other is non-MVP:
MVP: Router: validate input parameters #118
non-MVP: Router: coercion of complex parameters from string sources #100
As a team, we really need to focus on what's required by MVP and de-prioritize reviews of pull requests proposing changes out of scope of MVP. I don't mind to keep this pull request open as a long-term work, as long as our mutual expectation is that reviews will happen infrequently.

This PR fixes the console output from npm test:

WARNING: id property cannot be changed from 3 to 3 for model:Todo in 'before save' operation hook
WARNING: id property cannot be changed from 3 to 3 for model:Todo in 'loaded' operation hook

The corresponding test is https://github.com/strongloop/loopback-next/blob/master/packages/example-getting-started/test/acceptance/application.test.ts. The deep root cause is that our REST module fails to convert string to number and the legacy juggler warns that an update of id from 3 to "3" is not desired.

[bajtos]

This PR fixes the console output from npm test:

WARNING: id property cannot be changed from 3 to 3 for model:Todo in 'before save' operation hook
WARNING: id property cannot be changed from 3 to 3 for model:Todo in 'loaded' operation hook

@raymondfeng Could you please include such information in the pull request description in the future? It changes the importance of this pull request, at least to me.

IIRC, these warnings are printed when making HTTP requests to our example applications too, therefore I think we should look into fixing this problem soon.

[bajtos]

Handling all edge cases of type conversions and writing a good test suite that's easy to understand and maintain - that's non-trivial a very involved task. While I see a lot of value in converting parameters from strings to integers/dates/etc., I am concerned about the tech debt this code can quickly introduce.

If we are looking for a quick temporary solution (because we are moving to OpenAPI Spec v3 soon anyways), then I'd prefer to leverage type conversions provided by strong-remoting@3, see https://github.com/strongloop/strong-remoting/blob/02f6be30354a5a6c562359a7c6b05c383e6efe6e/lib/type-registry.js and usage here:

https://github.com/strongloop/strong-remoting/blob/d026cedb0f332627812560ddc662359d7ba58d6a/lib/http-context.js#L188-L194

If strong-remoting cannot be leveraged, or we don't want to, then I am proposing to defer this change after MVP and start with a spike exploring other options that will not require us to maintain our own implementation of OpenAPISpec-based coercion. For example, AJV's coercion may be good enough, especially considering that we are thinking about using AJV to validate complex types in JSON bodies (#118).

As a short-term solution to get rid of the warnings printed by npm test, I am willing to consider adding a very trimmed-down version of your deserializer, one that handles only string-to-number conversion.

[bajtos]

To my taste, this code is repeating the call of deserialize too much.

Please rework the code so that the switch block reads the argument value from the correct place in the request (query, pathParams, etc.) and then move calls of deserialize and args.push to a single place after the switch block.

[bajtos]

Please move all helper function to the bottom of the test file, see http://loopback.io/doc/en/contrib/style-guide.html#layout-of-test-files (the section addresses hook function, not helper functions, but the principle is the same).

[bajtos]

One logical assert per test please. This should be split into several tests grouped by describe block.

describe('number parameter', () => {
 const param: ParameterObject = {
     in: 'query',
     name: 'balance',
     type: 'number',
   };

  it('deserializes integer values', () => {
    expectToDeserialize(param, [0, 10, -10], [0, 10, -10]);
  });

  it('deserializes integer strings', () => {
    expectToDeserialize(param, ['0', '10', '-10'], ['0', '10', '-10']);
  });

  // etc.
});

The same comment applies to other new tests added below.

[bajtos]

@raymondfeng I have an alternative and much smaller patch to get rid of the warnings, while documenting limitations of our MVP scope - please take a look at #967.

I am labelling this PR as non-MVP because the task of implementing coercion was removed from MVP recently (see #750 (comment)).

[bajtos]

Closing as a duplicate of #1370

Feel free to reopen if you disagree.