strongloop / strong-remoting

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

Fix improper type coercions, especially on array types. #196

Closed STRML closed 9 years ago

STRML commented 9 years ago

This moves the arrayItemDelimiters configuration into http-context.js, as it is an HTTP implementation detail like querystring parsing and is not generalizable to remote methods exposed in other ways.

This PR also makes argument coercion more robust for any, object, and array types.

I was having an issue with single array arguments being coerced improperly to numbers - for example, with accepts type of ['string'], input like '123456' was being coerced to the number 123456. It should be coerced to ['123456'].

ritch commented 9 years ago

This looks good overall. I'll take a closer look and merge soon if I don't find anything else.

Great job! Thanks for the contribution @STRML

STRML commented 9 years ago

Anything I can do to help this along? We're running this fork in production & this is a serious bug IMO.

STRML commented 9 years ago

FYI the merge error is caused by https://github.com/strongloop/strong-remoting/commit/fca2fb05f44fe124d285fe2db768375e94f6bc49, which seems to be a typoed fix to the lint error. Once that is fixed this will merge cleanly.

STRML commented 9 years ago

Thanks. On Apr 14, 2015 5:19 PM, "Ritchie Martori" notifications@github.com wrote:

Merged #196 https://github.com/strongloop/strong-remoting/pull/196.

— Reply to this email directly or view it on GitHub https://github.com/strongloop/strong-remoting/pull/196#event-281415692.

ritch commented 9 years ago

released as 2.16.1

ritch commented 9 years ago

Thanks.

No. Thank you :+1:

XinV commented 9 years ago

Hi, guys, we are having issues when passing the array as the input param to the API endpoint, do you cover the array as param in your unit testing?

The request is:

      utils.authorizedCall("post", url, token)
        .send({
          "partnerships" : [9006],
          "groups" : ["sony usa"]
        })
        .expect(200)
        .end(function(err, res){
          console.log("Error", err);

2015-04-15T19:12:10.642Z - error: /api/orgs/sony/groupmemberships statusCode=400, statusDescription=Bad Request, @context=/contexts/Error.jsonld, @id=/errors/BadRequestError, code=4004000
Error [Error: expected 200 "OK", got 400 "Bad Request"]
Res { error: 
   { name: 'Error',
     message: 'invalid value for argument \'groups\' of type \'Array\': sony usa. Received type was string. Error: Unexpected token s',
     '@context': '/contexts/Error.jsonld',
     '@id': '/errors/BadRequestError',
     code: 4004000,
     statusDescription: 'Bad Request',
     status: 400,
     statusCode: 400 } }
bajtos commented 9 years ago

@XinV I have submitted a patch to fix the bug, see https://github.com/strongloop/strong-remoting/pull/204

XinV commented 9 years ago

Thanks @bajtos , for now we force the dependency to v2.15.0. Will upgrade it once your fix is released.

bajtos commented 9 years ago

@XinV the bug should be fixed in strong-remoting@2.16.2, please let us know if the new version works for you well.