Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Dec 14, 2015

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

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 bajtos self-assigned this Dec 14, 2015
@bajtos bajtos added the #review label Dec 14, 2015
bajtos added a commit that referenced this pull request Dec 14, 2015
@bajtos bajtos merged commit df59c64 into master Dec 14, 2015
@bajtos bajtos removed the #review label Dec 14, 2015
@bajtos bajtos deleted the fix/sloppy-argument-coercion branch December 14, 2015 10:15
@bajtos
Copy link
Member Author

bajtos commented Dec 14, 2015

@STRML released in strong-remoting@2.23.0.

@seriousben
Copy link

@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
Copy link
Member

STRML commented Dec 14, 2015

@bajtos #231 had two commits, STRML@6ffa963 and STRML@b1037d1, this only brought in one.

@doublemarked
Copy link

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: df59c64#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
Copy link
Member

STRML commented Dec 15, 2015

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
Copy link
Member Author

bajtos commented Jan 5, 2016

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.

@STRML STRML mentioned this pull request May 24, 2016
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Strings which are only numbers causes issues.

5 participants