Refactor and rework http coercion. #231

STRML · 2015-08-04T21:41:12Z

This fixes a number of subtle bugs and restricts "sloppy"
argument coercion (e.g. 'true' to the bool true) to string-only
HTTP datasources like querystrings and headers.

Fixes Strings which are only numbers causes issues. #223 (coerced Number past MAX_SAFE_INTEGER)
Possible fix for Cleanup argument coercion and validation #208

raymondfeng · 2015-08-04T21:42:03Z

@STRML Thanks for the patch!

STRML · 2015-08-04T21:49:09Z

Just wanted to note that there are some breaking changes here:

application/json bodies are no longer being sloppily coerced (so 'true' no longer coerces to true), since they can encode some proper type information.
null/undefined is not automatically coerced, so null no longer coerces to 0 on a 'number' argument, or 'null' on a 'string' argument (this bit me hard in a JSON body today).

STRML · 2015-08-04T21:50:03Z

lib/dynamic.js

-  this.converters.unshift(converter);
+  this.converters[name] = function(val, ctx) {
+    // Ignore null/undefined
+    if (name !== 'array' && val === null || val === undefined) return val;


I should probably expand the comment here - I skip 'array' so that the 'array' converter has a chance to coerce null/undefined to [].

STRML · 2015-08-04T22:10:03Z

I'll squash when ready - open to discussion on this.

ritch · 2015-09-15T21:10:02Z

test/http-context.test.js

-    describe('arguments without a defined type (or any)', function() {
-      it('should coerce boolean strings into actual booleans', givenMethodExpectArg({
+    describe('don\'t coerce arguments without a defined type (or any) in JSON', function() {
+      it('should not coerce boolean strings into actual booleans', givenMethodExpectArg({


This breaks backwards compat. I understand why this change makes sense, but it also makes things like this impossible in loopback:

GET /todos?filter[where][done]=true

Obviously we need a bit more sophisticated type declaration support... but when you don't even know the structure (eg. a nested property of an object) it gets a bit tricky. I'm open to suggestions to overhaul this type system if it means better support for dynamicly typed queries in URLs. Maybe there is some syntax out there that caries more type information?

This is only with JSON request bodies, not with querystrings.

I see... you switched up givenMethodExpectArg to send JSON.

ritch · 2015-09-15T21:25:27Z

I think this might be a good basis for strong-remoting v3.0.0. I'd like to be careful and clearly document the breaking changes instead of trying to preserve the old behavior (any sort of type inference) in some unpredictable way.

We must support something capable of these type of queries though:

GET /todos?filter[where][priority][gt]=100&filter[where][done]=false

Right now we do this by inferring the type information. This makes the framework a bit magic, and gives less control to the consumer of the API as well producer. You do have the ability to specify explicit types, but that only works in the obvious cases. So in summary I welcome the changes to type inference. I am also in favor of just removing it entirely.

But lets not give up on the simplicity of the query syntax, because the flaw of the implementation. Maybe there is a nice syntax / notation / encoding out there that:

easily fits into URLs
supports explicit typing / doesn't require type inference
works well with the browser / curl / and other tools (eg. doesn't get converted into something un-readable by humans)

A couple that come close:

ejson or json - don't play nicely in URLs
our current approach - no explicit type support (like JSON)

@STRML @bajtos thoughts?

ritch · 2015-09-15T21:35:07Z

Here is some food for thought that would extend our current syntax with JSON literals:

GET /todos?filter[where][priority][gt]=100&filter[where][done]=false

Would become...

GET /todos?filter[where][priority][gt]=`100`&filter[where][done]=`false`

This would parse to:

{
  where: {
    priority: { gt: 100 },
    done: false
  }
}

We could also limit these values to a subset of JSON to avoid JSON.parse() overhead. Some examples off the top of my head:

100
0.1
true
false
Date(str) - "str" would also match a pattern before being parsed
null
undefined

STRML · 2015-09-15T21:41:33Z

@ritch See https://github.com/strongloop/strong-remoting/pull/231/files#diff-d54d77feb22761cf4ef39576b7edd949R118, I am handling sloppy coercion when no type data is available (formData and querystring).

ritch · 2015-09-15T22:04:05Z

Yes I like what you've done... but we are trying to preserve behavior that we should probably just get rid of (sloppy coercion). What if I had the unfortunate name: "Mr. False Undefined Null"?

GET /people?filter[where][firstName]=false

I don't like the fact that as the API consumer I cannot specify that I mean the literal string "false". Maybe its a silly example, but its something I'd like to get rid of. The type inference / sloppy coercion was an attempt to fix a problem with the query string syntax but from the wrong way.

application/json bodies are no longer being sloppily coerced (so 'true' no longer coerces to true), since they can encode some proper type information.

I think we can land/release this as a minor version.

null/undefined is not automatically coerced, so null no longer coerces to 0 on a 'number' argument, or 'null' on a 'string' argument (this bit me hard in a JSON body today).

If this is limited to JSON, we can land/release also as a minor version. When the input is JSON we should not infer any type that JSON can natively express. AFAICT your patch makes this change.

Anyways, we can discuss removing type inference in another issue.

ritch · 2015-09-15T22:07:21Z

lib/dynamic.js

+ */
+Dynamic.getArrayConverter = function(converter, val, ctx) {
+  if (!Array.isArray(val)) val = this.converters.array(val, ctx);
+  return val.map(function(v) {


Can we avoid this closure?

Not if we're going to pass ctx to converter - and the anon func syntax is still quite a bit faster than bind. It's not async, no chance of a leak here because of vars bound outside the closure.

STRML · 2015-09-15T22:08:21Z

Yes, this is an unfortunate problem with querystring syntax.

This PR is designed to be merged as a minor release as the only behavioral changes it makes should be considered bugs / undesirable behavior.

Unfortunately I don't have a good answer for how to do nice querystring syntax with support for nested attributes and some semblance of type. I don't think we should invent one ourselves, though.

In general, if the API author properly creates models (say, filter could be an actual user model), this won't be a problem. Otherwise, the author should know that if the input is a deep object in a GET param with no provided type information, he should coerce it to the needed type.

ritch · 2015-09-15T22:12:22Z

test/http-context.test.js

+}
+
+// Tests sending via querystring - should be sloppy conversions
+function givenQSExpectArg(options) {


Ah... missed this. Makes sense.

ritch · 2015-09-15T22:22:47Z

I've done a second pass review and I don't see anything preventing us from merging this. Though I am a bit nervous that with this large amount of changes to tests that something was missed. @STRML @bajtos any ideas on this? In theory, since this build is green we shouldn't have to worry about loopback tests failing after merging this.

@STRML have you run any LoopBack apps / automated tests with this version?

STRML · 2015-09-15T22:28:14Z

At this time I have not run any tests outside of the suite with this version.

But I am looking forward to the merge so I can remove this code:

      // FIXME why is strong-remoting sending 'null'???
      if (pgpPubKey == null || pgpPubKey === 'null' || pgpPubKey === user.pgpPubKey) return;

ritch · 2015-09-15T22:32:24Z

@slnode test please

ritch · 2015-09-15T22:35:13Z

At this time I have not run any tests outside of the suite with this version.

OK. I'll run it against some apps to get some more confidence in the BC.

ritch · 2015-09-15T22:37:15Z

Looks like the build failed. I don't think it has to do with these changes though. Something weird is going on with json-rpc.

ritch · 2015-09-16T06:21:14Z

@slnode test please

rmg · 2015-09-16T06:27:13Z

Branch needs to be rebased to pull in the fix from #243 that makes our CI green.

This fixes a number of subtle bugs and restricts "sloppy" argument coercion (e.g. 'true' to the bool true) to string-only HTTP datasources like querystrings and headers. Fixes strongloop#223 (coerced Number past MAX_SAFE_INTEGER) Possible fix for strongloop#208

STRML · 2015-09-16T17:46:36Z

Rebased.

ritch · 2015-09-16T18:04:50Z

@STRML thanks for the PR... this is really great.

I'll try and land this today. Need to ensure the top down build is all passing and a couple of examples are failing for unrelated reasons.

rmg · 2015-09-16T21:00:46Z

@slnode test please

rmg · 2015-09-16T22:15:46Z

@STRML don't be alarmed by the large number of failures - it's a result of me adding an additional level of dependencies to the CI mix and a lot of those modules were added only recently and are still lacking real tests.

bajtos · 2015-09-17T09:14:26Z

@ritch I don't have a better solution for nice query strings, I am fine with landing this patch as an incremental (non-breaking) enhancement. As for unit-tests, it seems to me that most existing tests remain unchanged and this patch is adding more new tests. I guess it would be nice to run the new tests against the old (current) implementation, to see if there are any unanticipated changes?

ritch · 2015-09-17T16:41:08Z

@slnode test please

Refactor and rework http coercion.

STRML · 2015-10-17T17:07:43Z

Did this get rebased out of master and force-pushed somehow?

STRML · 2015-10-17T17:16:16Z

Additionally, there's more that I'd like to add to this in another PR - for example, null in POST bodies is incorrectly coerced to 0 when the type is number, which feels almost certainly incorrect.

STRML · 2015-12-04T15:07:53Z

@bajtos What happened here?

I just got snagged by a bug because null was being coerced to 'null'. Why was this rebased out of master without any notification?

bajtos · 2015-12-04T15:11:29Z

This is weird, I have no idea why 5fdf291 is no longer on the master. @ritch @raymondfeng any ideas?

STRML · 2015-12-09T21:42:07Z

@bajtos Why does it take almost two months to get an answer as to why a commit was rebased out and force pushed to loopback's most important dependency?

I understand there are some bugs - working through them now - but notification + a revert (and/or new failing tests) would be the correct way to do it, simply rebasing the commit out without notice is unacceptable.

bajtos · 2015-12-14T09:44:29Z

@STRML it looks like somebody has accidentally run git push -f on master and remove the merge commit which landed this patch. I have carefully reviewed the current git history on master against the list of pull requests and parent commits, I believe this is the only patch that was lost.

I opened a new PR #265 to verify the patch on CI before landing it again.

To prevent this situation from happening in the future, I have configured master and 1.x branches as protected (docs), which means GitHub will reject git push -f from now on.

I apologise for the troubles.

altsang added the #community contribution label Aug 4, 2015

raymondfeng assigned ritch Aug 4, 2015

STRML reviewed Aug 4, 2015
View reviewed changes

ritch reviewed Sep 15, 2015
View reviewed changes

STRML added 2 commits September 16, 2015 13:42

Add a few more tests and fix some subtle bugs.

b1037d1

STRML force-pushed the reworkHTTPCoercion branch from 3671c44 to b1037d1 Compare September 16, 2015 17:42

ritch added a commit that referenced this pull request Sep 18, 2015

Merge pull request #231 from STRML/reworkHTTPCoercion

5fdf291

Refactor and rework http coercion.

ritch merged commit 5fdf291 into strongloop:master Sep 18, 2015

ritch removed the #community contribution label Sep 18, 2015

STRML mentioned this pull request Dec 9, 2015

Coercion refactor #264

Closed

bajtos mentioned this pull request Dec 14, 2015

Refactor and rework http coercion. #265

Merged

bajtos mentioned this pull request Dec 16, 2015

Revert "Refactor and rework http coercion." #269

Merged

bajtos mentioned this pull request May 9, 2016

Improve coercion of deeply nested queries #298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and rework http coercion. #231

Refactor and rework http coercion. #231

STRML commented Aug 4, 2015

raymondfeng commented Aug 4, 2015

STRML commented Aug 4, 2015

STRML Aug 4, 2015

STRML commented Aug 4, 2015

ritch Sep 15, 2015

STRML Sep 15, 2015

ritch Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 15, 2015

STRML commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch Sep 15, 2015

STRML Sep 15, 2015

STRML commented Sep 15, 2015

ritch Sep 15, 2015

ritch commented Sep 15, 2015

STRML commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 16, 2015

rmg commented Sep 16, 2015

STRML commented Sep 16, 2015

ritch commented Sep 16, 2015

rmg commented Sep 16, 2015

rmg commented Sep 16, 2015

bajtos commented Sep 17, 2015

ritch commented Sep 17, 2015

STRML commented Oct 17, 2015

STRML commented Oct 17, 2015

STRML commented Dec 4, 2015

bajtos commented Dec 4, 2015

STRML commented Dec 9, 2015

bajtos commented Dec 14, 2015

Refactor and rework http coercion. #231

Refactor and rework http coercion. #231

Conversation

STRML commented Aug 4, 2015

raymondfeng commented Aug 4, 2015

STRML commented Aug 4, 2015

STRML Aug 4, 2015

Choose a reason for hiding this comment

STRML commented Aug 4, 2015

ritch Sep 15, 2015

Choose a reason for hiding this comment

STRML Sep 15, 2015

Choose a reason for hiding this comment

ritch Sep 15, 2015

Choose a reason for hiding this comment

ritch commented Sep 15, 2015

ritch commented Sep 15, 2015

STRML commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch Sep 15, 2015

Choose a reason for hiding this comment

STRML Sep 15, 2015

Choose a reason for hiding this comment

STRML commented Sep 15, 2015

ritch Sep 15, 2015

Choose a reason for hiding this comment

ritch commented Sep 15, 2015

STRML commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 15, 2015

ritch commented Sep 16, 2015

rmg commented Sep 16, 2015

STRML commented Sep 16, 2015

ritch commented Sep 16, 2015

rmg commented Sep 16, 2015

rmg commented Sep 16, 2015

bajtos commented Sep 17, 2015

ritch commented Sep 17, 2015

STRML commented Oct 17, 2015

STRML commented Oct 17, 2015

STRML commented Dec 4, 2015

bajtos commented Dec 4, 2015

STRML commented Dec 9, 2015

bajtos commented Dec 14, 2015