Rework FolderSerializer to use native DRF validation #130

brianhelba · 2021-01-30T23:18:17Z

FullCleanModelSerializer does not work properly with internally-set fields like tree, since the value is not available at the time of initial validation. Currently, it only works because tree has editable=False, so the Model-layer validator skips the field.

See this comment for more information on the problem.

This refactor also has the result of effectively removing the default of parent=None when creating a folder. This is probably a net benefit, as it forces API clients to be explicit when creating a new root folder.

Also:

Add tests for POSTing invalid Folder values
Test folder updates with a PATCH request

dkc/core/rest/folder.py

zachmullen

Could we expand the scope of this PR to deleting FullCleanModelSerializer altogether? Based on this PR and our discussion, it seems like we shouldn't be using it anywhere. I think the only other changes needed would be in rest/file.py.

FullCleanModelSerializer does not work properly with internally-set fields like "tree", since the value is not available at the time of initial validation. Currently, it only works because "tree" has "editable=False", so the Model-layer validator skips the field. This refactor also has the result of effectively removing the default of "parent=None" when creating a folder. This is probably a net benefit, as it forces API clients to be explicit when creating a new root folder.

brianhelba · 2021-01-30T23:20:16Z

dkc/core/rest/folder.py

+                fields=['name'],
+                message='Root folders must have a unique name.',
+            ),
+            # folder_max_depth and unique_root_folder_per_tree are internal sanity constraints,


Actually, folder_max_depth is something we might want to check at the REST layer (rather than allowing the database to do it). I'm not sure how to do this, so I'll probably file a bug for a future fix if nobody else has suggestions.

brianhelba · 2021-01-30T23:22:54Z

dkc/core/rest/folder.py


    def get_access(self, folder: Folder) -> Dict[str, bool]:
        return folder.tree.get_access(self.context.get('user'))


 class FolderUpdateSerializer(FolderSerializer):
    class Meta(FolderSerializer.Meta):
-        fields = ['id', 'name', 'description']
+        read_only_fields = ['parent']


Specifying fields had the result of also limiting the output of what this serializer produced in the response to a PUT/PATCH request. What we really want to do is ensure that 'parent' cannot be changed in an update request, which this now does.

dkc/core/rest/folder.py

brianhelba · 2021-01-30T23:26:52Z

dkc/core/tests/test_folder_rest.py

@@ -51,7 +95,7 @@ def test_folder_rest_retrieve(admin_api_client, folder):

 @pytest.mark.django_db
 def test_folder_rest_update(admin_api_client, folder):
