Propagate 'default' from model_field to serializer field#8130
Propagate 'default' from model_field to serializer field#8130benesch wants to merge 1 commit intoencode:masterfrom
Conversation
Fix encode#7469. Co-authored-by: Nikhil Benesch <[email protected]>
benesch
force-pushed
the
model-serializer-defaults
branch
from
9333773 to
bbea2dc
Compare
benesch
changed the title
|
I'll close my issue. |
|
Just a note for discussion, part of the motivating factor for this change was to include the default value in the If this PR gets merged, I can rework 8003 as its own PR. |
|
Hi folks. 👋 So, if we did want to accept this, then I'd suggest that this pull request should also update the OPTIONS responses in order to include the default - https://github.com/encode/django-rest-framework/blob/master/rest_framework/metadata.py#L115 Staying in line with how we generate django-rest-framework/rest_framework/schemas/openapi.py Lines 535 to 536 in 6ea95b6 I'm a bit reluctant, simply because at this point in it's lifecycle almost any change in REST framework ends up having unintended side-effects for existing users, and I could easily see this being the case for this change. We've used "default on the model => not required on the serializer" since ~forever. On the other hand, exposing default values on the schema and |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I think this may also fix #7489 which is still current. |
There was a problem hiding this comment.
the PR need to address following suggestion:
So, if we did want to accept this, then I'd suggest that this pull request should also update the OPTIONS responses in order to include the default - https://github.com/encode/django-rest-framework/blob/master/rest_framework/metadata.py#L115
Staying in line with how we generate 'default' for schemas would make sense...
django-rest-framework/rest_framework/schemas/openapi.py
Lines 535 to 536 in 6ea95b6
if field.default is not None and field.default != empty and not callable(field.default):
schema['default'] = field.default
I'm a bit reluctant, simply because at this point in it's lifecycle almost any change in REST framework ends up having unintended side-effects for existing users, and I could easily see this being the case for this change. We've used "default on the model => not required on the serializer" since ~forever.
On the other hand, exposing default values on the schema and OPTIONS responses does seem like an improvement.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
auvipy
left a comment
auvipy
left a comment
There was a problem hiding this comment.
I would like to see this rebased with a fresh approach to be considered to be accepted
|
Anyone should feel free to take over this PR. I’m not going to have time myself. |
|
@ashupednekar Do you have time for this? (Asking due to #7489 (comment)) |
|
@samims you can also check this out |
|
@auvipy I have made the changes requested in this PR.
I have created PR for this #9030 I can't update this PR as I don't have permission. |
7 participants
Description
This is a resubmission of #8002 that will hopefully pass tests. The tl;dr is that field defaults are now propagated from models to model serializers.
Fix #7469.