strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Refactor and rework http coercion. #265

Closed bajtos closed 8 years ago

bajtos commented 8 years ago

This patch brings back #231 which was force-pushed away, probably by an accident.

/cc @STRML @ritch


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 #223 (coerced Number past MAX_SAFE_INTEGER) Possible fix for #208

bajtos commented 8 years ago

@STRML released in strong-remoting@2.23.0.

seriousben commented 8 years ago

@bajtos

This seems to break parsing of array in bulk creation somehow, our use case is using an array in the remote create for bulk creation and we expand the given array when a condition matches.

ctx.req.body:

[ { invitedCommunityId: '566f119567ee4ae2596935b3',
    cause: 'signup-flow' } ] 

ctx.args.data:

[ { '0':
     { invitedCommunityId: '566f119567ee4ae2596935b3',
       cause: 'signup-flow' },
    isDeleted: false,
    createdAt: Mon Dec 14 2015 13:59:34 GMT-0500 (EST),
    postIds: List [] } ]
STRML commented 8 years ago

@bajtos #231 had two commits, https://github.com/STRML/strong-remoting/commit/6ffa9635e48fcbe4bc6f7802077d14c3b7906bc1 and https://github.com/STRML/strong-remoting/commit/b1037d1a81f0c202592818d05d0ee7b6655c0108, this only brought in one.

doublemarked commented 8 years ago

Somebody came into Gitter tonight and complained that new strong-remoting is breaking false boolean value coercion when passed in on the query string. Looking at this commit it seems possible this is the case? The old logic was replaced with Boolean(val) , but Boolean('false') will evaluate as true. And I checked the tests and I don't see a QS test for false, only for true.

Relevant code: https://github.com/strongloop/strong-remoting/commit/df59c64249cabf54cee361f21f51a95be98ec7d9#diff-2fe12acc9dbfa09c6fb0e901266cf5e7R117

Forgive me if I'm off base here - I'm not following this PR, but since the merge happened so recently I thought it might be relevant to toss in here despite having not gotten fully familiar.

STRML commented 8 years ago

This is true. The issue is that this merge was part of 2.21, somehow got force-pushed out, and then was re-merged half-completed. #264 has a fix for the 'false' > true issue, but it's waiting on the proper merge in master so it can be rebased properly.

bajtos commented 8 years ago

For future readers coming to this thread, I have reverted this pull request in #269, the issues should be gone starting from v2.23.1.