-    resp = admin_api_client.put(
+    resp = admin_api_client.patch(


We might want to outright disallow PUT requests at some point, but since I think we typically expect clients to use PATCH requests, it's better for this test to use that.

brianhelba · 2021-01-30T23:31:15Z

@zachmullen I intend to submit a second PR to remove FullCleanModelSerializer entirely, but it's going to be more manageable to refactor FileSerializer separately.

dkc/core/rest/folder.py

brianhelba · 2021-01-30T23:37:16Z

Tests are failing due to kitware-resonant/django-composed-configuration#114 . They'll pass as soon as that's released.

zachmullen

After this change, the error response has the form:

{
  "non_field_errors": [
    "..."
  ]
}

I think we should associate these UniqueTogetherValidators with the name field, as that will be more consumable downstream. (Is that difficult to do?)

brianhelba · 2021-01-31T01:26:53Z

After this change, the error response has the form:
{
  "non_field_errors": [
    "..."
  ]
}
I think we should associate these UniqueTogetherValidators with the name field, as that will be more consumable downstream. (Is that difficult to do?)

For the case of sibling files, this is easy, as we just need to explicitly raise ValidationError({'name': ...}), and DRF will skip nesting it under NON_FIELD_ERRORS_KEY.

In the cases where UniqueTogetherValidator is used, setting message to be a dict instead of a string causes an error, as DRF calls .format on the message value. I've created a FormattableDict to wrap the values as a workaround.

Fixed.

zachmullen · 2021-01-31T02:02:04Z

Let's open that ticket for validating max depth.

brianhelba · 2021-01-31T03:05:32Z

Pushed 2 more minor commits.

brianhelba · 2021-01-31T03:13:54Z

As a note, following the advice to print(repr(FolderSerializer())) from https://www.django-rest-framework.org/api-guide/validators/#validation-in-rest-framework was quite helpful. Given how much of ModelSerializer subclasses are auto-generated, future development should always include this as an essential sanity check.

Add tests for POSTing invalid Folder values

82ffbbe

zachmullen reviewed Jan 30, 2021

View reviewed changes

dkc/core/rest/folder.py Outdated Show resolved Hide resolved

zachmullen reviewed Jan 30, 2021

View reviewed changes

dkc/core/rest/folder.py Outdated Show resolved Hide resolved

zachmullen suggested changes Jan 30, 2021

View reviewed changes

brianhelba added 2 commits January 30, 2021 18:28

Test folder updates with a PATCH request

1c29237

brianhelba force-pushed the folder-serializer branch from 0f2167b to 1c29237 Compare January 30, 2021 23:29

brianhelba commented Jan 30, 2021

View reviewed changes

brianhelba requested a review from jbeezley January 30, 2021 23:32

zachmullen reviewed Jan 30, 2021

View reviewed changes

dkc/core/rest/folder.py Show resolved Hide resolved

zachmullen previously requested changes Jan 30, 2021

View reviewed changes

brianhelba added 3 commits January 30, 2021 20:02

Use simpler error messages on validator failure

9bfe4be

Check for duplicate sibling files during FolderSerializer validation

b095121

Don't use NON_FIELD_ERRORS_KEY for FolderSerializer 'name' failures

5c2fd26

brianhelba changed the title ~~@brianhelba Rework FolderSerializer to use native DRF validation~~ Rework FolderSerializer to use native DRF validation Jan 31, 2021

brianhelba requested a review from zachmullen January 31, 2021 01:28

zachmullen approved these changes Jan 31, 2021

View reviewed changes

brianhelba added 2 commits January 30, 2021 22:03

Use consistent wording for errors from non-unique folder names

c879bbe

Fix a harmless bug in folder name validation

458eeae

brianhelba merged commit 3fb2b9a into master Jan 31, 2021

brianhelba deleted the folder-serializer branch January 31, 2021 04:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework FolderSerializer to use native DRF validation #130

Rework FolderSerializer to use native DRF validation #130

brianhelba commented Jan 30, 2021 •

edited

Loading

zachmullen left a comment

brianhelba Jan 30, 2021

brianhelba Jan 30, 2021

brianhelba Jan 30, 2021

brianhelba commented Jan 30, 2021

brianhelba commented Jan 30, 2021

zachmullen left a comment •

edited

Loading

brianhelba commented Jan 31, 2021

zachmullen commented Jan 31, 2021

brianhelba commented Jan 31, 2021

brianhelba commented Jan 31, 2021

Rework FolderSerializer to use native DRF validation #130

Rework FolderSerializer to use native DRF validation #130

Conversation

brianhelba commented Jan 30, 2021 • edited Loading

zachmullen left a comment

Choose a reason for hiding this comment

brianhelba Jan 30, 2021

Choose a reason for hiding this comment

brianhelba Jan 30, 2021

Choose a reason for hiding this comment

brianhelba Jan 30, 2021

Choose a reason for hiding this comment

brianhelba commented Jan 30, 2021

brianhelba commented Jan 30, 2021

zachmullen left a comment • edited Loading

Choose a reason for hiding this comment

brianhelba commented Jan 31, 2021

zachmullen commented Jan 31, 2021

brianhelba commented Jan 31, 2021

brianhelba commented Jan 31, 2021

brianhelba commented Jan 30, 2021 •

edited

Loading

zachmullen left a comment •

edited

Loading