miLibris / flask-rest-jsonapi

Flask extension to build REST APIs around JSONAPI 1.0 specification.
http://flask-rest-jsonapi.readthedocs.io
MIT License
597 stars 153 forks source link

Force string comparison for id field in patch. #54

Closed Natureshadow closed 7 years ago

Natureshadow commented 7 years ago

The patch code expected the id in the data object to be a string, while Flask-REST-JSONAPI itself delivers it as integer if it is one in the schema.

This made it a bit awkward to transform a loaded object into a patch object. This commit forces casting both sides of the comparison to string.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 95.038% when pulling 76f33e3d235d5bf4a2586b808e3487ba1368f265 on Natureshadow:patch-3 into 9566f6f9ae371d0e348c8a8804d3b27dcb4dc2c6 on miLibris:master.

Natureshadow commented 7 years ago

Did someone have a chance to have a look into this? As we will need it in production soon, a release with this fix would be much appreciated ☺.

rgant commented 7 years ago

To me it seems that your schema is wrong if it is leaving the id field as a number. In my case the id fields in the schema is defined as id = Integer(as_string=True). I wonder if that would solve this issue for you.

https://marshmallow.readthedocs.io/en/latest/api_reference.html#marshmallow.fields.Number

Natureshadow commented 7 years ago

That would work around the issue, not solve it. If my id field is an integer, why shouldn't I be able to expose it as an integer? Having to break my schema to make the API work seems flawed.

rgant commented 7 years ago

I don't think you are "breaking your schema" there are several different representations of an Integer such as a string or as a number. In JSONAPI spec the id field is a string, but in the database the column is an integer. So to me it seems reasonable to make the Schema handle the translation from database (integer) to JSONAPI (string that happens to be an integer).

I will also note that the examples in this project define the id field in the schema as a string field:

id = fields.Str(dump_only=True)

My suggestion is perhaps a better solution because you get the benefit of fields.Integer validation that fields.Str won't provide. Of course if you prefer your solution then I'll stop trying to convince you. I am just trying to find you a fix that works now. Best of luck!

Natureshadow commented 7 years ago

I do have a fix that works now (see patch) ☺.

akira-dev commented 7 years ago

Fixed in #58