Skip to content

Raise ValidationError (not ValueError) for NaN in TimeDelta - #3004

Open
vidigoat wants to merge 2 commits into
marshmallow-code:devfrom
vidigoat:fix/timedelta-nan-validationerror
Open

Raise ValidationError (not ValueError) for NaN in TimeDelta#3004
vidigoat wants to merge 2 commits into
marshmallow-code:devfrom
vidigoat:fix/timedelta-nan-validationerror

Conversation

@vidigoat

Copy link
Copy Markdown

Problem

A TimeDelta field raises a bare ValueError (instead of ValidationError) when it receives NaN, so loading untrusted input can crash with an exception that isn't part of marshmallow's public error contract:

from marshmallow import Schema, fields

class S(Schema):
    duration = fields.TimeDelta()

S().load({"duration": "nan"})
# ValueError: cannot convert float NaN to integer   <- leaks; expected ValidationError

"inf" / "-inf" already behave correctly (they raise ValidationError), which makes the NaN case an inconsistency rather than intended behaviour.

Cause

TimeDelta._deserialize only catches OverflowError around dt.timedelta(**kwargs):

try:
    return dt.timedelta(**kwargs)
except OverflowError as error:
    raise self.make_error("invalid") from error

float("nan") passes the earlier float(value) guard, but dt.timedelta(nan) raises ValueError — not OverflowError — so it escapes uncaught. (inf/-inf work because timedelta raises OverflowError for those.)

Fix

Catch ValueError alongside OverflowError, so NaN produces the field's standard "Not a valid period of time." error like every other invalid input:

except (OverflowError, ValueError) as error:
    raise self.make_error("invalid") from error

Extended the existing parametrized test_invalid_timedelta_field_deserialization with "nan", "inf", "-inf". Full test suite passes locally.


Disclosure: I used an LLM to help find and draft this fix; I've reviewed every line, reproduced the bug, and run the suite myself.

TimeDelta._deserialize only caught OverflowError around
dt.timedelta(**kwargs). float("nan") passes the earlier float() guard,
but dt.timedelta(nan) raises ValueError, which escaped uncaught -- so
loading {"duration": "nan"} from untrusted input raised a raw
ValueError instead of a ValidationError, breaking the contract that all
bad field input surfaces as ValidationError. (inf/-inf already worked
because timedelta raises OverflowError for those.)

Catch ValueError alongside OverflowError so NaN yields the field's
standard 'Not a valid period of time.' error. Extends the existing
parametrized invalid-input test with nan/inf/-inf.

@lafrech lafrech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
Just a few minor comments. LLM tend to be too verbose.

Comment thread src/marshmallow/fields.py
@vidigoat

Copy link
Copy Markdown
Author

Comment shortened, thanks.

On the commit message: I wasn't able to rewrite the original commit here. If you squash-merge, feel free to use just the PR title as the message.

@lafrech lafrech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants