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

fix: let use a default value for foreignkey model field #901

Merged
merged 1 commit into from
Mar 4, 2023

Conversation

freddez
Copy link
Contributor

@freddez freddez commented Dec 17, 2022

If there is a default value in a model foreign key ('0' im my case), the actual code calls field.to_representation(field.default) and tries to interpret field.default.pk and raises an error. This fix let use raw default value.

See Django documentation : https://docs.djangoproject.com/en/4.1/ref/models/fields/#default

For fields like ForeignKey that map to model instances, defaults should be the value of the field they reference (pk unless to_field is set) instead of model instances.

@tfranzel
Copy link
Owner

Tox is broken since yesterdays release. Upstream issue is already raised. I will review this in the next couple of days.

@codecov
Copy link

codecov bot commented Dec 18, 2022

Codecov Report

Base: 98.70% // Head: 98.70% // No change to project coverage 👍

Coverage data is based on head (f6929d0) compared to base (7bfcfb1).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #901   +/-   ##
=======================================
  Coverage   98.70%   98.70%           
=======================================
  Files          68       68           
  Lines        7936     7936           
=======================================
  Hits         7833     7833           
  Misses        103      103           
Impacted Files Coverage Δ
drf_spectacular/openapi.py 97.68% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@freddez
Copy link
Contributor Author

freddez commented Jan 6, 2023

Hello,
could you integrate or reject this pull request ?

tfranzel added a commit that referenced this pull request Mar 4, 2023
@tfranzel tfranzel merged commit aaae20a into tfranzel:master Mar 4, 2023
@tfranzel
Copy link
Owner

tfranzel commented Mar 4, 2023

sry @freddez this took a while for me to get around to. thank you for the PR

Actually this got drive-by fixed in #936, due to a "bug" in DRF. However, I think it makes sense to include and also add SlugRelatedField for consistency and clarity even though the outcome would be the same.

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.

2 participants