Skip to content

Conversation

kc0506
Copy link
Contributor

@kc0506 kc0506 commented Jul 26, 2024

Change Summary

This is a fix for bug #9872. The annotation Sequence ignores discriminator metadata in certain cases. The generated schema has not correct python_schema where a union is presented rather than expected tagged-union. json_schema is normal.

The bug occurs in GenerateSchema._sequence_schema. In the json-or-python schema created, the underling reference of item_type_schema is the same, therefore the metadata dict is also shared. When calling apply_discriminators on the model schema, because json_schema is walked first, it has correct conversion from union to tagged-union. But the walk will pop the discriminator from metadata. Therefore when python_schema is walked, metadata is already empty, resulting in union, not tagged-union schema.

# `metadata` object has the same memory location
metadata = s.get('metadata', {})
discriminator = metadata.pop(CORE_SCHEMA_METADATA_DISCRIMINATOR_PLACEHOLDER_KEY, None)

I suspect there are more of these kinds of reference sharing problems in other parts of schema generation process. 🤔

Note the author mentioned that the bug disappears when adding model_validator to both models. This is because the procedure for applying discriminators is different. It is GenerateSchema._apply_discriminator_to_union that generates tagged-union in this case.

Related issue number

fix #9872

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Jul 26, 2024
Copy link

codspeed-hq bot commented Jul 26, 2024

CodSpeed Performance Report

Merging #9980 will not alter performance

Comparing kc0506:fix-sequence-ignore-discriminator (c53c5fb) with main (cfe6635)

Summary

✅ 14 untouched benchmarks

@kc0506 kc0506 changed the title Fix Sequence ignore discriminator Fix Sequence ignoring discriminator Jul 26, 2024
@sydney-runkle
Copy link
Contributor

@kc0506, thanks for your awesome work this past week. Btw, we have a slack channel to allow our contributors to ask questions + chat. You can join via this link, if you're interested!

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Great job with this fix - discriminator fixes are often hard to track down!

This fix reflects an impressive understanding of the schema generation process!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Union discrimination ignored in Sequence validation
2 participants