Skip to content

Ensure ValidationInfo.field_name is correct on validator reuse #1692

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

Merged
merged 9 commits into from
Apr 17, 2025

Conversation

DouweM
Copy link
Contributor

@DouweM DouweM commented Apr 15, 2025

Change Summary

See pydantic/pydantic#11737 (comment)

Companion PR in pydantic: pydantic/pydantic#11761

Related issue number

Part of pydantic/pydantic#11737

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: @sydney-runkle

…e called with correct ValidationInfo.field_name
@DouweM
Copy link
Contributor Author

DouweM commented Apr 15, 2025

@davidhewitt Please review!

cc @Viicos

Edit: Never mind, failing tests, looking

Copy link

codspeed-hq bot commented Apr 15, 2025

CodSpeed Performance Report

Merging #1692 will not alter performance

Comparing DouweM:validator-reuse-field-name (3d69981) with main (9a25aa6)

Summary

✅ 157 untouched benchmarks

@DouweM
Copy link
Contributor Author

DouweM commented Apr 15, 2025

I ended up changing some tests in 0fd7ef1 and df34451.

Both are similar to the issue described in pydantic/pydantic#11737 (comment): we now get ValidationInfo.field_name even if the validator wasn't explicitly created for a specific field, which is not always expected.

I think the changes in the second commit make sense as the validator acts on the field value, but not necessarily in first commit where the validator acts on the entire model.

I'm not sure the State based approach gives us the ability to drop the field_name in that specific case though. I think I'm OK with this change in behavior, interpreting field_name as "this is the field currently being validated" more than "this is the field the validator acts on".

@Viicos What do you thnk?

@DouweM
Copy link
Contributor Author

DouweM commented Apr 15, 2025

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, looking good!

@davidhewitt
Copy link
Contributor

We're hitting some leak/GC issues! https://github.com/pydantic/pydantic-core/actions/runs/14476861875/job/40604489000?pr=1692

These might be reproducable on main; it's possible it's something new with 3.13.3.

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

LGTM, with just the question of maybe we start cleaning up the existing state to keep things efficient.

The 3.13t tests will not block merge, so depending on whether we leave the above for a follow-up this is good to go 👍

@Viicos
Copy link
Member

Viicos commented Apr 15, 2025

LGTM, with just the question of maybe we start cleaning up the existing state to keep things efficient.

You mean having the state rebound only if we are certain a validator (with info) is going to be used?

@davidhewitt
Copy link
Contributor

LGTM, with just the question of maybe we start cleaning up the existing state to keep things efficient.

You mean having the state rebound only if we are certain a validator (with info) is going to be used?

Nope I just meant the deprecation which you agreed with. 👍

@@ -1954,7 +1954,7 @@ class NoInfoValidatorFunctionSchema(TypedDict):
class WithInfoValidatorFunctionSchema(TypedDict, total=False):
type: Required[Literal['with-info']]
function: Required[WithInfoValidatorFunction]
field_name: str
field_name: str # deprecated
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 think this TypedDict is only used for type hinting -- let me know if this is sufficient or if we should force a deprecation warning here as well somehow.

# insert_assert(reprs)
assert reprs == [
'ValidationInfo(config=None, context=None, data=None, field_name=None)',
"ValidationInfo(config=None, context=None, data=None, field_name='x')",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test explicitly tested the now-deprecated behavior of setting a field_name on the validator function schema. If we assume no one was directly constructing these instead of us, removing this should be fine.

I could also leave the test and wrap the call in pytest.warns, to verify the old behavior keeps working (which it does) -- let me know what you prefer.

@DouweM
Copy link
Contributor Author

DouweM commented Apr 15, 2025

@Viicos Pydantic is still passing field_name, so it makes sense that test-pydantic-integration is failing now that I've deprecated it.

What's the best way to update pydantic for the new pydantic-core, before this is even merged?

@Viicos Viicos changed the title Ensure ValidationInfo.field_name is correct on validator reuse Ensure ValidationInfo.field_name is correct on validator reuse Apr 16, 2025
@Viicos
Copy link
Member

Viicos commented Apr 16, 2025

I'm not sure the State based approach gives us the ability to drop the field_name in that specific case though. I think I'm OK with this change in behavior, interpreting field_name as "this is the field currently being validated" more than "this is the field the validator acts on".

@Viicos What do you thnk?

I think this is fine, it is actually not a change in behavior as it is already behaving as such (as per my comment). Would be great if this wasn't this way but anyway I don't think many users are relying on this field_name attribute.

What's the best way to update pydantic for the new pydantic-core, before this is even merged?

We usually skip it as it will work again once the pydantic-core version is bumped on pydantic.

@DouweM
Copy link
Contributor Author

DouweM commented Apr 16, 2025

We usually skip it as it will work again once the pydantic-core version is bumped on pydantic.

@Viicos OK, I'll get a PR up to stop passing the field_name argument in pydantic, so that we can bump pydantic-core in that same PR once this PR is merged, and ensure everything keeps working right.

@DouweM
Copy link
Contributor Author

DouweM commented Apr 16, 2025

Companion PR in Pydantic: pydantic/pydantic#11761

cc @Viicos

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

I think this is good, thanks 👍

@davidhewitt davidhewitt merged commit 4760f38 into pydantic:main Apr 17, 2025
26 of 29 checks passed
@DouweM
Copy link
Contributor Author

DouweM commented Apr 17, 2025

@Viicos Let me know when you've bumped the version here and I'll update the Pydantic PR to point at the new version!

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.

3 participants