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 inconsistency with strict mode of date validation #870

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

davidhewitt
Copy link
Contributor

Change Summary

Stops strict dates from accepting timestamp strings, if strict=True at the extra level.

Related issue number

Related to pydantic/pydantic#7039

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

false => date_from_datetime(input, date_err),
}?,
// if the error was a parsing error, in lax mode we allow datetimes at midnight
Err(line_errors @ ValError::LineErrors(..)) if !strict => date_from_datetime(input)?.ok_or(line_errors)?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With apologies I couldn't help golfing this line, with the result of modifying date_from_datetime a little 🙈

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #870 (eebccb5) into main (3f7df80) will increase coverage by 0.00%.
The diff coverage is 95.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #870   +/-   ##
=======================================
  Coverage   94.05%   94.06%           
=======================================
  Files         102      102           
  Lines       15028    15023    -5     
  Branches       25       25           
=======================================
- Hits        14135    14131    -4     
+ Misses        887      886    -1     
  Partials        6        6           
Files Changed Coverage Δ
src/validators/date.rs 96.26% <95.00%> (+0.72%) ⬆️
src/errors/types.rs 99.49% <100.00%> (ø)

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 3f7df80...eebccb5. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 9, 2023

CodSpeed Performance Report

Merging #870 will degrade performances by 43.7%

Comparing dh/strict-date-bug (eebccb5) with main (3f7df80)

Summary

❌ 1 regressions
✅ 134 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main dh/strict-date-bug Change
test_tagged_union_int_keys_python 33 µs 58.6 µs -43.7%

// otherwise, try creating a date from a datetime input
false => date_from_datetime(input, date_err),
}?,
// if the error was a parsing error, in lax mode we allow datetimes at midnight
Copy link
Contributor

Choose a reason for hiding this comment

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

let date = ... allow datetimes at midnight

This is so strange 😅 what was the original reasoning for allowing this?

Its also inconsistent with Python pydantic/pydantic#7039 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Neither its valid json for a date property in a json schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this behaviour is a bit surprising. It originates from #77 so we will have to ask @samuelcolvin for the motivation and if we can remove it!

Copy link
Member

Choose a reason for hiding this comment

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

Yup we need to evaluate that but it'll be in a separate discussion / PR

@davidhewitt davidhewitt merged commit c94fa15 into main Aug 9, 2023
29 of 30 checks passed
@davidhewitt davidhewitt deleted the dh/strict-date-bug branch August 9, 2023 21:52
@ariofrio
Copy link

ariofrio commented Jun 15, 2024

Is there a way to implement custom serialization and deserialization code for all datetime fields used in a project or deriving from a BaseModel? For example, one user might want to use unix timestamps, while another would prefer ISO dates as strings. This is particularly important now that datetime is not allowed by default when using strict mode.

@davidhewitt
Copy link
Contributor Author

Currently you'd need to use a type alias everywhere like Annotated[datetime, MyCustomValidation]. In the wider context, I think you're probably asking for pydantic/pydantic#3208

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.

4 participants