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

Fix max length error on conlist with type int #902

Merged
merged 6 commits into from
Aug 23, 2023

Conversation

yohanvalencia
Copy link
Contributor

@yohanvalencia yohanvalencia commented Aug 19, 2023

Change Summary

The list length displayed an incorrect error message when constrained to only integer types

Example Code:

from pydantic_core import SchemaValidator, ValidationError

v = SchemaValidator(
    {
        'type': 'typed-dict',
        'fields': {
            'foo': {
                'type': 'typed-dict-field',
                'schema': {
                    'type': 'list',
                    'max_length': 4,
                    'items_schema': {'type': 'int'},
                },
            },
        },
    }
)

try:
    v.validate_python({'foo': list(range(9))})
except ValidationError as e:
    print(e)


Before: List should have at most 4 items after validation, not 5.
Now: List should have at most 4 items after validation, not 9.

Related issue number

fix pydantic/pydantic#7059

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @dmontagu

@yohanvalencia yohanvalencia marked this pull request as draft August 19, 2023 22:54
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #902 (5e139b0) into main (7f7d03d) will increase coverage by 0.05%.
Report is 9 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #902      +/-   ##
==========================================
+ Coverage   93.78%   93.84%   +0.05%     
==========================================
  Files         105      105              
  Lines       15331    15459     +128     
  Branches       25       25              
==========================================
+ Hits        14378    14507     +129     
+ Misses        947      946       -1     
  Partials        6        6              
Files Changed Coverage Δ
src/input/return_enums.rs 85.59% <100.00%> (+0.64%) ⬆️

... and 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f7d03d...5e139b0. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 19, 2023

CodSpeed Performance Report

Merging #902 will not alter performance

Comparing yvalencia91:max-length-error-with-type-int (5e139b0) with main (7f7d03d)

Summary

✅ 138 untouched benchmarks

@adriangb adriangb self-assigned this Aug 19, 2023
@@ -391,9 +396,9 @@ def f(v: int) -> int:
{
'type': 'too_long',
'loc': (),
'msg': 'List should have at most 10 items after validation, not 11',
'msg': 'List should have at most 10 items after validation, not 15',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the actual size of the list is 15 and not 11 I decided to change the values.

@yohanvalencia yohanvalencia marked this pull request as ready for review August 19, 2023 23:45
@yohanvalencia
Copy link
Contributor Author

Please review

@@ -86,7 +86,7 @@ def test_subclass_bytes(schema_type, input_value, expected_json):

v = s.to_python(input_value, mode='json')
assert v == expected_json
assert type(v) == str # noqa: E721
assert isinstance(v, str)
Copy link
Member

Choose a reason for hiding this comment

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

I see that this is also happening in #914. I might make a PR to make just this change so we avoid conflicts

Comment on lines +381 to +383
let capacity = self
.generic_len()
.unwrap_or_else(|| max_length.unwrap_or(DEFAULT_CAPACITY));
Copy link
Member

Choose a reason for hiding this comment

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

So if it's somethign that has a len (like a list) we use that, if it doesn't have a len we use DEFAULT_CAPACITY. That means that if you feed a generator in you'll end up with an error message that includes whatever DEFAULT_CAPACITY is, right?

Copy link
Contributor Author

@yohanvalencia yohanvalencia Aug 23, 2023

Choose a reason for hiding this comment

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

That's right, for the first part but for the generator I realise it wasn't working. The max_length need to be caught by a None condition. So if someone validates this:

from pydantic_core import SchemaValidator


def infinite_generator():
    i = 0
    while True:
        yield i
        i += 1


v = SchemaValidator({'type': 'list'})
v.validate_python(infinite_generator())

Then the error message will be:
Screenshot 2023-08-23 at 21 10 27

Otherwise, this will run into an infinite loop.

src/input/return_enums.rs Outdated Show resolved Hide resolved
Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Sorry meant to request changes in the earlier review.

@yohanvalencia
Copy link
Contributor Author

Please review

@adriangb adriangb enabled auto-merge (squash) August 23, 2023 19:26
@adriangb adriangb merged commit 882b57f into pydantic:main Aug 23, 2023
28 of 29 checks passed
@samuelcolvin
Copy link
Member

this is a mistake, we need to revert it ASAP.

@davidhewitt
Copy link
Contributor

Improved in #990

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.

Incorrect max_length error message
5 participants