This repository has been archived by the owner. It is now read-only.

Initializing migration from django-piston to django-restframework (bug 974952) #1749

Merged
merged 1 commit into from Mar 3, 2014

Conversation

Projects
None yet
5 participants
@vinyll
Contributor

vinyll commented Feb 11, 2014

Moving api.views to api.views_drf while migrating piston to DRF.

See bug 974952 for further details.
This is part 1: moving AddonDetailView and LanguageView.
The ticket is being splitted.

Latests tests results are here: https://ci-addons.allizom.org/job/marketplace-build-branch/31/ (passing)

@diox

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 12, 2014

Member

Using renderers instead of having various render_* methods would be more "DRFesque" : http://www.django-rest-framework.org/api-guide/renderers

Member

diox commented Feb 12, 2014

Using renderers instead of having various render_* methods would be more "DRFesque" : http://www.django-rest-framework.org/api-guide/renderers

+ return super(JSONRenderer, self).render(data, *args, **kwargs)
+
+
+class XMLTemplateRenderer(XMLRenderer):

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

can we have a docstring here to have a better understanding of the use for this class?

@magopian

magopian Feb 20, 2014

Contributor

can we have a docstring here to have a better understanding of the use for this class?

@magopian

View changes

apps/api/renderers.py
+class XMLTemplateRenderer(XMLRenderer):
+
+ def render(self, data, accepted_media_type=None, renderer_context=None):
+ request = renderer_context['request']

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

this (and the following line) will fail if render_context is None

@magopian

magopian Feb 20, 2014

Contributor

this (and the following line) will fail if render_context is None

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 20, 2014

Contributor

That's a feature , it should actually fail if renderer_context is None

@vinyll

vinyll Feb 20, 2014

Contributor

That's a feature , it should actually fail if renderer_context is None

@magopian

View changes

apps/api/renderers.py
+ response = renderer_context['response']
+ if not hasattr(self, 'template_name') and not response.template_name:
+ raise Exception('AddonXMLRenderer response requires '
+ 'a "template_name" kwarg.')

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

This exception message sounds weird. Maybe something like "A template_name attribute is needed"?

@magopian

magopian Feb 20, 2014

Contributor

This exception message sounds weird. Maybe something like "A template_name attribute is needed"?

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 20, 2014

Contributor

Indeed. It's even nomore AddonXMLRenderer now but XMLTemplateRenderer.
I changed it, let me know what you think.

@vinyll

vinyll Feb 20, 2014

Contributor

Indeed. It's even nomore AddonXMLRenderer now but XMLTemplateRenderer.
I changed it, let me know what you think.

@magopian

View changes

apps/api/renderers.py
+ raise Exception('AddonXMLRenderer response requires '
+ 'a "template_name" kwarg.')
+ if not jingo._helpers_loaded:
+ jingo.load_helpers()

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

Is this needed? Does this piece of code run before the jingo helpers are loaded? If so, maybe add a comment?

@magopian

magopian Feb 20, 2014

Contributor

Is this needed? Does this piece of code run before the jingo helpers are loaded? If so, maybe add a comment?

@magopian

View changes

