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

GenericForeignKey not accounted for in baker._skip_field() #416

Closed
paduszyk opened this issue May 23, 2023 · 5 comments
Closed

GenericForeignKey not accounted for in baker._skip_field() #416

paduszyk opened this issue May 23, 2023 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@paduszyk
Copy link

When trying to bake a model with mandatory generic relation and set _fill_optional=True at the same time, I get AttributeError due to invalid reference to generic FK field's null attribute.

If I add GenericForeignKey to the tuple ref. below (followed by its import, of course), the problem disappears.

if isinstance(field, (AutoField, GenericRelation, OrderWrt)):

I guess the problem is that generic FK is identified as nullable, thus optional... Anyone can explain why this has not been done yet?

@paulredman-
Copy link

We also have this problem, and are currently pinned to version <1.3 because of it. Would a patch containing the above change be acceptable? What else would be needed?

Thanks in advance!
Paul

@amureki amureki added the bug Something isn't working label Jul 17, 2023
@amureki
Copy link
Collaborator

amureki commented Jul 17, 2023

Greetings, and sorry for the radio silence!

I'd happily accept the patch with the test (if possible). Otherwise, I could tackle this some time at the beginning of the next month.

@paulredman-
Copy link

Rust, Many thanks for your quick response. Paul

@amureki
Copy link
Collaborator

amureki commented Aug 14, 2023

@paduszyk @paulredman- I got a bit of time to dig into this issue.
GenericForeignKey is not included in _skip_field() logic, as it is (and I believe, should be) allowed to be overwritten.

For example, there is a test TestFillingGenericForeignKeyField.test_iteratively_filling_generic_foreign_key_field that will fail in case we do that.

After playing around a bit, I have some kind of a workaround to cover the case described here: #438
(please, check out the test and correct me if I got it wrong).

While this solution works, I do not think it is the right way of solving this and would need to dig more.

@amureki
Copy link
Collaborator

amureki commented Aug 9, 2024

A bugfix for this was released in 1.18.3.

@amureki amureki closed this as completed Aug 9, 2024
amureki added a commit that referenced this issue Aug 9, 2024
* origin/main:
  Bump 1.19.0
  Fix #483 -- Add Django 5.1 support (#485)
  Bump 1.18.3
  Refs #416 -- Allow combination of GFK and `_fill_optional` (#438)
  Bump 1.18.2
  Update ruff CI syntax (#481)
  Fix #28 -- allow make_recipe to work with _quantity (#480)
  Bump 1.18.1
  Replace expensive `count()` with cheap `exists()` (#478)
  Update CI Python version to 3.12
  Delete hard action requirement for a changelog
  Bump 1.18.0
  Fix #265 -- drop hard dependency on `contenttypes` framework (#476)
  Bump GitHub Actions artifacts to v4 (#470)
  Drop Django 3.2 support (#475)
  Bump actions/setup-python from 4 to 5 (#466)
  Drop Django 4.1 support (reached end of life) (#465)
  Support Django 5.0 (#464)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants