fix: merge field constraints across all metadata items#2036
Closed
anxkhn wants to merge 1 commit into
Closed
Conversation
get_field_metadata returned the first field.metadata item matching (PydanticMetadata, MaxLen) and stopped. Pydantic v2 spreads constraints across several metadata items, so when a matching item without the length or precision (for example a pattern or allow_inf_nan constraint) precedes the item that carries max_length, max_digits, or decimal_places, those values were silently dropped and the generated SQLAlchemy column was unbounded (VARCHAR / NUMERIC with no length or precision). Scan all matching metadata items and merge max_length, max_digits, and decimal_places into a single result, keeping the first non-None value for each attribute. Single-metadata fields are unaffected. Add a regression test covering the order-dependent str and Decimal cases. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Contributor
|
This was marked as potentially AI generated and will be closed now. If this is an error, please provide additional details, make sure to read the docs about contributing and AI. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
get_field_metadatareturns the firstfield.metadataitem that is aPydanticMetadataorMaxLenand stops there. Pydantic v2 spreads a field'sconstraints across several separate metadata items, so when a matching item that
does not carry the length or precision precedes the one that does,
max_length,max_digits, anddecimal_placesare silently dropped. The generated SQLAlchemycolumn then comes out unbounded (
VARCHAR/NUMERICwith no length orprecision) even though the field declared a limit.
This is order dependent: putting the size constraint first happens to work, and
reordering the annotation makes the same field lose its constraint, which is
surprising.
Reproduction
On
main:Swapping the annotation order so the size constraint comes first yields the
correct
10/8/2, which confirms the loss is purely order dependent.The
Decimalcase is worse than it first looks: Pydantic storesmax_digitsand
decimal_placesin two separate metadata items, so before this change thefirst-match
returncould keep the precision but drop the scale (or vice versa)regardless of annotation order.
Fix
Instead of returning the first matching metadata item, scan all matching items
and merge
max_length,max_digits, anddecimal_placesinto a single result,keeping the first non-None value found for each attribute.
FakeMetadata(already the empty fallback here, and declaring exactly those three attributes)
is reused as the aggregator, so the change stays small.
The only consumer,
get_sqlalchemy_type, reads just those three attributes viagetattr(..., None), so fields that carry all their constraints on a singlemetadata item behave exactly as before. Only fields whose constraints were split
across items (and were therefore already broken) change behavior.
Tests
Adds
test_metadata_constraints_not_shadowed_by_earlier_metadata, covering boththe
str(max_lengthafterpattern) andDecimal(max_digits/decimal_placesafterallow_inf_nan) cases. It fails onmain(
assert None == 10) and passes with this change. The full suite (pytest tests) and the linters (ty,ruff check,ruff format --check) pass locally.