apps/api/renderers.py
+ data.update(processor(request))
+ xml_env = jingo.env.overlay()
+ old_finalize = xml_env.finalize
+ xml_env.finalize = (

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

I'd love a comment here too

@magopian

magopian Feb 20, 2014

Contributor

I'd love a comment here too

@magopian

View changes

apps/api/views_drf.py
+from tower import ugettext_lazy
+
+from addons.models import Addon
+import amo

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

I believe you should put the import ... before the from ...

@magopian

magopian Feb 20, 2014

Contributor

I believe you should put the import ... before the from ...

@magopian

View changes

apps/api/views_drf.py
+import api
+
+from .utils import addon_to_dict
+from .renderers import JSONRenderer, XMLTemplateRenderer

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

alphabetic order

@magopian

magopian Feb 20, 2014

Contributor

alphabetic order

+
+ERROR = 'error'
+OUT_OF_DATE = ugettext_lazy(
+ u'The API version, {0:.1f}, you are using is not valid. '

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

The commas aren't needed, are they? Also, extraneous space at the end.

@magopian

magopian Feb 20, 2014

Contributor

The commas aren't needed, are they? Also, extraneous space at the end.

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 20, 2014

Contributor

Changing this here would require to update the tests, which we'll avoid while migrating the lib.

@vinyll

vinyll Feb 20, 2014

Contributor

Changing this here would require to update the tests, which we'll avoid while migrating the lib.

@magopian

View changes

apps/api/views_drf.py
+ @allow_cross_site_request
+ def get(self, request, addon_id, api_version, format=None):
+ self.format = format or request.QUERY_PARAMS.get(
+ getattr(settings, 'URL_FORMAT_OVERRIDE'), 'xml')

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

do we really need a setting for that? Do you think we'll need a different format name later?

@magopian

magopian Feb 20, 2014

Contributor

do we really need a setting for that? Do you think we'll need a different format name later?

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 20, 2014

Contributor

I wouldn't go for it if django_restframework would make it that way. We do use it for DRF so that it read ?format=json which is how AMO works right now.

@vinyll

vinyll Feb 20, 2014

Contributor

I wouldn't go for it if django_restframework would make it that way. We do use it for DRF so that it read ?format=json which is how AMO works right now.

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 20, 2014

Member

Add a comment

@diox

diox Feb 20, 2014

Member

Add a comment

@magopian

View changes

apps/api/views_drf.py
+from .renderers import JSONRenderer, XMLTemplateRenderer
+
+
+ERROR = 'error'

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

not sure about settings a module level constant when it's only used once.

@magopian

magopian Feb 20, 2014

Contributor

not sure about settings a module level constant when it's only used once.

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 20, 2014

Contributor

You'e right about that. I got it from views.py and will be present a few times in this module.
I'm afraid it would not be obvious to replace it there where migrating the rest of the views to views_drf.

@vinyll

vinyll Feb 20, 2014

Contributor

You'e right about that. I got it from views.py and will be present a few times in this module.
I'm afraid it would not be obvious to replace it there where migrating the rest of the views to views_drf.

@magopian

View changes

apps/api/views_drf.py
+ 'amo': amo,
+ 'version': version
+ }
+ return Response(context, template_name='api/addon_detail.xml')

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 20, 2014

Contributor

rendering to json or not still uses the same .xml template?

@magopian

magopian Feb 20, 2014

Contributor

rendering to json or not still uses the same .xml template?

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 20, 2014

Contributor

Actually JSON does not use a template at all as of now. I think it could be refactored when needed and get a nice way by that time.

@vinyll

vinyll Feb 20, 2014

Contributor

Actually JSON does not use a template at all as of now. I think it could be refactored when needed and get a nice way by that time.

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 20, 2014

Member

Add a comment

@diox

diox Feb 20, 2014

Member

Add a comment

@diox

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 20, 2014

Member

I'd remove the 2 old views you are replacing in the same commit. Do the tests pass ? Do we have enough tests on these APIs ? We really don't want to change anything for consumers.

Member

diox commented Feb 20, 2014

I'd remove the 2 old views you are replacing in the same commit. Do the tests pass ? Do we have enough tests on these APIs ? We really don't want to change anything for consumers.

@@ -0,0 +1,78 @@
+"""
+This view is a port of the views.py file (using Piston) to DRF.
+It is a work in progress that is supposed to replace the views.py completely.

This comment has been minimized.

Show comment Hide comment
@davidbgk

davidbgk Feb 21, 2014

Member

My take on that kind of migration is to move the actual views.py to views_piston.py then add new DRF-based views to views.py. Then move constants from views_piston to views and import from here if necessary (or maybe better to move those to constants.py too?) From my experience the final removal is way easier and thus not forgotten but YMMV.

@davidbgk

davidbgk Feb 21, 2014

Member

My take on that kind of migration is to move the actual views.py to views_piston.py then add new DRF-based views to views.py. Then move constants from views_piston to views and import from here if necessary (or maybe better to move those to constants.py too?) From my experience the final removal is way easier and thus not forgotten but YMMV.

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 21, 2014

Contributor

I liked the idea and therefore started to go for that. Now I think it's better to do it afterwards for these reasons:

  1. there's quite a few files that are impacted. That would make this PR heavier, less readable and lose its first purpose: initlalising a first step to move to DRF.
  2. Those dependencies with external files could be removed while rewriting the code along the refactoring.
@vinyll

vinyll Feb 21, 2014

Contributor

I liked the idea and therefore started to go for that. Now I think it's better to do it afterwards for these reasons:

  1. there's quite a few files that are impacted. That would make this PR heavier, less readable and lose its first purpose: initlalising a first step to move to DRF.
  2. Those dependencies with external files could be removed while rewriting the code along the refactoring.
@davidbgk

View changes

apps/api/views_drf.py
+
+class AddonDetailView(APIView):
+
+ renderer_classes = (XMLTemplateRenderer, JSONRenderer,)

This comment has been minimized.

Show comment Hide comment
@davidbgk

davidbgk Feb 21, 2014

Member

Extra comma.

@davidbgk

davidbgk Feb 21, 2014

Member

Extra comma.

@andymckay

View changes

apps/api/urls.py
name='api.addon_detail'),
- url(r'^get_language_packs$', class_view(views.LanguageView),
+ url(r'^get_language_packs$', views_drf.LanguageView.as_view(),

This comment has been minimized.

Show comment Hide comment
@andymckay

andymckay Feb 21, 2014

Contributor

Should this be behind a waffle flag? Some of these APIs are really high traffic and should be tested well. I'm not sure how well they are tested by QA, because they haven't changed in so long.

@andymckay

andymckay Feb 21, 2014

Contributor

Should this be behind a waffle flag? Some of these APIs are really high traffic and should be tested well. I'm not sure how well they are tested by QA, because they haven't changed in so long.

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 24, 2014

Member

This sounds like a good idea. I commented earlier that the older view should be removed, but you're right that we might want to be more careful here.

If we want a waffle flag however, it needs to be done in a wrapper function, we can't do that in urls.py. Also, do we have statsd logging in the current API, so get an idea of its performance ?

@diox

diox Feb 24, 2014

Member

This sounds like a good idea. I commented earlier that the older view should be removed, but you're right that we might want to be more careful here.

If we want a waffle flag however, it needs to be done in a wrapper function, we can't do that in urls.py. Also, do we have statsd logging in the current API, so get an idea of its performance ?

@diox

View changes

apps/api/tests/test_urls.py
+ def test_module(self):
+ view = switch_to_drf('LanguageView')
+ eq_(view.__module__, 'api.views')
+ waffle.models.Switch.objects.create(name='drf', active=True)

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 24, 2014

Member

self.create_switch('drf')

@diox

diox Feb 24, 2014

Member

self.create_switch('drf')

@diox

View changes

apps/api/tests/test_views.py
+ """
+ def setUp(self):
+ super(DRFMixin, self).setUp()
+ waffle.models.Switch.objects.create(name='drf', active=True)

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 24, 2014

Member

self.create_switch('drf')

@diox

diox Feb 24, 2014

Member

self.create_switch('drf')

@diox

View changes

apps/api/urls.py
+ """
+ Waffle switch to move from Piston to DRF.
+ """
+ if waffle.switch_is_active('drf'):

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 24, 2014

Member

I don't think that'll work. The function will be called directly when evaluating urls, and the result stored in urlconf cache, so you'll never be able to switch the waffle back without restarting the server.

If we truly want to be able to switch it we need a different kind of wrapper function, that is only evaluated when the view is called.

@diox

diox Feb 24, 2014

Member

I don't think that'll work. The function will be called directly when evaluating urls, and the result stored in urlconf cache, so you'll never be able to switch the waffle back without restarting the server.

If we truly want to be able to switch it we need a different kind of wrapper function, that is only evaluated when the view is called.

+
+ def test_piston_view(self):
+ view = SwitchToDRF('LanguageView')
+ eq_(view(Mock(), 1).__module__, 'django.http')

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 24, 2014

Member

I would have mocked the 2 views and checked the call count on both. Saves you the trouble of really calling them.

@diox

diox Feb 24, 2014

Member

I would have mocked the 2 views and checked the call count on both. Saves you the trouble of really calling them.

@@ -583,6 +593,10 @@ def test_cross_origin(self):
eq_(response['Access-Control-Allow-Methods'], 'GET')
+class DRFAPITest(DRFMixin, APITest):

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 24, 2014

Member

Add a comment. Also, because I'm paranoid, I'd love to have something to make sure the piston views aren't being called here. TestDRFSwitch helps but I'd feel better with something extra added here.

@diox

diox Feb 24, 2014

Member

Add a comment. Also, because I'm paranoid, I'd love to have something to make sure the piston views aren't being called here. TestDRFSwitch helps but I'd feel better with something extra added here.

@diox

View changes

apps/api/renderers.py
+
+ template_name = response.template_name or self.template_name
+ # This is a renderer that ensures a proper escaping.
+ # Jingo.render does not do the job here.

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 26, 2014

Member

What's the issue with jingo rendering ? Why is that happening ?

@diox

diox Feb 26, 2014

Member

What's the issue with jingo rendering ? Why is that happening ?

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 26, 2014

Contributor

Using Jingo's render() just crashes HTML escaping tests.
This code was taken from views.py and actually solves it all.

@vinyll

vinyll Feb 26, 2014

Contributor

Using Jingo's render() just crashes HTML escaping tests.
This code was taken from views.py and actually solves it all.

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 26, 2014

Contributor

if it's only an issue because of jingo, you could also exclude those templates from jingo, and have them automatically rendered using Django's template engine (check the JINGO_EXCLUDE_APPS and JINGO_EXCLUDE_PATHS).

Also, please note https://bugzilla.mozilla.org/show_bug.cgi?id=976727 that reverted the jingo update (I'll be working on fixing that next week).

@magopian

magopian Feb 26, 2014

Contributor

if it's only an issue because of jingo, you could also exclude those templates from jingo, and have them automatically rendered using Django's template engine (check the JINGO_EXCLUDE_APPS and JINGO_EXCLUDE_PATHS).

Also, please note https://bugzilla.mozilla.org/show_bug.cgi?id=976727 that reverted the jingo update (I'll be working on fixing that next week).

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 26, 2014

Contributor

Used template do actually use djingo. We can't exclude it here.

@vinyll

vinyll Feb 26, 2014

Contributor

Used template do actually use djingo. We can't exclude it here.

This comment has been minimized.

Show comment Hide comment
@magopian

magopian Feb 26, 2014

Contributor

Ah, I see, sorry, I misunderstood the issue.

@magopian

magopian Feb 26, 2014

Contributor

Ah, I see, sorry, I misunderstood the issue.

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 26, 2014

Member

Can't you call render_xml_to_string() instead of copy/pasting it then ?

@diox

diox Feb 26, 2014

Member

Can't you call render_xml_to_string() instead of copy/pasting it then ?

@diox

View changes

migrations/756-piston-to-drf.sql
@@ -0,0 +1,2 @@
+INSERT INTO waffle_switch_amo (name, active, note, created, modified)
+ VALUES ('drf', 1, 'Move from Piston to DRF.', NOW(), NOW());

This comment has been minimized.

Show comment Hide comment
@diox

diox Feb 26, 2014

Member

We probably don't want to activate it by default for now.

@diox

diox Feb 26, 2014

Member

We probably don't want to activate it by default for now.

@vinyll

This comment has been minimized.

Show comment Hide comment
@vinyll

vinyll Feb 27, 2014

Contributor

Anything else blocking?
r?

Contributor

vinyll commented Feb 27, 2014

Anything else blocking?
r?

@diox

This comment has been minimized.

Show comment Hide comment
@diox

diox Mar 3, 2014

Member

r+

Member

diox commented Mar 3, 2014

r+

davidbgk added a commit that referenced this pull request Mar 3, 2014

Merge pull request #1749 from vinyll/piston_to_drf-956159
Initializing migration from django-piston to django-restframework (bug 974952)

@davidbgk davidbgk merged commit dd8b782 into mozilla:master Mar 3, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.