Closed Bug 1153288 Opened 11 years ago Closed 10 years ago

Reimplement DRF serializers and renderer

Categories

(developer.mozilla.org Graveyard :: BrowserCompat, enhancement)

Product:
developer.mozilla.org Graveyard
Component:
BrowserCompat
Platform:
All
Other
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jwhitlock, Unassigned)

References

Details

(Whiteboard: [specification][type:feature][bc:infra])

What problems would this solve? =============================== Django REST Framework's ModelSerializer is used to implement the API. This is useful for quickly exposing Django models as an API, but it is difficult to customize the behavior, and many customization hooks were removed in the 3.0 release. Our customizations mean that the most reliable way to test the API is integration tests that use the DRF test client to fully test the API endpoints, rather than focused unit tests of our modifications. This is difficult for code reviewers and code contributors. One of the 3.0 goals was to make it easier for developers to move from ModelSerializer to Serializer to fully customize behavior [1]. We should take the suggestion and use Serializer vs. Model Serializer w/ hacks. [1] https://youtu.be/3cSsbe-tA0E Who would use this? =================== Code reviewers and code contributors What would users see? ===================== Code reviewers would review tests that narrowly test our different functionality, with much less code duplication. Integration tests would be marked as such, and not be used for testing functionality or code coverage. These tests will ensure that API users see no changes. Code contributors would have examples of narrow unit tests for their functionality. What would users do? What would happen as a result? =================================================== Code reviewers can review changes more quickly. Code contributors can be clearer about the changed functionality. Is there anything else we should know? ====================================== This came up several times in code review, most recently in the review for bug 1128519. Moving to integration tests would be a natural part of bug 1063565.
Blocks: 996570
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 1159292
Blocks: 1078699
Component: General → BrowserCompat
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/0e0c2e444df3e19a47338c2c31de675e956d133a bug 1153288 - Use new TravisCI infrastructure https://github.com/mdn/browsercompat/commit/e8a4ff22c337d94f25d174e510d13abc5e0e3c13 bug 1153288 - requirements bump w/o code changes * Babel 1.3->2.0 - Small fixes, unused features * Django 1.7.8->1.7.10 - Security updates * Jinja2 2.7.3->2.8 - Fixes and unused features * alabaster 0.7.5->0.7.6 - Small fixes * django-debug-toolbar 1.3.0->1.3.2 - Fixes, translations * django-filter 0.10.0->0.11.0 - Unused features * django-nose 1.4->1.4.1 - Small fixes * django-simple-history 1.6.1->1.6.3 - Django 1.8 compat * django-sortedm2m 0.10.0->1.0.2 - Ordering fix on adds * ipython 3.1.0->4.0.0 - Split for Jupyter umbrella project * oauthlib 0.7.2->1.0.3 - Crypto update, stricter, fixes * pep257 0.5.0->0.6.0 - Small fixes, unused features * pluggy 0.3.0->0.3.1 - Python 3.5 fix * py 1.4.28->1.4.30 - Small fixes * pyflakes 0.9.1->1.0.0 - Small (py3.5) fixes * pylibmc 1.4.3->1.5.0 - Fix critical memory leaks * pytz 2015.4->2015.6 -i Olson tz database bump * sphinx-rtd-theme 0.1.8->0.1.9 - Small fixes * sqlparse 0.1.15->0.1.16 - Small fixes * tox 2.0.2->2.1.1 - More environment fixes * virtualenv 13.0.3->13.1.2 - pip sync
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/0e0c2e444df3e19a47338c2c31de675e956d133a bug 1153288 - Use new TravisCI infrastructure https://github.com/mdn/browsercompat/commit/98d22d511a2426cac8bd2401d2dac8236f7c52ee bug 1153288 - Update requirements w/o code changes These requirements do not require code changes: * Babel 1.3->2.1.1 - Small fixes, unused features * Django 1.7.8->1.7.10 - Security updates * Jinja2 2.7.3->2.8 - Fixes and unused features * alabaster 0.7.5->0.7.6 - Small fixes * drf-cached-instances 0.3.2 - Django 1.8 compat * django-debug-toolbar 1.3.0->1.3.2 - Fixes, translations * django-filter 0.10.0->0.11.0 - Unused features * django-nose 1.4->1.4.1 - Small fixes * django-simple-history 1.6.1->1.6.3 - Django 1.8 compat * django-sortedm2m 0.10.0->1.0.2 - Ordering fix on adds * ipython 3.1.0->4.0.0 - Split for Jupyter umbrella project * oauthlib 0.7.2->1.0.3 - Crypto update, stricter, fixes * pep257 0.5.0->0.6.0 - Small fixes, unused features * pluggy 0.3.0->0.3.1 - Python 3.5 fix * py 1.4.28->1.4.30 - Small fixes * pyflakes 0.9.1->1.0.0 - Small (py3.5) fixes * pylibmc 1.4.3->1.5.0 - Fix critical memory leaks * pytz 2015.4->2015.6 -i Olson tz database bump * sphinx-rtd-theme 0.1.8->0.1.9 - Small fixes * sqlparse 0.1.15->0.1.16 - Small fixes * tox 2.0.2->2.1.1 - More environment fixes * virtualenv 13.0.3->13.1.2 - pip sync https://github.com/mdn/browsercompat/commit/ace329b3a27fdf500c1154bb42e13cbb98fc989d bug 1153288 - Update coverage to 4.0 coverage 4.0 / coveralls 1.0 allows moving the configuration to setup.cfg, and requires explicitly allowing the TravisCI environment variables. https://github.com/mdn/browsercompat/commit/41a99bd2fbc120a6634f6c5b221f1fa14ddbbac1 bug 1153288 - Cleanup packaging
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/274f8f484c83f575cca4df05d665f963ae683b32 bug 1153288 - Fix tests of mock calls Previous code was using functions like mock_task.assertNotCalled() and mock_task.assertCalledOnce() to check the calls of mocked tasks. However, these are not actual functions, and mock 1.0.1 silently changed them to new mock methods. Switch to the actual functions such as mock_task.assert_not_called() and mock_task.assert_called_once(). mock 1.3.0 will raise an error on mock_task.assertNotCalled(), thus exposing my laziness in not writing a failing test first. https://github.com/mdn/browsercompat/commit/99cce423b3e08a830f8310eb0a3498586bc58629 bug 1153288 - Update mock to 1.3.0
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/2bb3d3d15b636177f6fa2fa2b935dca19419a93e bug 1153288 - Switch to Django's DateTimeField django_extensions 1.5.7 switches ModificationDateTimeField and CreationDateTimeField to use auto_now and auto_now_add. This breaks some existing tests, since the columns are set at the database level, not at instance initialization, and are unavailable unless re-read from the database. Since code changes are needed anyway, this seems like a good time to switch to Django's standard DateTimeField, and set the defaults used by django_extensions directly. https://github.com/mdn/browsercompat/commit/ea64ddf590c2d5c7ac55348496102a22d44fe86e bug 1153288 - Update django_extensions to 1.5.7
Commit pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/64f84188fcd89109a34056e1b5282d63c46e0c90 bug 1153288 - Update django-allauth to 0.23.0 django-allauth 0.23.0 drops the context processors, which changes how the providers list is accessed.
Commit pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/d392de2905a29672a4f87c874ae0b05e07e45639 bug 1153288 - Update djangorestframework to 3.2.4 In a many-to-many field, blank=False (default) now means that a blank result is not allowed. Switch to blank=True to allow empty, and remove non-standard extra args to feature.sections.
Commit pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/030e158b66a7ed62326b7ae62bf0d9624a770cc4 bug 1153288 - Pin flake8 dependencies Pin versions of packages installed by flake8. Currently, the TravisCI flake8 build is broken because pep257 updated to 0.7.0, and is finding more documentation issues with the code.
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/3b5d8b47c023fb4f1f1b522b33daf8360c8df975 bug 1153288 - Handle unicode responses Running tools/integration_requests.py with --display --include-mod will include a test that outputs unicode, and will fail when redirecting to a file. https://github.com/mdn/browsercompat/commit/826431f412ddd2a70fce0a0a648d8a6a6de4de4c bug 1153288 - Fix reverting specifications Reverting a specification was not reverting the maturity https://github.com/mdn/browsercompat/commit/fb2f9827e63ead69b0ac9f4f81165a5d44c0423f bug 1153288 - Fix canonical names in view_features In the list endpoint for view_features, include canonical names rather than displaying "none" https://github.com/mdn/browsercompat/commit/c3de021273016a5ba541bc6b50a620a8088cc159 Merge pull request #78 from mdn/api_fixes_1153288 bug 1153288 - More API fixes r=groovecoder
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/1abdf4dd5db57b5b297eb06707752cf442597ee8 bug 1153288 - Bump easy requirements These requirements updates require no code changes: * Django 1.8.5->1.8.6: South migration warning, unrelated fixes * Markdown 2.6.2->2.6.4: no changelog, assuming small fixes * Werkzeug 0.10.4->0.11.2: Fixes, PIN-based auth for debugger * amqp 1.4.6->1.4.7: unrelated fixes * billiard 3.3.0.20->3.3.0.21: unrelated fixes * celery 3.1.18->3.1.19: unrelated fixes * check-manifest 0.25->0.28: unrelated fixes * coverage 4.0->4.0.2: unrelated fixes * coveralls 1.0->1.1: unrelated fixes * django-allauth 0.23.0.x->0.24.1: Jinja2 patch released * django-debug-toolbar 1.3.2->1.4: Django 1.9 support * django-extensions 1.5.7->1.5.9: Django 1.8.6 compat, other fixes * django-nose 1.4.1->1.4.2: unrelated fixes * flake8 2.4.1->2.5.0: updates for newer pyflakes versions * kombu 3.0.26->3.0.29: Django 1.9 fixes, unrelated fixes * pyroma 1.8.2->1.8.3: unrelated fixes * pytz 2015.6->2015.7: new Olson database * requests 2.7.0->2.8.1: unrelated fixes * six 1.9.0->1.10.0: unrelated fixes * sqlparse 0.1.16->0.1.18: unrelated fixes * tox 2.1.1->2.2.1: unrelated fixes https://github.com/mdn/browsercompat/commit/08a5098b5ba55f71ecfa433a47d4a86e0fd9ac1d bug 1153288 - Update ignore rules for new pep257 Updating pep257 from 0.6.0 to 0.7.0 includes these changes: * Missing a docstring in __init__.py is now D104 instead of D100. * Missing a docstring in a magic method (__str__, etc.) is now a D105 instead of a D102. * PEP257 itself changed from requiring a blank line before the class docstring (D203) to no blank lines (D211) Updating the rules to ignore new warnings, drop ignores that aren't needed, and add issue counts. https://github.com/mdn/browsercompat/commit/e5c3428b6d8e7f0ad8823158523f30b154f5ff43 bug 1153288 - Update to Django REST Framework 3.3 Includes switching to the new pagination declaration style.
Over the past two months, I've been experimenting with different implementations while working on other issues, and I have different ideas than when I wrote this bug 8 months ago Implementing on top of Serializer rather than ModelSerializer means a lot of duplication with the model declarations, and bringing in a lot of additional code and tests to replace code provided by ModelSerializer. So, I'm dropping this idea. Instead, I'm collecting a lot of the modifications into a new class attribute for serializers, fields_extra. This dictionary customizes field behavior for parsing, rendering, and archiving. It consolidates and replaces old DRF additions like update_only_fields and write_once_fields. For the v1 API, the parser and renderer will continue using the DRF standard serializer data. For the v2 API, I'll use fields_extra to define additional behavior. This will make the code less generic, but also easier to work with. It also couples this task with the v2 task (bug 1159406), so they will be closed in rapid sequence, maybe with the same PR.
Depends on: 1159406
Commits pushed to master at https://github.com/mdn/browsercompat https://github.com/mdn/browsercompat/commit/9dc8afbb7b24e2d17b742c97e807a94639441e86 bug 1153288 - Move to Meta.fields_extra writable Move Meta.update_only_fields and Meta.write_once_fields, which specify write restrictions beyond DRF built-in functionality, to new Meta.fields_extra attribute https://github.com/mdn/browsercompat/commit/dd5c6067c5077d90ebc7848096d404f16156e0a0 bug 1153288 - Move archive defs to fields_extra Instead of overriding ArchivedObject.Meta, set new attributes in Meta.fields_extra: * Removing fields with omit_some becomes archive=omit * history_current becomes archive=history_id * id becomes link=self * archive_link_fields becomes link=to_one * archive_cached_link_fields becomes link=from_many Historic field order is preserved with setting Meta.fields, but will go away in a future commit. https://github.com/mdn/browsercompat/commit/c0c802cabf822b624c5a20aa23a45f1afcfd8329 bug 1153288 - Normalize attribute orders Arrange the fields declaration to standardize the order of fields: * The id field * Attributes * To-one links * To-many links * history_current * history Many documentation responses change, because they reflect this order. https://github.com/mdn/browsercompat/commit/02645c8a15554dd5e5f271721d739e17038da0e0 bug 1153288 - Move omit_some to view_serializers This helper function is no longer used in serializers.py https://github.com/mdn/browsercompat/commit/15fb48059c7bb2623ee171d6b20ef03f07a41afe bug 1153288 - Rewrite parser and renderer Instead of using a fork of the drf-json-api project, implement project-specific parser and renderer for the v1 API's JSON API Release Candidate 1 (RC1) implementation: * Rewrite JsonApiParser as JsonApiRC1Parser, with tests * Rewrite JsonApiRenderer as JsonApiRC1Renderer, with tests * Add fields_extra.resource, for the resource type of links * Add fields_extra.name, to alter attribute names in representation * Add serializer class method get_fields_extra for returning the extra field configuration data * Add field_extra data to renderer context for common case of single-resource responses. * Standardize HistoricalObjectSerializer field names * Add archive_extra to HistoricalObjectSerializer, to convert from standardized names to model-specific names and resource types * Use ReturnList in ViewFeatureExtraSerializer, so serializer is available in renderer * Use lists for field errors in linked resources, matching Django standard https://github.com/mdn/browsercompat/commit/559335818e2ce42e277ec8ec9b89e88db0b1c12e bug 1153288 - Fix view_feature/42?child_pages=0 Previously, any value for the query argument child_pages resulted in child pages being included and long responses paginated. This change allows disabling child pages with 'falsy' values. https://github.com/mdn/browsercompat/commit/11ce72f429845a2fd3fa527394ed74e35aab05a5 bug 1153288 - Refactor feature relation caching Standardize the feature interface for Django instances and cached instances, so that they are easier to use interchangably. Previously, the cached feature instances would have additional properties for relationships such as descendant features, implemented as CachedQuerysets. ViewFeatureSerializer expected these relationships and only worked with cached features. This changes the properties, and implements them on the Django model using the @cached_property decorator. This simplifies the cached feature serializer, and means that either can be used in ViewFeatureSerializer. The one difference is that cached features do not store very long descendant lists. If this situation is detected, the Django feature instance is loaded to be able to get the desendant PKs for pagination. https://github.com/mdn/browsercompat/commit/2d2e3fd4ff4c5232b47bc0ef26fddf41cbfb01a6 bug 1153288 - Fix display of unknown support https://github.com/mdn/browsercompat/commit/841eec786f6ed5c361e87016e484456ac81c28fb bug 1153288 - Handle missing context parameters Use context.get(key, default) instead of context[key], to handle cases where the context isn't fully populated (like the next commit). https://github.com/mdn/browsercompat/commit/120f0a650a7b7fcf1c656f98d83a2c4dd5e82656 bug 1153288 - Refactor serializer tests Serializer tests used the Django client, and so they were dependant on parser, view, and render behaviour as well. The tests are changed to test the serializers directly, using native DRF representations instead of JSON API RC1 representations. This means that serializer tests should stil be applicable in the v2 API using JSON API v1.0. https://github.com/mdn/browsercompat/commit/92a8df090cabd4aee010e1fa7728c0d238542a87 bug 1153288 - Render OPTIONS response Add unit and integration tests so that OPTIONS responses won't be lost again. https://github.com/mdn/browsercompat/commit/5d6aa3283d2746786e647ae8a07374e626bd6596 bug 1153288 - Remove RequestView request is already added to the context, no special class needed. https://github.com/mdn/browsercompat/commit/ccf3fc5634b74e2c327c61438a4edd1dfa2e978e bug 1153288 - Clean up comments, names https://github.com/mdn/browsercompat/commit/add9ad8f2212610f78d4fc96c90eddd8bd6029f4 bug 1153288 - Expect response, etc. in renderer Instead of using renderer_context.get('response'), use renderer_context['response']. This is defined for all unit and integration tests, as well as practical tests. https://github.com/mdn/browsercompat/commit/15961900cc85e85a07259c78af4620a992b1a8b1 bug 1153288 - Add JsonApiRC1Renderer.is_paginated Name the is_paginated test as a method, and remove the redundant pagination check in convert_paginated. https://github.com/mdn/browsercompat/commit/98b10800039b99a30ee745a7e1f08790d9512a71 bug 1153288 - Assert list response is ReturnList In renderer, assert that a list response is a ReturnList, so that we know .serializer is a ListSerializer, and .serializer.child is the original serializer. https://github.com/mdn/browsercompat/commit/3ab32e75529408631e09dcd294eb0d5784b84aef bug 1153288 - Refactor link switch in convert_link The current link options are a list ("from_many") or a single value ("self", "from_one", "to_one"). Handle all the single value links in the same else clause, with an exception that will break if "to_many" reappears as an option. https://github.com/mdn/browsercompat/commit/b49cccc31760a0dea706941de7d3d3ca77fd0342 bug 1153288 - Drop omit_some from view_serializers Explicitly override the fields and read_only_fields in view-specific serializers, instead of using the omit_some hack. https://github.com/mdn/browsercompat/commit/e6a406e38ff2541841bfe3e99237736097a68117 bug 1153288 - Expand docs on include_child_pages Make the docstring clearer on what the default value is, and different settings of the child_pages query string. Also, assume self.request is always set, until live code proves otherwise. https://github.com/mdn/browsercompat/commit/002ac95ee8eb07e446667dea6557fde251846de6 bug 1153288 - Add assertion messages Add assertion messages to the assertions in recently changes modules. There are another 113 in existing modules, but those will be handled in bug 1238587. https://github.com/mdn/browsercompat/commit/070c148b3b1b74742d3b06693390855e4c349801 Merge pull request #89 from mdn/refactor_1153288 fix bug 1153288 - Rewrite serializer, parser, renderer
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: [Compat Data] Reimplement DRF serializers and renderer → Reimplement DRF serializers and renderer
Whiteboard: [specification][type:feature] → [specification][type:feature][bc:infra]
Product: developer.mozilla.org → developer.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.