Skip to content

Fix UniqueTogetherValidator to handle fields w/ source attr#9688

Merged
auvipy merged 3 commits intoencode:masterfrom
joshuadavidthomas:unique-constraint-source
Apr 23, 2025
Merged

Fix UniqueTogetherValidator to handle fields w/ source attr#9688
auvipy merged 3 commits intoencode:masterfrom
joshuadavidthomas:unique-constraint-source

Conversation

@joshuadavidthomas
Copy link
Contributor

@joshuadavidthomas joshuadavidthomas commented Apr 11, 2025

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

joshuadavidthomas commented Apr 14, 2025

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
Copilot AI reviewed Apr 16, 2025
View reviewed changes
Copy link

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 = {

auvipy
auvipy requested changes Apr 16, 2025
View reviewed changes
browniebroke
browniebroke reviewed Apr 16, 2025
View reviewed changes
@joshuadavidthomas
Copy link
Contributor Author

joshuadavidthomas commented Apr 22, 2025

@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.

auvipy
auvipy approved these changes Apr 23, 2025
View reviewed changes
Copy link
Collaborator

@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
Collaborator

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

joshuadavidthomas commented Apr 23, 2025

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

@joshuadavidthomas joshuadavidthomas deleted the unique-constraint-source branch July 4, 2025 02:18
@rick2ricks
Copy link

rick2ricks commented Aug 13, 2025

I am not sure if it is intended but some of my tests started to fail because the validator is ignoring the serializer field "required=False" argument.

I fixed by replacing the argument with default=None.

@joshuadavidthomas
Copy link
Contributor Author

joshuadavidthomas commented Aug 14, 2025

I am not sure if it is intended but some of my tests started to fail because the validator is ignoring the serializer field "required=False" argument.

I fixed by replacing the argument with default=None.

@rick2ricks This PR is probably not the best place to report this. I'd write up a small reproduction or test case showing this behavior and create a new issue.

@encode encode locked as resolved and limited conversation to collaborators Aug 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@browniebroke browniebroke browniebroke left review comments

Copilot code review Copilot Copilot left review comments

@auvipy auvipy auvipy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joshuadavidthomas @auvipy @rick2ricks @browniebroke