Test rest coercion #304

bajtos · 2016-05-24T09:04:48Z

An integration test suite describing and verifying argument coercion in REST.

arguments in query string
arguments in JSON body (formdata)
arguments in url-encoded formdata
the JSON body as an argument

1230 passing (3s)

(see https://github.com/strongloop-internal/scrum-loopback/issues/706#issuecomment-217371058 for more information)

In this pull request, I am describing the current behaviour in the master branch. My intention is to run the test suite against different strong-remoting versions to find differences, I'll post results later.

In order to discover all breaking changes and document the new behaviour, we should write an extensive integration test suite that can be run against different versions of strong-remoting. Here is an initial draft for inspiration: https://gist.github.com/bajtos/b85e53a79ba06671e08d2013acccc611 Versions to run the suite against: v2.16.0, v2.16.1, latest 2.x, master (3.0-alpha). This commit may have introduced breaking change in v2.16.1: 34b0665

Connect to strongloop-internal/scrum-loopback#884
Connect to strongloop-internal/scrum-loopback#913

@ritch @STRML please take a look

bajtos · 2016-05-24T13:04:31Z

See also https://github.com/strongloop-internal/scrum-loopback/issues/706#issuecomment-217371058

STRML · 2016-05-24T13:09:31Z

Looks good so far, I agree with the idea of testing this more thoroughly. And it does indeed appear 34b0665 broke some backcompat, but you could argue previous behavior was a bug in the first place. Still worthwhile for cleaning up for good in 3.0.

STRML · 2016-05-24T13:11:33Z

See also #265 and especially #265 (comment) as handling this code was a comedy of errors - somebody inside strongloop force-pushed it out without any notice, then only half the PR was re-merged and released without review, causing significant breakage as the code was incomplete (it was two commits and should not have been merged incomplete).

bajtos · 2016-05-27T12:25:25Z

Done. @STRML @ritch PTAL

ritch · 2016-05-27T17:03:58Z

test/rest-coercion/_jsonbody.context.js

@@ -0,0 +1,73 @@
+// Copyright IBM Corp. 2014,2016. All Rights Reserved.


whats with the _ ?

I have two sets of files: helper files that does not contain any tests (names starts with underscode), and ".suite.js" files with the tests. The code loading all tests files is skipping underscore files.

I don't mind using a different approach/convention, can you propose a better solution?

bajtos · 2016-06-06T15:14:22Z

I have cleaned up the code a lot, restructured test cases and added comments to make it more clear what is being tested. The patch is ready for another round of code review.

@STRML @ritch PTAL.

bajtos · 2016-06-10T07:24:38Z

test/rest-coercion/README.md

+
+## URL path parameters
+
+We don't have coverage for this scenario yet.


I created an internal story to keep track of the task of adding test coverage for URL path parameters, see https://github.com/strongloop-internal/scrum-loopback/issues/921

bajtos · 2016-06-10T07:25:41Z

The high-level overview of changes between 2.16.x versions can be found here: https://gist.github.com/bajtos/8afaa9d1ed7d7b410c76ec2095a8a992

ritch · 2016-06-10T16:56:57Z

LGTM

I don't see anything that should change. But there is quite a bit I may be overlooking still. I think the size of this test suite points to a separate module making sense for coercion.

The documentation in these tests would also be helpful to everyone. I think we should create a section about coercion in the readme that lists out the nuances of coercion. Which can be done outside of this PR.

Amir-61 · 2016-06-10T19:35:07Z

@slnode test please

Amir-61 · 2016-06-10T20:41:01Z

I did not see any problem either. The patch LGTM.

davidcheung · 2016-06-13T03:49:16Z

LGTM too
should be really helpful for us to prevent regression on a much more detailed level 👍

bajtos · 2016-06-13T07:08:10Z

@ritch @Amir-61 @davidcheung thank you for the review!

The documentation in these tests would also be helpful to everyone. I think we should create a section about coercion in the readme that lists out the nuances of coercion. Which can be done outside of this PR.

I filled a new task for that - #313, I think it will be best to document the new behaviour after we make the intended changes/fixes.

Implement comprehensive test suite describing coercion of input arguments in the REST adapter. The test-suite produces test/rest-coercion/report.csv that can be used to compare results across different strong-remoting versions.

Implement comprehensive test suite describing coercion of input arguments in the REST adapter. The test-suite produces test/rest-coercion/report.csv that can be used to compare results across different strong-remoting versions. [back-port of #304]

relicense as Artistic-2.0 only update/insert copyright headers Set to no compression when using change stream Implement getEndpoints() * Implement getEndpoints * Deprecate getHttpMethod * Deprecate getFullPath 2.27.0 * Implement getEndpoints() (Amir Jafarian) * Set to no compression when using change stream (Candy) * update/insert copyright headers (Ryan Graham) * relicense as Artistic-2.0 only (Ryan Graham) * Handle array of errors. (Richard Pringle) Add integration tests for coercion in REST Implement comprehensive test suite describing coercion of input arguments in the REST adapter. The test-suite produces test/rest-coercion/report.csv that can be used to compare results across different strong-remoting versions. [back-port of strongloop#304] Fix integration tests for coercion in REST Address style violations, fix bug discovered by jshint. Add http source "formData" Add "formData" as an alias for "form", to make remoting metadata close to Swagger. [back-port of 2e4a500 and aa0bda6] Prioritise auth errors over sharedCtor errors Return 401 Not Authorized when making an unauthorised request to a prototype method for a model instance that does not exist. Potentially breaking change affecting prototype methods: Hooks registered before "invoke" phase (i.e. "auth" phase and any custom phases inserted before "invoke") will be called even when the model instance was not found. The property "ctx.instance" will be undefined in such case, hook handlers must account for this case. 2.28.0 * Prioritise auth errors over sharedCtor errors (Miroslav Bajtoš) * Add http source "formData" (Fabian Off) * Fix integration tests for coercion in REST (Miroslav Bajtoš) * Add integration tests for coercion in REST (Miroslav Bajtoš) 2.28.1 Introduce support for integer datatype There is single datatype in Javascript for representing numbers: i.e. `numbers`. This is a patch to fix strong-remoting#317 - a feature to support data type: `integer`. Includes following changes: - Update `convertValueToTargetType()` & `convertToBasicRemotingType()` - Update `SharedMethod.getType()` - Make `integer` check strict by-default. i.e. If argument and/or returned values are decimal or `> MAX_SAFE_INTEGER`, for argument: send `400`, for returns: send `500` - Rework `convert` method to ignore all arrays except "array of integers" - Add tests to rest-coercion integration tests - Update test cases for integer data type - Update shared-method.js to include support for Node v.0.10.x 2.29.0 * Introduce support for integer datatype (gunjpan) Backport of strongloop#329 Revert globalization of assert() messages Remove the usage of deprecated methods Backport from strong-remoting/strongloop#333 Add new API for finding/disabling remote methods 2.30.0 * Add new API for finding/disabling remote methods (Candy) * Remove the usage of deprecated methods (Amir Jafarian) * Revert globalization of assert() messages (Miroslav Bajtoš) * Backport of strongloop#329 (deepakrkris) Update dependencies 2.31.0 * Update dependencies (Miroslav Bajtoš) Fix tests to use res.get instead of res.headers The latter is case-sensitive and expects header names in lower-case. Backport strongloop#345 Add rest-coercion tests for zero-prefixed numbers Don't convert arg values returned by http function When an argument provides "http" mapping as a custom function, then we should not traverse the return value to detect buffers and dates. The return value could be a complex object, e.g. the full remoting context, in which case the traversal takes too long (in order of second!). Deprecate built-in CORS middleware Push the responsibility of enabling/configuring CORS back to the application developer. Add translation files Update translation files - round#2 Fix support for hooks returning a Promise Fix the code to correctly recurse and invoke next hooks after the promise returned by the first hook callback was resolved. 2.32.0 * Fix support for hooks returning a Promise (Miroslav Bajtoš) * Update translation files - round#2 (Candy) * Add translation files (Amir Jafarian) * Deprecate built-in CORS middleware (Miroslav Bajtoš) * Don't convert arg values returned by http function (Miroslav Bajtoš) * Add rest-coercion tests for zero-prefixed numbers (Miroslav Bajtoš) * Fix tests to use res.get instead of res.headers (Miroslav Bajtoš) Update ja translation file 2.32.1 * Update ja translation file (Candy) Distinguish between "file" and "File" result types Fix regression introduced by a85068e where we started to treat "File" models as the new built-in type "file". In this commit, the implementation is fixed to recognize only the following form as the built-in file: { type: 'file' } All other spellings, most notably "File", are treated as custom types (models) now. 2.32.2 * Distinguish between "file" and "File" result types (Fabien Franzen) lockdown dependency for http-auth Due to a recent release of 2.5.10 which broke node 10/12 support this commit locks down dependency before 2.5.11 is released as some registry will still pick up unpublished version (2.5.10) 2.4.11 is released with apache-md5, apache-crypt locked down See http-auth/http-auth/commit/0097ed6195986942a65e14b38b3f30281c9f6435 Stop changing read-only Number property Rework the code defining missing ES5/ES6 Number properties to not touch the existing properties. This is needed to prevent errors like the following: TypeError: Cannot assign to read only property 'MAX_SAFE_INTEGER' of function Number() { [native code] }"] 2.32.3 * lockdown dependency for http-auth (David Cheung) * Stop changing read-only Number property (Kogulan Baskaran) Stringify object query params as JSON [2.x] Stringify query params which are objects in a way in which empty ararys and null values are preserved instead of removed (default querystring implementation). backport of: strongloop#325 Rest-adapter errorHandler set content-type `json` the defaultHandler always res.sends an object therefore make sense to always set response header as such Release LB2 LTS Enable remote methods to be disabled by alias 2.33.0 * Enable remote methods to be disabled by alias (Rémi Bèges) * Release LB2 LTS (Simon Ho) * Rest-adapter errorHandler set content-type `json` (David Cheung) * Stringify object query params as JSON [2.x] (Horia Radu) Cache getRestMethodByName This function in RestAdapter is slow due to creating RestMethods each time. This can be cached initially, and rechecked if the entry was not found. add unit test updating for review

bajtos added this to the #Epic: Coercion Cleanup milestone May 24, 2016

bajtos assigned ritch May 24, 2016

bajtos force-pushed the test/rest-coercion branch from bcc2ad7 to 5263399 Compare May 26, 2016 13:22

bajtos mentioned this pull request May 27, 2016

Strings which are only numbers causes issues. #223

Closed

ritch reviewed May 27, 2016
View reviewed changes

bajtos self-assigned this May 30, 2016

bajtos force-pushed the test/rest-coercion branch 3 times, most recently from 9642ab7 to 772bd65 Compare June 8, 2016 11:44

bajtos mentioned this pull request Jun 9, 2016

Improve coercion of deeply nested queries #298

Closed

bajtos force-pushed the test/rest-coercion branch from 772bd65 to 8ce0efc Compare June 10, 2016 07:18

bajtos reviewed Jun 10, 2016
View reviewed changes

bajtos mentioned this pull request Jun 10, 2016

Fix coercion of input arguments (HTTP/REST) #312

Closed

bajtos force-pushed the test/rest-coercion branch from 11d06f0 to 777e76a Compare June 10, 2016 12:06

bajtos assigned davidcheung and Amir-61 Jun 10, 2016

bajtos mentioned this pull request Jun 13, 2016

Describe nuances of coercion in docs #313

Closed

Add integration tests for coercion in REST

c20b463

Implement comprehensive test suite describing coercion of input arguments in the REST adapter. The test-suite produces test/rest-coercion/report.csv that can be used to compare results across different strong-remoting versions.

bajtos force-pushed the test/rest-coercion branch from 777e76a to c20b463 Compare June 13, 2016 07:10

bajtos merged commit d94b375 into master Jun 13, 2016

bajtos deleted the test/rest-coercion branch June 13, 2016 08:15

		@@ -0,0 +1,73 @@
		// Copyright IBM Corp. 2014,2016. All Rights Reserved.


		## URL path parameters

		We don't have coverage for this scenario yet.

Test rest coercion #304

Test rest coercion #304

Uh oh!

Conversation

bajtos commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos commented May 24, 2016

Uh oh!

STRML commented May 24, 2016

Uh oh!

STRML commented May 24, 2016

Uh oh!

bajtos commented May 27, 2016

Uh oh!

ritch May 27, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos May 31, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 6, 2016

Uh oh!

bajtos Jun 10, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented Jun 10, 2016

Uh oh!

ritch commented Jun 10, 2016

Uh oh!

Amir-61 commented Jun 10, 2016

Uh oh!

Amir-61 commented Jun 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidcheung commented Jun 13, 2016

Uh oh!

bajtos commented Jun 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bajtos commented May 24, 2016 •

edited

Loading

Amir-61 commented Jun 10, 2016 •

edited

Loading