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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/errors/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ error_types! {
error: {ctx_type: Cow<'static, str>, ctx_fn: cow_field_from_context<String, _>},
},
DateFromDatetimeParsing {
error: {ctx_type: String, ctx_fn: field_from_context},
error: {ctx_type: Cow<'static, str>, ctx_fn: cow_field_from_context<String, _>},
},
DateFromDatetimeInexact {},
DatePast {},
Expand Down
62 changes: 27 additions & 35 deletions src/validators/date.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,12 @@ impl Validator for DateValidator {
_definitions: &'data Definitions<CombinedValidator>,
_recursion_guard: &'s mut RecursionGuard,
) -> ValResult<'data, PyObject> {
let date = match input.validate_date(extra.strict.unwrap_or(self.strict)) {
let strict = extra.strict.unwrap_or(self.strict);
let date = match input.validate_date(strict) {
Ok(date) => date,
// if the date error was an internal error, return that immediately
Err(ValError::InternalErr(internal_err)) => return Err(ValError::InternalErr(internal_err)),
Err(date_err) => match self.strict {
// if we're in strict mode, we doing try coercing from a date
true => return Err(date_err),
// 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

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 🙈

Err(otherwise) => return Err(otherwise),
};
if let Some(constraints) = &self.constraints {
let raw_date = date.as_raw()?;
Expand Down Expand Up @@ -122,35 +118,31 @@ impl Validator for DateValidator {

/// In lax mode, if the input is not a date, we try parsing the input as a datetime, then check it is an
/// "exact date", e.g. has a zero time component.
fn date_from_datetime<'data>(
input: &'data impl Input<'data>,
date_err: ValError<'data>,
) -> ValResult<'data, EitherDate<'data>> {
///
/// Ok(None) means that this is not relevant to dates (the input was not a datetime nor a string)
fn date_from_datetime<'data>(input: &'data impl Input<'data>) -> Result<Option<EitherDate<'data>>, ValError<'data>> {
let either_dt = match input.validate_datetime(false, speedate::MicrosecondsPrecisionOverflowBehavior::Truncate) {
Ok(dt) => dt,
Err(dt_err) => {
return match dt_err {
ValError::LineErrors(mut line_errors) => {
// if we got a errors while parsing the datetime,
// convert DateTimeParsing -> DateFromDatetimeParsing but keep the rest of the error unchanged
for line_error in &mut line_errors {
match line_error.error_type {
ErrorType::DatetimeParsing { ref error, .. } => {
line_error.error_type = ErrorType::DateFromDatetimeParsing {
error: error.to_string(),
context: None,
};
}
_ => {
return Err(date_err);
}
}
}
Err(ValError::LineErrors(line_errors))
// if the error was a parsing error, update the error type from DatetimeParsing to DateFromDatetimeParsing
// and return it
Err(ValError::LineErrors(mut line_errors)) => {
if line_errors.iter_mut().fold(false, |has_parsing_error, line_error| {
if let ErrorType::DatetimeParsing { error, .. } = &mut line_error.error_type {
line_error.error_type = ErrorType::DateFromDatetimeParsing {
error: std::mem::take(error),
context: None,
};
true
} else {
has_parsing_error
}
other => Err(other),
};
}) {
return Err(ValError::LineErrors(line_errors));
}
return Ok(None);
}
// for any other error, don't return it
Err(_) => return Ok(None),
};
let dt = either_dt.as_raw()?;
let zero_time = Time {
Expand All @@ -161,7 +153,7 @@ fn date_from_datetime<'data>(
tz_offset: dt.time.tz_offset,
};
if dt.time == zero_time {
Ok(EitherDate::Raw(dt.date))
Ok(Some(EitherDate::Raw(dt.date)))
} else {
Err(ValError::new(ErrorTypeDefaults::DateFromDatetimeInexact, input))
}
Expand Down
19 changes: 19 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,25 @@ class ChosenPyAndJsonValidator(PyAndJsonValidator):
return ChosenPyAndJsonValidator


class StrictModeType:
def __init__(self, schema: bool, extra: bool):
assert schema or extra
self.schema = schema
self.validator_args = {'strict': True} if extra else {}


@pytest.fixture(
params=[
StrictModeType(schema=True, extra=False),
StrictModeType(schema=False, extra=True),
StrictModeType(schema=True, extra=True),
],
ids=['strict-schema', 'strict-extra', 'strict-both'],
)
def strict_mode_type(request) -> StrictModeType:
return request.param


@pytest.fixture
def tmp_work_path(tmp_path: Path):
"""
Expand Down
16 changes: 8 additions & 8 deletions tests/validators/test_date.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ def test_date_json(py_and_json: PyAndJson, input_value, expected):
],
ids=repr,
)
def test_date_strict(input_value, expected):
v = SchemaValidator({'type': 'date', 'strict': True})
def test_date_strict(input_value, expected, strict_mode_type):
v = SchemaValidator({'type': 'date', 'strict': strict_mode_type.schema})
if isinstance(expected, Err):
with pytest.raises(ValidationError, match=re.escape(expected.message)):
v.validate_python(input_value)
v.validate_python(input_value, **strict_mode_type.validator_args)
else:
output = v.validate_python(input_value)
output = v.validate_python(input_value, **strict_mode_type.validator_args)
assert output == expected


Expand All @@ -148,13 +148,13 @@ def test_date_strict(input_value, expected):
('1654646400', Err('Input should be a valid date [type=date_type')),
],
)
def test_date_strict_json(input_value, expected):
v = SchemaValidator({'type': 'date', 'strict': True})
def test_date_strict_json(input_value, expected, strict_mode_type):
v = SchemaValidator({'type': 'date', 'strict': strict_mode_type.schema})
if isinstance(expected, Err):
with pytest.raises(ValidationError, match=re.escape(expected.message)):
v.validate_json(input_value)
v.validate_json(input_value, **strict_mode_type.validator_args)
else:
output = v.validate_json(input_value)
output = v.validate_json(input_value, **strict_mode_type.validator_args)
assert output == expected


Expand Down