skalarsystems / fhirzeug

A Python FHIR specification parser and class generator
Apache License 2.0
17 stars 1 forks source link

Serialization of Decimal values #50

Closed Wauplin closed 4 years ago

Wauplin commented 4 years ago

Issue first addressed in #47 . Ticket is splitted since two issues were initially reported.

Issue : 0.00 turns into 0.0 for float fields

To be more precise, float fields are at the moment handled using decimal.Decimal object to keep best possible precision. Problem arises when we use pydantic_fhir along FastAPI.

Wauplin commented 4 years ago

I took back what I wrote earlier in issue #47 :

FastAPI json encoder : The problem comes from the fact that FastAPI converts decimal to float. Since in our case we want to convert it to str, we can use the provided custom_encoder argument.

import decimal
from fastapi.encoders import jsonable_encoder

print(jsonable_encoder({"value": decimal.Decimal("5.000")}))
# {'value': 5.0}

print(
    jsonable_encoder(
        {"value": decimal.Decimal("5.000")}, custom_encoder={decimal.Decimal: str},
    )
)
# {'value': '5.000'}

FastAPI json encoder through pydantic :

FastAPI is using the json_encoders config attribute of pydantic models to do the serialization so it would be best to take advantage of it. However, it seems that it doesn't work in our case because of inheritance :

import decimal
import pydantic
from fastapi.encoders import jsonable_encoder

class ModelWithEncoder(pydantic.BaseModel):
    value: decimal.Decimal

    class Config:
        json_encoders = {decimal.Decimal: str}

class ChildModel(ModelWithEncoder):
    class Config:
        """No json_encoders"""

value = decimal.Decimal("5.000")
parent_model = ModelWithEncoder(value=value)
child_model = ChildModel(value=value)

print(jsonable_encoder(parent_model))
# FASTApi uses encoder from pydantic
# {'value': '5.000'}

print(jsonable_encoder(child_model))
# Encoder from parent model is not used
# {'value': 5.0}

Suggestions :

  1. In my very humble opinion, I think this issue has to do with FastAPI implementation.

https://github.com/tiangolo/fastapi/blob/master/fastapi/encoders.py#L53

    if isinstance(obj, BaseModel):
        encoder = getattr(obj.Config, "json_encoders", {})
        if custom_encoder:
            encoder.update(custom_encoder)

The code is checking at obj.Config which doesn't inherit from parents classes. The solution would be to use __config__ instead. If I change this line, it solves my problem :

        encoder.update(getattr(obj.__config__, "json_encoders", {}))
  1. As a quick workaround for us, we should use the custom_encoder attribute of FastAPI in our use case and signal the issue in FastAPI project.
Wauplin commented 4 years ago

Fun fact : We also need to change the json_encoder in our code to meet requirements. At the moment Decimal values are converted to either float or int :

def decimal_to_json(value: decimal.Decimal) -> typing.Union[float, int]:
    """Convert a decimal to float or int, depending on if it has a decimal part.

    It is for JSON serialization - to serialize it in the same form as was
    originally provided.
    """
    if value.as_tuple().exponent == 0:
        return int(value)
    return float(value)

https://github.com/skalarsystems/fhirzeug/blob/master/fhirzeug/generators/python_pydantic/templates/resource_header.py#L58