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

Django Upgrade #3123

Merged
merged 15 commits into from Jul 24, 2019
Merged

Django Upgrade #3123

merged 15 commits into from Jul 24, 2019

Conversation

@rajadain
Copy link
Member

rajadain commented Jul 15, 2019

Overview

Upgrades Django to latest long term supported version for security patches. This is the last version of Django to support Python 2.

Connects #3118
Connects #3105

Notes

There are two known issues with the upgrade that were not resolved, and have been extracted into their own cards:

  • Upgrade Django Rest Swagger #3124
  • Verbose Logging for 404s #3125
  • Celery chord_unlock Loop #3126

The Swagger update is higher priority, since currently our API docs are broken. The verbose logging is a minor annoyance. The Celery Loop is hard to recreate, but could hold up workers in production if happening consistently.

Testing Instructions

  • Check out this branch and destroy and recreate app and worker.
    • Also destroy and recreate tiler because the tiler service doesn't work after the first time #3112.
  • Run migrations manage.sh migrate.
  • Run debugserver and debugcelery.
    • Also start debugtiler to ensure it is working correctly.
  • Go to :8000/.
  • Ensure you are not logged in.
  • Draw a shape and proceed to Analyze. Ensure the analysis completes successfully.
  • Go to the Monitor tab and search for items in all four catalogs. Ensure they return results correctly. Spot check the detail pages of those results to ensure they work too.
  • Go to the Model tab and create a TR-55 project. Ensure it is created correctly.
  • Add a new scenario, and add some changes to it.
  • Open the Compare View and ensure it works correctly.
  • Go back to the home page.
  • Sign up for a new account. Ensure you see the email output in the console correctly. Log in with it.
  • Search for a location on the map. Ensure you get suggestions. Ensure you can select a suggestion.
  • Select a HUC-10 or HUC-8. Proceed to Analyze.
  • Go the Model tab and create a MapShed project. Ensure it is created correctly.
  • Add a new scenario. Make some changes. Ensure it works correctly.
  • Go to the Water Quality tab, and select Subbasin
  • Ensure subbasin results come through correctly

Checklist

  • All JavaScript tests pass ./scripts/testem.sh
@rajadain rajadain force-pushed the tt/upgrade-django branch from f92e11d to 8428007 Jul 16, 2019
rajadain added 5 commits Jul 16, 2019
This is the latest stable version that supports Python 2.7.

Also upgrade some supporting libraries.
This is the last version that supports Python 2.7.

All serializers need an explicit field list now, so that is
added wherever it was missing. Extraneous serializers that
were not used anywhere were removed.

Read-only complex serialized fields now need explicit code
to save in new instances, so that is also added.
Change to the new version of using the app by adding to the
THIRD_PARTY_APPS list and replacing the now deprecated
GeoModelSerializer with a regular ModelSerializer.
- Replace render_to_response with render
- Move custom context processor to be within template
- Ensure context variables are always defined and have defaults
- Use == instead of = for equality test within template
- Use new TEMPLATES configuration setting rather than the
  separate TEMPLATE_CONTEXT_PROCESSORS, TEMPLATE_LOADERS, etc
@rajadain rajadain force-pushed the tt/upgrade-django branch from 8428007 to 23dc740 Jul 17, 2019
@rajadain rajadain changed the title WIP: Django Upgrade Django Upgrade Jul 17, 2019
@rajadain rajadain marked this pull request as ready for review Jul 17, 2019
@rajadain rajadain requested a review from caseycesari Jul 18, 2019
@caseycesari

This comment has been minimized.

Copy link
Member

caseycesari commented Jul 18, 2019

When running debugserver and visiting the http://localhost:8000, I'm see the following. It looks like the same error, but note the URL at the end error is different.

Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.monitoring.urls' from '/opt/app/apps/monitoring/urls.pyc'> (monitoring:monitoring) ^health-check/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'rest_framework.urls' from '/usr/local/lib/python2.7/dist-packages/rest_framework/urls.pyc'> (rest_framework:rest_framework) ^api-auth/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'registration.backends.default.urls' from '/usr/local/lib/python2.7/dist-packages/registration/backends/default/urls.pyc'> (None:None) ^accounts/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.bigcz.urls' from '/opt/app/apps/bigcz/urls.pyc'> (bigcz:bigcz) ^bigcz/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.geocode.urls' from '/opt/app/apps/geocode/urls.pyc'> (geocode:geocode) ^mmw/geocode/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.modeling.urls' from '/opt/app/apps/modeling/urls.pyc'> (modeling:modeling) ^mmw/modeling/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.export.urls' from '/opt/app/apps/export/urls.pyc'> (export:export) ^export/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.geoprocessing_api.urls' from '/opt/app/apps/geoprocessing_api/urls.pyc'> (geoprocessing_api:geoprocessing_api) ^api/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.water_balance.urls' from '/opt/app/apps/water_balance/urls.pyc'> (water_balance:water_balance) ^micro/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u"<RegexURLResolver <module 'apps.user.urls' from '/opt/app/apps/user/urls.pyc'> (user:user) ^user/>"
Exception while resolving variable 'name' in template 'unknown'.
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/template/base.py", line 903, in _resolve_lookup
    (bit, current))  # missing attribute
