node-tastypie/tastypie

Error posting a JSON that has an array with a single value coercible to number

pfurini opened this issue · 11 comments

Given a model like this (using tastypie-rethink):

const EntryCategory = thinky.createModel('entry_category', {
  id: type.string(),
  extCategories: [type.string()],  // array of strings
}, {
  enforce_extra: 'strict'
});

and a corresponding resource:

const Category = RethinkResource.extend({
  options: {
    queryset: Model.filter({}),
    filtering: {
      extCategories: 1
    },
    allowed: {
      list: { get: true, post: true },
      detail: { get: true, put: true, patch: true, delete: true }
    }
  },
  fields: {
    id: {type: 'char', readonly: true},
    extCategories: {type: 'array', default: []}
  }
});

if you post a json like this

{
  "extCategories": ["123"]
}

results in a model validation error by thinky (value must be an array or null).

It is caused by the code in fields/array.js module, more specifically in the hydrate function:

try {
  cb( null, JSON.parse( value ))  //---> value is a JS array, not a string, and the result is the number 123..
} catch( err ){
  cb(null, this.convert( value ) )
}

A workaround can be

      if (typeof value === 'string')
        try {
          cb( null, JSON.parse( value ))
        } catch( err ){
          cb(null, this.convert( value ) )
        }
      else
        cb(null, this.convert( value ) );

@Nexbit Probably un related, but

I wouldn't set default on the field to an array / object. They don't get cloned. So if you modify that, then the default would also be changed

a lot like thinky models, you can set default to a function that returns an array

Or set it to null, and let the model do the defaults.

@Nexbit There is nothing in there that tries to pull out values from the array, or does type casting of values in the array.

JSON.parse on an array should throw and then the convert function is run.
Having a bit of trouble reproducing this. Can you add a test case to illustrate.

This might be more a thinky schema / tastypie-rethink bug.

Thanks.

Indeed when I debug that function value is a JS array, not a string, and JSON.parse does not throw.. If I try to evaluate JSON.parse([123]) (that is the value passed to hydrate) in a node repl it does not throw and returns the number 123.
So catching an error on parse does not work as intended in the first place, I fear..

If it's not intended that value gets an object (an array in this specific case) instead of a string, that's another story, and maybe it's tastypie-rethink that does the wrong thing.

I can assemble a test project in a public cloud9 workspace when I have some spare time, but that quick (and dirty) fix seems to be the only workaround for me.

@Nexbit Can you just add a test cast to the arrayfield test that fails ?

You're right, I cannot reproduce it without tastypie-rethink in the mix..
The problem might be in the create_object implementation. Currently it calls deserialize and then full_hydrate:

  , create_object: function create_object(bundle, callback) {
    var format = this.format(bundle, this.options.serializer.types)
      , that = this
      ;

    this.deserialize(bundle.data, format, function (err, data) {
      bundle = that.bundle(bundle.req, bundle.res, data);
      bundle.object = new that.options.queryset._model(data);
      that.full_hydrate(bundle, function (err, bndl) {
        bndl.object.saveAll().then(function () {
          return callback && callback(err, bndl);
        }).error(function (err) {
          return that.build_error(err, bundle);  //--> this is an enhancement in my fork
        });
      });
    });
  }

Is it ok to do deserialize and then feeding full_hydrate with a deserialized bundle? It seems to break several assumptions in your pipeline..

Yes, that should be fine. I have use the default rethink resource in real projects w/ arrays and not seen the behavior you are describing

@Nexbit I'm going to close this on. Can you re-open on the tastypie-rethink repo?

Sorry to bother again, but there's definitely some problem with the usage pattern of the hydration pipeline.
I just realized that the hydrate method is not affected, but the problem resides in the full_hydrate method, that you don't use in your fields tests.
I just set a watch on the bundle.object or bundle.data content before and after calling full_hydrate, and what I see is that before:

bundle.object.extCategories = ['123']

and after:

bundle.object.extCategories = 123

I can for sure open an issue on tastypie-rethink, but can you add more testing on the full_hydrate method usage, like what it does tastypie-rethink in create_object?
Thx

@Nexbit Yep. I'll take a look later today

@Nexbit So I updated the test data on the rethink repo and added a test to hold an array with a number as a string and posted the data as an encoded string using the default tastypie / resource set up everything worked as expected.

The build still passes.

If you can submit a test on the rethink repo that fails with the behavior you are seeing, that would be great. I'm just not having any luck reproducing.

Thanks.

I take that Back I was able to recreate it.
Investigating.

For the record, this is either a quirk or bug with JSON.parse.

JSON.parse(["1","2"]) // throws an error
JSON.parse(["1"]) // 1