Skip to content

fix: submodel fields with wrap validator affect smart union selection #1700

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

weiliddat
Copy link

@weiliddat weiliddat commented Apr 18, 2025

Change Summary

  • Keep fields_set_count state when validating submodels

Related issue number

Fixes pydantic/pydantic#11752

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: @davidhewitt

Comment on lines +289 to +290
// state.exactness = self.exactness;
// state.fields_set_count = self.fields_set_count;
Copy link
Author

Choose a reason for hiding this comment

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

@Viicos I'm not sure that these are being used at the moment for validate_assignment. I don't see any explicit use for these — I've also added an additional assertion in the added test.

But maybe Chesterton's Fence in effect 😄

Let me know if I should keep it

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, I'm tempted to leave it and add a TODO is it necessary to report the state for validate_assignment?, but is this TODO going to be solved anytime.. 😄 thoughts @davidhewitt?

Copy link
Author

Choose a reason for hiding this comment

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

I'll dig into it a bit more to see if this state is necessary on validate_assignment, but if I can't figure it out this week then I'll just add the TODO and leave it

@weiliddat
Copy link
Author

please review

Copy link

codspeed-hq bot commented Apr 18, 2025

CodSpeed Performance Report

Merging #1700 will not alter performance

Comparing weiliddat:fix/wrap-validator-smart-union (0bb8a07) with main (3414703)

Summary

✅ 157 untouched benchmarks

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WrapValidator annotation changes smart union score
3 participants