Skip to content

Fix UniqueTogetherValidator to handle fields w/ source attr #9688

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 3 commits into from
Apr 23, 2025

Conversation

joshuadavidthomas
Copy link
Contributor

This pull request introduces a failing test case to demonstrate a KeyError encountered during validation when a ModelSerializer field uses the source attribute and the corresponding model field is part of a UniqueConstraint.

When a ModelSerializer field specifies a source attribute to map to a model field with a different name (e.g., serializer_field = serializers.CharField(source="model_field_name")), and model_field_name is included in a UniqueConstraint, the validation process incorrectly attempts to access the validated data using the serializer field name (serializer_field) instead of the model field name (model_field_name) specified by source.

This leads to a KeyError within the validator, as the serializer field name is not present in the dictionary being checked at that stage. The relevant part of the traceback is:

tests/test_validators.py:702: in test_unique_constraint_source
    assert serializer.is_valid()
rest_framework/serializers.py:225: in is_valid
    self._validated_data = self.run_validation(self.initial_data)
rest_framework/serializers.py:446: in run_validation
    self.run_validators(value)
rest_framework/serializers.py:479: in run_validators
    super().run_validators(to_validate)
rest_framework/fields.py:551: in run_validators
    validator(value, self)
rest_framework/validators.py:191: in __call__
    condition_kwargs = {source: attrs[source] for source in self.condition_fields}
E   KeyError: 'raceName'

Note: I just added the test case at the very end of the class purely so I could quickly get a reproduction of this issue. Open to suggestions regarding its placement or structure if a different organization is preferred!

@joshuadavidthomas joshuadavidthomas changed the title Add test for UniqueConstraint validation w/ source attribute Fix UniqueTogetherValidator to handle fields w/ source attr Apr 14, 2025
@joshuadavidthomas
Copy link
Contributor Author

Alright, I have a fix in for the test. I tried to keep the fix localized and only change what is needed to fix the test, so if I need to restructure or extract some of the logic out for clarity, just let me know!

@auvipy auvipy requested review from auvipy and Copilot April 16, 2025 04:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

rest_framework/validators.py:191

  • The updated mapping now relies on serializer.fields[field_name].source being present in attrs. Consider ensuring that this key exists or handling the potential absence to avoid another KeyError.
condition_kwargs = {

@joshuadavidthomas
Copy link
Contributor Author

@auvipy @browniebroke Alright, I've got that change in. Is there anything else needed for this PR?

I noticed there's no CHANGELOG file and that it seems to be tracked in either docs/community/<version>-announcement.md or docs/community/release-notes.md, but no place for unreleased changes (unless I'm just obtuse and missing something). If this something I don't need to worry about, that's okay too ☺️. Just trying to be a bit pro-active to help out.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

it looks better now

@auvipy auvipy merged commit 5439967 into encode:master Apr 23, 2025
7 checks passed
@auvipy
Copy link
Member

auvipy commented Apr 23, 2025

If this something I don't need to worry about, that's okay too ☺️. Just trying to be a bit pro-active to help out.

you don't have to worry about it. thanks a lot for your work on this PR

@joshuadavidthomas
Copy link
Contributor Author

@auvipy @browniebroke Thanks for y'all's help and reviews on this! 💚

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.

3 participants