VariableDoesNotExist: Failed lookup for key [name] in u'<RegexURLResolver <RegexURLPattern list> (admin:admin) ^admin/>'
Not Found: /version.txt
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 18, 2019

That's covered in #3125

@caseycesari

This comment has been minimized.

Copy link
Member

caseycesari commented Jul 18, 2019

I don't have all the data loaded yet, but I'm seeing this error when analyze is kicked off and jobs are polled.

Internal Server Error: /api/analyze/climate/
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/exception.py", line 41, in inner
    response = get_response(request)
  File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 187, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py", line 185, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/csrf.py", line 58, in wrapped_view
    return view_func(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/django/views/generic/base.py", line 68, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 495, in dispatch
    response = self.handle_exception(exc)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 455, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py", line 492, in dispatch
    response = handler(request, *args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/rest_framework/decorators.py", line 55, in handler
    return func(*args, **kwargs)
  File "/opt/app/apps/core/decorators.py", line 28, in decorator
    view_result = view(request, *args, **kwargs)
  File "/opt/app/apps/geoprocessing_api/views.py", line 1176, in start_analyze_climate
    ], area_of_interest, user, link_error=False)
  File "/opt/app/apps/geoprocessing_api/views.py", line 1323, in start_celery_job
    headers={'Location': reverse('get_job', args=[task_chain.id])}
  File "/usr/local/lib/python2.7/dist-packages/django/urls/base.py", line 91, in reverse
    return force_text(iri_to_uri(resolver._reverse_with_prefix(view, prefix, *args, **kwargs)))
  File "/usr/local/lib/python2.7/dist-packages/django/urls/resolvers.py", line 497, in _reverse_with_prefix
    raise NoReverseMatch(msg)
NoReverseMatch: Reverse for 'get_job' not found. 'get_job' is not a valid view function or pattern name.

This happens for all of the analyze components.

image

The celery jobs for each component however, appear to be working.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 18, 2019

Wow yeah I'm seeing that too. Not sure why that didn't happen earlier. Fixing it now.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 18, 2019

Fixed in ecba1c2.

@caseycesari

This comment has been minimized.

Copy link
Member

caseycesari commented Jul 19, 2019

Seeing this error when trying to confirm a new account via the email link

image

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 19, 2019

Registration URL errors should be fixed in 1acd5e3

@caseycesari

This comment has been minimized.

Copy link
Member

caseycesari commented Jul 19, 2019

When clicking "I'll do this later" on the profile information modal, I got the following error:

AttributeError at /user/profile
This QueryDict instance is immutable

