Skip to content

Conversation

Viicos
Copy link
Member

@Viicos Viicos commented Jan 27, 2025

Since core schema validation was disabled, constraints such as gt are not coerced to a Decimal instance if provided as e.g. an int.

Partially addresses #11341.

Change Summary

Related issue number

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Since core schema validation was disabled, constraints such
as `gt` are not coerced to a `Decimal` instance if provided
as e.g. an `int`.
@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Jan 27, 2025
Copy link

Deploying pydantic-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 54f8b7d
Status: ✅  Deploy successful!
Preview URL: https://f5b318da.pydantic-docs.pages.dev
Branch Preview URL: https://decimal-constraints.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hq bot commented Jan 27, 2025

CodSpeed Performance Report

Merging #11350 will not alter performance

Comparing decimal-constraints (54f8b7d) with main (ea3f18d)

Summary

✅ 45 untouched benchmarks

Copy link
Contributor

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pydantic/_internal
  _known_annotated_metadata.py
Project Total  

This report was generated by python-coverage-comment-action

Comment on lines +223 to +226
if schema_type == 'decimal' and constraint in {'multiple_of', 'le', 'ge', 'lt', 'gt'}:
schema[constraint] = Decimal(value)
else:
schema[constraint] = value
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to put this in pydantic-core rather than coupling this to what pydantic-core expects?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good idea

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, only concern is that as is, the core schema will not comply to the corresponding typed dict definition (e.g. with f: Decimal = Field(gt=1), the core_schema is {'type': 'decimal', 'gt': 1}).

Unless we change the constraints type on the typed dict to be Decimal | float, and then do the coercion during the validator build in pydantic-core?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems ok to me to make the core schema definition more lax in this way? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum but then how easy will it be to do so e.g. for date constraints? As ideally we'd want to do Pydantic coercion on it. Is it possible to perform such validation/coercion during pydantic-core schema build?

I know the current change looks ugly, but this is inherent to the current implementation of the known metadata application that has several flaws.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about proceeding as is, can you open an issue in core for these cases and I'll propose it there another time?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sydney-runkle
Copy link
Contributor

I'm fine with merging as is 👍

@Viicos Viicos merged commit 5f40fe4 into main Jan 28, 2025
56 checks passed
@Viicos Viicos deleted the decimal-constraints branch January 28, 2025 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants