Skip to content
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

Fixed #34816 -- Fixed GenericForeignKey crash when checking cached for primary keys with different types. #17234

Merged
merged 8 commits into from
Sep 8, 2023

Conversation

oguzhanakan0
Copy link
Contributor

@oguzhanakan0 oguzhanakan0 commented Sep 7, 2023

Following the suggestion in ticket/34816, changed the logic while accessing a GenericForeignKey to skip checking PK match if ContentType IDs do not match.

Test function is placed in tests/generic_relations_regress.py.

Unit test output before the patch:

Testing against Django installed in '/Users/oguzhanakan/Desktop/oakan/django/django' with up to 8 processes
Found 27 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
..........................E
======================================================================
ERROR: test_ticket_34816_generic_fk_crash (generic_relations_regress.tests.GenericRelationTests.test_ticket_34816_generic_fk_crash)
Ensure pk type change is handled when using GenericForeignKey.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "../django/django/db/models/fields/__init__.py", line 2135, in to_python
    return int(value)
           ^^^^^^^^^^
ValueError: invalid literal for int() with base 10: 'some test'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "../tests/generic_relations_regress/tests.py", line 351, in test_ticket_34816_generic_fk_crash
    self.assertEqual(type(charlink.content_object), Board)
                          ^^^^^^^^^^^^^^^^^^^^^^^
  File "../django/contrib/contenttypes/fields.py", line 245, in __get__
    pk_match = rel_obj._meta.pk.to_python(pk_val) == rel_obj.pk
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "../django/db/models/fields/__init__.py", line 2137, in to_python
    raise exceptions.ValidationError(
django.core.exceptions.ValidationError: ['“some test” value must be an integer.']

Everything seems to be working, thanks to rlaager for the suggestion and @felixxm for the test function.

P.S. This is my first contribution so please let me know if I missed anything. Thanks-

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, make sure change the MR description to point at 34816 and not 34813.

tests/generic_relations_regress/tests.py Outdated Show resolved Hide resolved
tests/generic_relations_regress/tests.py Outdated Show resolved Hide resolved
tests/generic_relations_regress/tests.py Outdated Show resolved Hide resolved
@oguzhanakan0 oguzhanakan0 changed the title Fixed #34813 -- Added logic to skip PK match check if ContentType IDs do not match in GenericForeignKey field Fixed #34816 -- Added logic to skip PK match check if ContentType IDs do not match in GenericForeignKey field Sep 7, 2023
oguzhanakan0 and others added 3 commits September 7, 2023 18:30
Co-authored-by: Simon Charette <charette.s@gmail.com>
Co-authored-by: Simon Charette <charette.s@gmail.com>
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@oguzhanakan0 Thanks 👍 I left comments.

tests/generic_relations_regress/tests.py Outdated Show resolved Hide resolved
tests/generic_relations_regress/tests.py Outdated Show resolved Hide resolved
@felixxm felixxm changed the title Fixed #34816 -- Added logic to skip PK match check if ContentType IDs do not match in GenericForeignKey field Fixed #34816 -- Fixed GenericForeignKey crash when checking cached for primary keys with different types. Sep 8, 2023
oguzhanakan0 and others added 2 commits September 8, 2023 08:58
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@oguzhanakan0 Thanks 👍 Welcome aboard ⛵

@felixxm felixxm merged commit e41f9f9 into django:main Sep 8, 2023
8 checks passed
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