Traceback:  

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/exception.py" in inner
  41.             response = get_response(request)

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py" in _get_response
  187.                 response = self.process_exception_by_middleware(e, request)

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py" in _get_response
  185.                 response = wrapped_callback(request, *callback_args, **callback_kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/csrf.py" in wrapped_view
  58.         return view_func(*args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/generic/base.py" in view
  68.             return self.dispatch(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py" in dispatch
  495.             response = self.handle_exception(exc)

File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py" in handle_exception
  455.             self.raise_uncaught_exception(exc)

File "/usr/local/lib/python2.7/dist-packages/rest_framework/views.py" in dispatch
  492.             response = handler(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/rest_framework/decorators.py" in handler
  55.             return func(*args, **kwargs)

File "/opt/app/apps/user/views.py" in profile
  114.         data['was_skipped'] = str(data['was_skipped']).lower() == 'true'

File "/usr/local/lib/python2.7/dist-packages/django/http/request.py" in __setitem__
  435.         self._assert_mutable()

File "/usr/local/lib/python2.7/dist-packages/django/http/request.py" in _assert_mutable
  432.             raise AttributeError("This QueryDict instance is immutable")

Exception Type: AttributeError at /user/profile
Exception Value: This QueryDict instance is immutable

image

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 19, 2019

That should be fixed in 1b41bf8.

Copy link
Member

caseycesari left a comment

Looking good! Tested all of the fixups after reporting bugs, verified they were indeed fixed.

Still might be worth having another person run through things, though.

@caseycesari caseycesari assigned rajadain and unassigned caseycesari Jul 19, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 19, 2019

Thanks for finding all the bugs! Think I'll have @mmcfarland take a look next week if he can. Will squash the fixups for now.

@rajadain rajadain requested a review from mmcfarland Jul 19, 2019
@rajadain rajadain assigned mmcfarland and unassigned rajadain Jul 19, 2019
rajadain added 2 commits Jul 17, 2019
request.body is now a stream instead of a variable, and its
value is read somewhere up the chain, thus it can't be read
again at this point. We switch to request.data instead,
which is already a dict, thus supplanting the json.loads.

Also don't modify request.data, instead make a copy and modify
that, since request.data is now immutable.
This used to be a function, now it is a property.
rajadain added 5 commits Jul 17, 2019
The generic request.REQUEST is deprecated in favor of the
more explicit alternatives.
Previously, all ForeignKey and OneToOne fields implicitly
assumed on_delete to be CASCADE. Now this specification is
required to be explicit.

All models are upgraded to have on_delete specified in their
ForeignKey and OneToOne fields. In most cases, we stick with
the default CASCADE behavior. The exceptions are:

- The User field on Project is set to PROTECT. This is to
  prevent the accidental deletion of projects if a user is
  deleted. To delete a user, their projects must be deleted
  manually.
- The Mapshed_Job and Subbasin_Mapshed_Job fields on Project
  are set to SET_NULL. This is to allow the recalculation
  of Mapshed and Subbasin values for a project without
  deleting the project itself.
- The User field on Job is set to SET_NULL. This is because
  the field already allows NULLs, and having historic job
  records may be useful even when there's no associated user.

New migrations were created to specify these constraints
explicitly. Furthermore, existing migrations were manually
updated to also have the same constraints. This will have
no impact on production, since existing migrations will not
be re-run. This does help pacify the deprecation warnings
that Django generates.
For live tests that exercise direct endpoints, use reverse
rather than a full URL. Also use django.test.Client instead
of requests.

The test Client's response.url is now a relative URL, not
an absolute one. Remove the extraction code which is not
needed anymore.

The test Client also doens't use the params= argument anymore.
Switch to giving it the request body directly.
@rajadain rajadain force-pushed the tt/upgrade-django branch from 1b41bf8 to 8eb8144 Jul 19, 2019
@mmcfarland

This comment has been minimized.

Copy link
Member

mmcfarland commented Jul 23, 2019

Getting some 500s from a few Monitor detail related fetches. Used a "sediment" search around Philly to get to this site.

Update: This happens on production, too so is likely an unrelated error

    site_code = site_info.find(namespace + "siteCode")
AttributeError: 'NoneType' object has no attribute 'find'

Screenshot from 2019-07-23 10:04:48

@mmcfarland

This comment has been minimized.

Copy link
Member

mmcfarland commented Jul 23, 2019

I created a user and activated it successfully. However, trying to log in with that user gives a

Forbidden (CSRF token missing or incorrect.): /user/login

When trying to log in. Same result after page refresh.

Copy link
Member

mmcfarland left a comment

Largely looking good except for the new user login error. I tried several times with the same result.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 24, 2019

Ah sorry, but good catch. I added an addition @csrf_protect to the login view to make some tests pass in 86eb0db, but as it is interfering with functionality I'm just going to comment those tests out.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 24, 2019

The 500s in Monitor are transitive errors from the underlying APIs we're fetching from, and is a known issue.

rajadain added 3 commits Jul 18, 2019
This will now not fail installation on minor version changes.
Since we're not using Upstart for scripts anymore, going
with systemd instead, we have to use `sudo service` instead
of `sudo start`.
The endpoint these tests were inspecting do not require
CSRF protection, thus to test for it was extraneous.

Since the upgrade, the tests began to fail as they would
always return correct values. Since the tests are no
longer necessary, they are removed.
@rajadain rajadain force-pushed the tt/upgrade-django branch from 8eb8144 to 39e7d4c Jul 24, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 24, 2019

Removed the commit that added extra CSRF protection, and added one that removes the failing tests 39e7d4c. Login is working correctly on my end, would appreciate confirmation from yours.

Copy link
Member

mmcfarland left a comment

Registration/login/saved workflow is now working. Everything looks good 👍

@mmcfarland mmcfarland assigned rajadain and unassigned mmcfarland Jul 24, 2019
@rajadain rajadain merged commit 2ebbe1c into develop Jul 24, 2019
2 checks passed
2 checks passed
default Build finished.
Details
model-my-watershed-pull-requests Build #4065 succeeded in 9 min 32 sec
Details
@rajadain rajadain deleted the tt/upgrade-django branch Jul 24, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Jul 24, 2019

Thanks for the thorough reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.