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

Error serializing MaxValueValidator.limit_value of type datetime.timedelta #388

Closed
voyc-jean opened this issue May 17, 2021 · 11 comments
Closed
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@voyc-jean
Copy link

Describe the bug
Similar to #357, the yaml renderer is unable to serialize a MaxValueValidator.limit_value of type datetime.timedelta.

Using the JSON renderer solves the issue.

To Reproduce
Add a MaxValueValidator to a model field with a limit_value type of datetime.timedelta:

import datetime
from django.db import models

class MyModel(models.Model):
    duration = models.DurationField(
        default=datetime.timedelta(seconds=10),
        validators=[
            MaxValueValidator(
                limit_value=datetime.timedelta(seconds=3600),
                message='Limit message.',
            ),
        ],
    )

Raised exception:

  File "/usr/local/lib/python3.8/site-packages/yaml/representer.py", line 231, in represent_undefined
    raise RepresenterError("cannot represent an object", data)
yaml.representer.RepresenterError: ('cannot represent an object', datetime.timedelta(seconds=3600))

Expected behavior
The yaml render should be able to serialize datetime.timedelta values.

@tfranzel
Copy link
Owner

hi @voyc-jean,

nice catch! yes that is a bug. i had a quick look into it and found that DRF's JSONEncoder serializes timedelta as str(obj.total_seconds()), while Django's DjangoJSONEncoder actually produces '{}P{}DT{:02d}H{:02d}M{:02d}{}S' (ISO8601) via duration_iso_string.

so you would get total seconds from DRF without further modification. for parity we should probably do the same for yaml.

furthermore, i think we should introduce a hook into our normalize_result_object to allow for custom behavior before the actual encoders are hit. this would allow to account for user modified encoders.

@tfranzel tfranzel added the bug Something isn't working label May 18, 2021
@tfranzel
Copy link
Owner

this should get rid of the exception but i think we have one issue left. the validator fills minimum/maximum which is required to be a number in the spec and thus my schema validation failed afterwards. can you confirm this?

@voyc-jean
Copy link
Author

Hi @tfranzel,

Thanks a lot for the quick solution! I can confirm that this gets rid of the exception, though after looking into this further I think there might be more to the issue.

I've just tested this and you're correct that DRF's JSONEncoder responds with str(obj.total_seconds()).

However, the DRF DurationField serializes these values to a 'duration string' by using django.utils.duration.duration_string, formatting the value for representation as: [DD] [HH:[MM:]]ss[.uuuuuu].

Values are parsed back from 'duration strings' to timedelta by DRF's django.utils.dateparse.parse_duration.

I think the schema (including the minimum / maximum) should reflect the same?

Thanks.

@tfranzel
Copy link
Owner

ok so to summarize. depending on how one would go through the code, you could end up with

  • duration_iso_string (raw via django encoder)
  • duration_string (via field)
  • str(total_seconds) (raw via DRF encoder)

the real issue i see is that maximum/minimum is required to be a number in the OpenAPI spec. it does not apply to anything else, which is why validation failed. so none of the above outcomes are valid values wrt OpenAPI.

bummer! the least broken thing could be to remove it altogether for non-numbers.

@voyc-jean
Copy link
Author

ok so to summarize. depending on how one would go through the code, you could end up with

  • duration_iso_string (raw via django encoder)
  • duration_string (via field)
  • str(total_seconds) (raw via DRF encoder)

the real issue i see is that maximum/minimum is required to be a number in the OpenAPI spec. it does not apply to anything else, which is why validation failed. so none of the above outcomes are valid values wrt OpenAPI.

bummer! the least broken thing could be to remove it altogether for non-numbers.

I see this has been fixed now in release 0.17.0, thanks @tfranzel!

Regarding the minumum/maximum issue, I agree that the best course of action would be to remove them for non-number values. Should I create a separate issue for this?

@tfranzel
Copy link
Owner

tfranzel commented Jun 7, 2021

you're welcome! i think we can reuse this issue, as it is still closely related to the remaining issue.

@ngnpope
Copy link
Contributor

ngnpope commented Sep 28, 2021

@tfranzel I guess this one can be labelled fix confirmation pending after 37ee061?

@tfranzel tfranzel added enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending and removed enhancement New feature or request labels Sep 28, 2021
@tfranzel
Copy link
Owner

had the exact same thought yesterday. 👍 thx

@dirodriguezm
Copy link

Hi, I'm using drf's DurationField with choices that are datetime.timedelta objects and I get the error Object of type timedelta is not JSON serializable. Is this issue related to my problem ?

@tfranzel
Copy link
Owner

@dirodriguezm please post a code example. serializers.DurationField() does not accept choices. it's hard to tell if this is related without a snippet that produces that error. also please make sure to update to the most recent version. further fixes are on master also.

@tfranzel
Copy link
Owner

tfranzel commented Oct 3, 2021

closing this issue for now. feel free to comment if anything is missing or not working and we will follow-up.

@dirodriguezm feel free to open a new issue if your problem persists.

@tfranzel tfranzel closed this as completed Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

4 participants