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

Propagate 'default' from model_field to serializer field #8130

Closed
wants to merge 1 commit into from

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Aug 13, 2021

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.

@benesch benesch changed the title Propogate 'default' from model_field to serializer field Propagate 'default' from model_field to serializer field Aug 13, 2021
@john-parton
Copy link
Contributor

I'll close my issue.

@john-parton
Copy link
Contributor

john-parton commented Aug 13, 2021

Just a note for discussion, part of the motivating factor for this change was to include the default value in the OPTIONS response. More info here: #8003

If this PR gets merged, I can rework 8003 as its own PR.

@tomchristie
Copy link
Member

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 'default' for schemas would make sense...

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.

@tomchristie tomchristie added this to the 3.14 milestone Dec 13, 2021
@stale
Copy link

stale bot commented Apr 17, 2022

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.

@stale stale bot added the stale label Apr 17, 2022
@peterthomassen
Copy link
Contributor

I think this may also fix #7489 which is still current.

Copy link
Member

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

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.

@stale stale bot removed the stale label Nov 22, 2022
@stale
Copy link

stale bot commented Jan 22, 2023

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.

@stale stale bot added the stale label Jan 22, 2023
@auvipy auvipy removed the stale label Jan 22, 2023
@stale
Copy link

stale bot commented Mar 25, 2023

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.

@stale
Copy link

stale bot commented Jun 18, 2023

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.

@stale stale bot added the stale label Jun 18, 2023
Copy link
Member

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

I would like to see this rebased with a fresh approach to be considered to be accepted

@benesch
Copy link
Contributor Author

benesch commented Jun 18, 2023

Anyone should feel free to take over this PR. I’m not going to have time myself.

@stale stale bot removed the stale label Jun 18, 2023
@peterthomassen
Copy link
Contributor

@ashupednekar Do you have time for this?

(Asking due to #7489 (comment))

@auvipy
Copy link
Member

auvipy commented Jun 19, 2023

@samims you can also check this out

@rnshaikh
Copy link
Contributor

rnshaikh commented Jul 1, 2023

@auvipy I have made the changes requested in this PR.
default fields propagated from model to serializers.

  1. Generate 'default' for serializer as generated for open API schema.
  2. Updated OPTIONS response to accommodate the default field there. Created Testcase to check default is generated for OPTIONS response for different fields.

I have created PR for this #9030 I can't update this PR as I don't have permission.

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.

ModelSerializer generate ChoicesField will discard default parameter
7 participants