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

✨ Add JSON-compatible float constraints for NaN and Inf #3994

Merged
merged 6 commits into from Aug 22, 2022

Conversation

tiangolo
Copy link
Member

@tiangolo tiangolo commented Apr 16, 2022

Change Summary

✨ Add JSON-compatible float constraints for NaN and Inf (+inf, -inf).

Full discussion in FastAPI: tiangolo/fastapi#4589

Short example

from typing import List

from fastapi import FastAPI
from pydantic import BaseModel

app = FastAPI()


class DataModel(BaseModel):
    a_string_value: str
    a_float_value: float


stored_objects: List[DataModel] = []


@app.get("/", response_model=List[DataModel])
async def get_objects():
    return stored_objects


@app.post("/")
async def save_object(to_save: DataModel):
    return stored_objects.append(to_save)

Then:

$ curl localhost:8000 -H 'Content-Type: application/json' -d '{"a_string_value": "object 1", "a_float_value": "NaN"}'
$ curl localhost:8000

That creates an InternalServerError with ValueError: Out of range float values are not JSON compliant.

Reason

Pydantic parses for a float field any string that could be parsed by Python's float, including "NaN". But Starlette encodes JSON responses with json.dumps(allow_nan=False). This means that Pydantic will "successfully" parse the "NaN" string as a Python float (which is not valid JSON but is valid Python and valid JavaScript), but then the response will not be able to convert the values back to JSON.

Alternatives

Change the response in Starlette/FastAPI

Changing the behavior in the response in Starlette (or FastAPI) to use json.dumps(allow_nan=True) (the default) would convert those float("nan") values to JSON null, the equivalent of Python None.

But that would mean that any type annotated as float would implicitly actually be Union[float, None] when converted to JSON. But that would be counterintuitive as static analysis (mypy, the editor, autocompletion, inline errors, TypeScript automatically generated clients, etc) would expect a float to be just float, not that in a corner case it could be None/null.

Accept this PR and have the new Pydantic types

If this PR is accepted, then my plan would be to document in FastAPI that using float in Python has those caveats, and it might be better to use the new Pydantic's JSONFloat that ensures compatibility with JSON.

Change the default behavior of float

Currently, there are no tests about how float('NaN'), float('+inf'), and float('-inf') should behave (although there's the current implicit behavior of accepting them).

There could be the option to require floats to not be NaN, +inf, or -inf, the same way that currently Decimals are treated. Currently, there are even tests that check that Decimal('NaN') is disallowed. So it might not be crazy to replicate the same behavior for floats.

Related issue number

Related to this issue: #1779 in particular to this comment: #1779 (comment)

I wouldn't say this directly closes that issue, but it's indeed related.

It is also related to this FastAPI issue: tiangolo/fastapi#4589

Next

I want to gauge the interest/acceptability of this or the alternatives before writing the docs.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@tiangolo
Copy link
Member Author

please review

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Otherwise I'm happy with this idea, needs some docs.

pydantic/pydantic-core#224 to remind us we need to update core to support this.

Comment on lines 235 to 236
allow_nan: bool = None,
allow_inf: bool = None,
Copy link
Member

Choose a reason for hiding this comment

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

My only question is: do we need both of these options?

I can't imagine a scenario where we want to allow inf but not nan or visa. versa.???

I would say we should have one allow_nan_inf argument for clarity.

@tiangolo what do you think?

@samuelcolvin
Copy link
Member

please update.

@samuelcolvin
Copy link
Member

To match pydantic/pydantic-core#228, we need one option allow_nan_inf, default to True.

@samuelcolvin
Copy link
Member

@tiangolo you get special treatment, I'll try to fix this for you today.


Notice

See twitter 🐦, I've you'd like this to be included in V1.10, please fix it and request a review TODAY.

Or if you need this in V1.10 but don't have time to complete it (or aren't the author), please comment here and on #4324.

@samuelcolvin
Copy link
Member

should be mostly there, just needs some docs.

@tiangolo @PrettyWood please review.

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

LGTM! Should we add also this option at config level? I feel like it's something you often want enabled or disabled for the whole model

@samuelcolvin
Copy link
Member

samuelcolvin commented Aug 22, 2022

Good point, I'll do that now. It'll also match V2.

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Great

def float_finite_validator(v: 'Number', field: 'ModelField', config: 'BaseConfig') -> 'Number':
allow_inf_nan = getattr(field.type_, 'allow_inf_nan', None)
if allow_inf_nan is None:
allow_inf_nan = config.allow_inf_nan
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Amazing! Thanks for taking over it @samuelcolvin! 🙇 LGTM 🚀

@samuelcolvin samuelcolvin merged commit 8dade7e into pydantic:main Aug 22, 2022
@samuelcolvin
Copy link
Member

thanks again for this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants