Fixed a bug where floats in the database would not be serialized corr… #89
Conversation
…ectly in the json response
| content, | ||
| default=default, | ||
| option=orjson.OPT_NON_STR_KEYS | orjson.OPT_SERIALIZE_NUMPY, | ||
| ) |
There was a problem hiding this comment.
could we create a custom JSONResponse class to avoid duplicating the render code?
There was a problem hiding this comment.
Yeah should be easy
There was a problem hiding this comment.
Looks good to me. Can you check if we need to do the same thing for postgres fields of type "numeric" as well (which is equivalent to the postgres "decimal" type?
|
@RemcoMeeuwissen I've been away and focused on titiler for the past week, I'll try to have a look on all the contributions next week 🙏 |
| def default(obj): | ||
| """Instruct orjson what to do with types it does not natively serialize""" | ||
| if isinstance(obj, decimal.Decimal): | ||
| return str(obj) |
There was a problem hiding this comment.
why do we use string instead of float here?
The result of this will return a string within the JSON so we loose the fact that it is a number in the table
There was a problem hiding this comment.
I'm not actually sure what we want here as best practice as decimal/numeric can be arbitrarily larger than what float could handle. Returning as numeric in the json would definitely fit most people's expectations, but could lose range / precision that is why people use those type rather than float.
There was a problem hiding this comment.
Yeah string felt like the safe option to me so that's why I went with it, but I don't know at which point precision is lost so I don't know if it is actually a relevant concern.
There was a problem hiding this comment.
I'd say that we stick with string as it is the only way we can be certain to not be lossy. If someone knows their data fits into a float, they can alter their table.
There was a problem hiding this comment.
Looking at simplejson library, it seems they do convert decimal to float
a = {"a": decimal.Decimal('1.00000000006')}
simplejson.dumps(a)
'{"a": 1.00000000006}'There was a problem hiding this comment.
🤔 I think it's because, simplejson will automatically convert decimal to float internally!
I'd say that we stick with string as it is the only way we can be certain to not be lossy. If someone knows their data fits into a float, they can alter their table.
👍 let's merge this, and we will revisit later if needed 🙏
| INSERT INTO "public"."my_data" ("geom" , "id", "datetime") VALUES ('0103000020E610000001000000110000000A4C8422590E46C0B656FB86F03B5340D5E76A2BF60F46C0075F984C153C5340FA28B2217F0346C0CE0A257ADB3D5340BEE6287052F545C01AA33BF2DF3F5340F25A937BB7D244C009CB92853C69534049A5CD2EAE0644C03857A7D84686534063B4EEABC4F943C08D992E511D88534034A2B437F8EA43C0F54A5986388A53409C72BC6BC5E843C0920AAB5C038A534050AF9465883342C0363B85F6B5605340D43E0032881142C02A5884BF7F5D5340F4FDD478E90641C007F01648504453409C6F1F2DEA1541C00EA6095E6A4253404E4E9C88873342C06DC6E4C7471E53403EDF52396E3443C0DC9EAF2DC7FD524044696FF0854143C032772D211FFC52400A4C8422590E46C0B656FB86F03B5340', '4', '2004-10-23 10:23:54'); | ||
| INSERT INTO "public"."my_data" ("geom" , "id", "datetime") VALUES ('0103000020E6100000010000000D000000BBE9944235C347C0EBF06E7961EE52406ADE718A8EC447C0D122DBF97EEE5240942D6301ECB947C05B59871F60F0524086CAEEF61AAE47C0BDEF3BBB76F252400A4C8422590E46C0B656FB86F03B5340FA28B2217F0346C0CE0A257ADB3D534057EC2FBB27F745C02B1895D409405340BEE6287052F545C01AA33BF2DF3F53401D386744692743C07958A835CDFF52403EDF52396E3443C0DC9EAF2DC7FD5240B9E39237FD0645C0574B4E2543B552400AD7A3703D1245C03A234A7B83B35240BBE9944235C347C0EBF06E7961EE5240', '5', '2004-10-24 10:23:54'); | ||
| ALTER TABLE "public"."my_data" ADD COLUMN "decimal" DECIMAL; | ||
| ALTER TABLE "public"."my_data" ADD COLUMN "numeric" NUMERIC; |
There was a problem hiding this comment.
I have added values for numeric and it seems that they get returned as Decimal so we're fine
| @@ -769,6 +769,10 @@ def test_items_env_table_config_main(app, monkeypatch): | |||
| body = response.json() | |||
| assert body["features"][0]["geometry"]["type"] == "Polygon" | |||
|
|
|||
| # Make sure that Postgres Decimal and Numeric are converted to str | |||
| assert isinstance(body["features"][0]["properties"]["decimal"], str) | |||
| assert isinstance(body["features"][0]["properties"]["numeric"], str) | |||
| @@ -14,6 +14,7 @@ Note: Minor version `0.X.0` update might break the API, It's recommended to pin | |||
| - `type` query parameter to filter collections based on their type (`Function` or `Table`) | |||
| - fixed a small bug in the `tipg_properties` SQL function where the bounds property was not properly transformed to 4326 (author @RemcoMeeuwissen, https://github.com/developmentseed/tipg/pull/87) | |||
| - handling functions that are interpreted as collections but lack parameters (author @jackharrhy, https://github.com/developmentseed/tipg/pull/96) | |||
| - fixed a bug where orjson could not properly serialize float values present in the properties of a feature (author @RemcoMeeuwissen, https://github.com/developmentseed/tipg/pull/89) | |||
There was a problem hiding this comment.
I'll update this line to mention that Decimal will be converted to string
What I am changing
Ran into a bug where having a float in the database would cause a
Type is not JSON serializable: decimal.Decimalerror when requesting a json response.How I did it
orjson has a default parameter when calling the dumps function which allows you to specify how to serialize unknown types, using this Decimal is now serialized to string.
Because the ORJsonResponse wrapper provided by fastapi does not support sending along this parameter I tweaked the GeoJSONResponse and SchemaJSONResponse classes to use orjson directly (basically recreating the wrapper, luckily it's fairly small).
How you can test it
I've added a number field to the my_data.sql fixture, this caused the bug to be triggered on existing tests so no new tests are needed.