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

improve decimal handling #24

Closed
tfranzel opened this issue Apr 14, 2020 · 7 comments
Closed

improve decimal handling #24

tfranzel opened this issue Apr 14, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@tfranzel
Copy link
Owner

decimals can be both string and number. utilize COERCE_DECIMAL_TO_STRING to detect the case

implement funtionality comparable to encode/django-rest-framework@603aac7

@tfranzel tfranzel added the enhancement New feature or request label Apr 14, 2020
@gujacob
Copy link

gujacob commented Sep 24, 2022

Hi,
I see this is closed for a long time, but i think there is still a small issue on this:

When using SerializerMethodField with TypeHiting, it generates Decimal fields as number, even with COERCE_DECIMAL_TO_STRING=True

class TestSerializer(serializers.Serializer):
    properties = serializers.SerializerMethodField()

    def get_properties(self, obj) -> List[Decimal]:
        return [Decimal(0), Decimal(1)]

And the generated schema:

Test{
properties*	[
readOnly: true
number($double)]
}

This is also happening with TypedDicts as the Type Hinting for the SerializerMethodField function.

Shouldn't it be handled the same way DecimalSerializer is handled, and the generated schema should be String?

Thanks

@tfranzel
Copy link
Owner Author

Hi,

I think this is the correct behavior. COERCE_DECIMAL_TO_STRING is a DRF setting and only applies to instances of DecimalField. The SerializerMethodField is unaffected by this.

I'm pretty certain your API will in fact return a JSON number and not a string if used like that. DRF's JSONEncoder will make it a float. You would need to do the string conversion yourself here (and use -> List[str]).

@gujacob
Copy link

gujacob commented Sep 24, 2022 via email

@tfranzel
Copy link
Owner Author

I see, but your change does not reflect the default behavior of DRF and so I suppose there is nothing to fix. spectacular cannot know what you change inside your JSONEncoder.

but this way I don't have the regex for numbers on it.

yeah that is a tradeoff. If you are hard set on that detail your would need to subclass AutoSchema and override _map_response_type_hint.

@gujacob
Copy link

gujacob commented Sep 25, 2022 via email

@gujacob
Copy link

gujacob commented Sep 25, 2022 via email

@XF-FW
Copy link

XF-FW commented Nov 6, 2022

Hey @gujacob . I found this thread while googling for a solution for a similar problem. Since I didn't find anything I sketched this field class, which I think has all the correct behaviors, including the correct generated schema:

class DecimalSerializerMethodField(s.DecimalField, s.SerializerMethodField):
    def __init__(
        self,
        max_digits: int | None,
        decimal_places: int | None,
        **kwargs: Any,
    ):
        try:
            method_name = kwargs.pop("method_name")
        except KeyError:
            method_name = None

        s.DecimalField.__init__(self, max_digits, decimal_places, **kwargs)
        s.SerializerMethodField.__init__(self, method_name=method_name, **kwargs)

    def to_representation(self, value: Any) -> Any:
        method_result = s.SerializerMethodField.to_representation(
            self, value  # type: ignore
        )
        decimal_result = s.DecimalField.to_representation(
            self, method_result  # type: ignore
        )

        return decimal_result

I've tried to fix the type: ignore, but I don't think it's 100% trivial. I think the stubs might require a small correction, by adding the type parameters to the Field here. This is only relevant, if you, like me, have an unhealthy relationship with mypy :)

Any updates / fixes to this snippet are appreciated, although it's working fine for my use-case.

Let me just finish, by apologizing to @tfranzel for putting this here, when this is not directly related to drf-spectacular.

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

No branches or pull requests

3 participants