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] Fixed REST API can creates device configs inadvertently #699 #759

Merged
merged 2 commits into from
May 15, 2023

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented May 4, 2023

Fixes #699

@pandafy pandafy force-pushed the issues/699-device-config branch 4 times, most recently from be422c0 to 4cb70f9 Compare May 8, 2023 16:45
@coveralls
Copy link

coveralls commented May 8, 2023

Coverage Status

Coverage: 98.907% (+0.9%) from 98.035% when pulling e09895d on issues/699-device-config into 22590a4 on master.

@@ -149,6 +151,16 @@ def _create_config(self, device, config_data):
raise serializers.ValidationError({'config': error.messages})

def _update_config(self, device, config_data):
if (
config_data.get('backend') == app_settings.BACKENDS[0][0]
Copy link
Member

Choose a reason for hiding this comment

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

this should compare to the default value of the Config.backend field, which if I remember correctly can be modified via OPENWISP_CONTROLLER_DEFAULT_BACKEND.

config_data.get('backend') == app_settings.BACKENDS[0][0]
and not config_data.get('templates')
and not config_data.get('context')
and not config_data.get('config')
Copy link
Member

Choose a reason for hiding this comment

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

Can you get the default values from the serializer fields and compare each property to the corresponding serializer field default? Such a solution would be more robust to future changes, I doubt anyone changing the default values (if ever needed, but we never know) of the serializer or model will remember to change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The ModelSerializer defers enforcing defaults to the model
encode/django-rest-framework#2683

Copy link
Member Author

@pandafy pandafy May 15, 2023

Choose a reason for hiding this comment

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

The Browsable API is not using the default values defined in serializer fields when rendering form for Device object that does not have a Config:

Screenshot from 2023-05-15 21-55-30

The context and config field values are set to null instead of {}. When form with such values is saved, then serializer parses the data to

{
    'backend': 'netjsonconfig.OpenWrt',
     'config': None,
     'context': None,
     'templates': []
}

Which is different from the default values defined for the field:

{
    'backend': 'netjsonconfig.OpenWrt',
     'config': {},
     'context': {},
     'templates': []
}

I have enquired about this on DRF's mailing list.

Copy link
Member

Choose a reason for hiding this comment

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

no problem let's use the model then

@nemesifier nemesifier merged commit 84abe37 into master May 15, 2023
@nemesifier nemesifier deleted the issues/699-device-config branch May 15, 2023 19:55
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.

[bug] REST API can creates device configs inadvertently
3 participants