Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bajtos opened this issue Jun 10, 2016 · 4 comments
Closed

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

bajtos opened this issue Jun 10, 2016 · 4 comments
Assignees
Labels

Comments

@bajtos
Copy link
Member

bajtos commented Jun 10, 2016

Based on the integration test suite written in #304, I am proposing to make the following changes/fixes in the way how we coerce input arguments.

I think most (if not all) of this changes can be considered as backwards-compatible fixes to be landed on 2.x too.

@STRML @ritch thoughts?


Terminology:

  • json body - the argument is set to the full request body which is JSON encoded
  • json form - the argument is read from a property in the JSON-encoded request body
  • urlencoded - the argument is read either from query string or from urlencoded request body

Tighten validation of required arguments

  • json body - any - required should reject null value
  • json form - array - required should reject empty request and null value
  • json form - * - required should reject empty request, null value
    and empty string value
  • urlencoded - * should reject empty query, ?arg, ?arg=, ?arg=null

Reject scalar values for array type

  • json body - array of *
  • json form - array - required

Do not coerce missing value to empty array

This may be possibly controversial. Should we introduce a config option
to control this behaviour?

  • json form - array of * should convert empty body to undefined
  • json form - array of * should convert null to null
  • urlencoded - array of * should convert ?, ?arg, ?arg= to undefined

Do not coerce values from JSON-encoded sources

  • Reject requests sending array items of wrong type
    • json body - array of *
    • json form - array of *
  • Do not coerce array items from string to number, boolean, etc.
    • json body - array of any
    • json form - array of any
  • Reject requests providing value of wrong type
    • json form - boolean (reject strings, numbers, etc.)
    • json form - date (reject booleans, large numbers, non-date strings, etc.)
    • json form - number (reject strings, booleans, arrays, etc.)
    • json form - object (reject strings, booleans, numbers)
    • json form - string (reject booleans, numbers, objects, arrays, etc.)

Allow null as a value for object type

Convert null input to null value:

  • json body - object - optional
  • json form - *
  • urlencoded - *

Treat missing argument as undefined

Set argument to undefined when not set:

  • json form - *
  • urlencoded - * - treat ?arg as undefined

Don't coerce too large numbers

  • urlencoded - any - keep ?arg=2343546576878989879789 as string value

Recognize scientific notation when parsing strings

  • urlencoded - any - convert ?arg=1.234e%2B30 to a number

Malformed JSON in urlencoded value should trigger 400 Bad Request

  • urlencoded - array of * should reject ?arg={malformed} and
    ?arg=[malformed]
  • urlencoded - object should reject ?arg={malformed} and
    ?arg=[malformed]

Convert numeric timestamps from urlencoded sources to number

?arg=0 should be converted to new Date(0) for both

  • urlencoded - date
  • urlencoded - array of date

Do not coerce array items from urlencoded sources

  • urlencoded - array of *

Do not coerce non-boolean values

urlencoded - boolean should reject all values except true/false, e.g.

  • numbers like ?arg=0 and ?arg=1
  • strings
  • arrays
  • objects

Avoid NaN dates, return 400 Bad Request instead

Examples: ?arg=undefined, ?arg=true, but also ?arg=2343546576878989879789

Open points to discuss

  • Should we coerce missing array values into an empty array? Perhaps make
    this configurable?

  • Should we coerce date strings stored in object properties/array items
    from JSON sources for type:object, type:any and type:array?

    Example inputs:

    { "arg": "2016-05-19T13:28:51.299Z" }
    ["2016-05-19T13:28:51.299Z"]
  • Should we keep distinction between undefined (argument is missing) and
    null (argument is provided with null value)?

  • Should we allow array values for object type arguments? If yes, should
    we allow date strings too and coerce them into Date instances?

  • How to treat ?arg= in query string/urlencoded request body? Should we
    treat it as an empty string or as a missing value?

  • How to treat ?arg=null, should we parse it as null or as a string
    "null"?

  • urlencoded - any - should we coerce Date strings to Date instances? What
    formats other than the full ISO string should be recognized, e.g 2016-05-01
    or T09:30:00?

@bajtos
Copy link
Member Author

bajtos commented Jun 10, 2016

Case-insensitive boolean strings

Both ?arg=FALSE and ?arg=FalsE should be coerced to `false.

@STRML
Copy link
Member

STRML commented Jun 10, 2016

This all looks really good and I'm glad you guys are taking a serious look at it.

urlencoding is probably the trickiest part because there is so much sloppiness in this string-only input method. By far the most coercion should go there.

On the contentious issues:

  • If you're accepting an array input for a field, I don't see any semantic difference between [] (no items) and null (no items). But [] is much more convenient as you can simply iterate.
  • Re urlencoded boolean coercion, I think it should coerce 0 and 1 to boolean as this is often typical in querystrings due to perceived/legacy url size limits.
  • I think Date coercion should only happen when the input type is Date, otherwise it's potentially too much magic - the user must then know which date formats are coerced and which are not. Of course, I wouldn't recommend ever using the any type or an untyped array.
    • I apologize for being somewhat out of the loop, but if I type a parameter as an actual model type (or is there any anonymous schema type?), will it coerce its attributes? E.g. something like `type: 'object', schema: {date: Date}' or the like.
  • Collection types (arrays of objects) should be allowable. I have an endpoint that does this now but I have to rely on my own code to typecheck it. This goes along with the point above. Technically I suppose this checking should be as composable as you wish.
  • IMO ?arg= should be '' for string types only, null otherwise.
  • ?arg=null should be the string null for string types only, null otherwise.
  • any should do less coercion than explicit typing (such as null above) and should not coerce dates (too much magic).

@ritch
Copy link
Member

ritch commented Jun 10, 2016

I think Date coercion should only happen when the input type is Date, otherwise it's potentially too much magic

Sure, and I thought we all agreed that we were moving away from implicit coercion.

I apologize for being somewhat out of the loop, but if I type a parameter as an actual model type (or is there any anonymous schema type?), will it coerce its attributes? E.g. something like `type: 'object', schema: {date: Date}' or the like.

If you specify type: 'SomeModel' strong-remoting will use the converter specified by loopback to new SomeModel(arg), which will set default properties and apply all setters and getters. It will coerce based on the properties type.

var properties = {date: {type: Date}};
var MyModel = loopback.createModel('MyModel', properties);
var data = {date: '2016-06-10T16:25:55.132Z'};
var inst = new MyModel(data);
assert(inst.date instanceof Date);

@bajtos
Copy link
Member Author

bajtos commented Sep 9, 2016

Done and released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants