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

Compatibility with Django 4.2 #480

Merged
merged 8 commits into from
Jul 25, 2023
Merged

Compatibility with Django 4.2 #480

merged 8 commits into from
Jul 25, 2023

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Jun 20, 2023

Introduction

This PR is a follow on from the Django 4.0.x compatibility added in #458.

The strategy followed in this PR is to first allow Django 4.2.x and then make all the changes in a way that is also still compatible with Django 3.2. This should allow us to work with both versions and then revert the singular commit allowing Django 4.2 if desired and when we are ready.

Release notes for the major versions between our current 4.0.x and the 4.2.x target:

The django-pipeline dependency has been updated to the latest version in order to support Django 4.2.x. django-redis has been updated to the latest version for better Django 4.2.x compatibility. All other dependencies have been left alone.

NOTE: The supported Python versions for Django 4.2.x are currently 3.8, 3.9, 3.10, and 3.11.

Per-version specific concerns

Django 4.1.x (deprecations from Django 3.2.x removed)

See:

Detail:

  • django.contrib.sessions.serializers.PickleSerializer is deprecated due to the risk of remote code execution.

Django 4.2.x (no past deprecates from earlier versions of Django were removed)

See:

Detail:

  • Our own copy of GzipMiddleware, not in use and has not had substantial updates since 2010, has been removed
  • The minimum supported version of redis-py is increased from 3.0.0 to 3.4.0.
  • The minimum supported version of Pillow is increased from 6.2.0 to 6.2.1.

@ome ome deleted a comment from snoopycrimecop Jun 21, 2023
@will-moore
Copy link
Member Author

So this was the only change needed for me to run omero-web with Django 4.2.2 locally.
I tried updating to Django 4.2.2 on merge-ci but that failed at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/208/console with:

19:56:39 + omero config append omero.web.csrf_trusted_origins '"https://merge-ci.openmicroscopy.org/"'
19:56:40 Cannot retrieve default value for property omero.web.csrf_trusted_origins: Requested setting SESSION_COOKIE_NAME, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

Strange error. I moved that config line till later in the script, which resulted in a more cryptic failure much later on..

https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/209/console

20:07:51 + omero web config nginx --http 80
20:07:52 Error printing text
20:07:52 Build step 'Execute shell' marked build as failure

Reverting back to Django 4.0.10 restored the build.

@will-moore
Copy link
Member Author

will-moore commented Jun 21, 2023

@will-moore
Copy link
Member Author

Googling for "Error printing text" finds https://forum.image.sc/t/installing-omero-web-server/67398/3

Just testing locally, it looks like the same issue I first saw above: #480 (comment)

$ omero web config nginx --http 80
Error printing text
Requested setting SESSION_COOKIE_NAME, but settings are not configured. You must either define the environment variable DJANGO_SETTINGS_MODULE or call settings.configure() before accessing settings.

@will-moore
Copy link
Member Author

So that last commit fixes the merge-ci deploy with Django 4.2.2: https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/217/console
I don't know what changed in Django 4.2.0, but something probably caused settings to be imported before we set DJANGO_SETTINGS_MODULE. Simply setting this env variable earlier fixes it.

cc @sbesson @chris-allan @jburel

@chris-allan
Copy link
Member

In 4.2 the changes in django/django#15352 were made which check if the key matches settings.SESSION_COOKIE_NAME. This of course requires the Django settings infrastructure to be initialized. The decisions around why to use the settings cleansing implementation from django.views.debug were made over 10 years ago in ome/openmicroscopy#80 and are probably lost to the annals of time.

I'm quite concerned that the requirement for those settings to be available during our own settings processing might create race conditions or other unintended consequences now or in the future. Basically, we've been using that filter beyond its intended use case for some time and that's likely never been a good idea; cleanse_setting() was "internal" Django API (SafeExceptionReporterFilter is what's advertised in the documentation1) back when we addressed changes to it for the Django 3.2 upgrade in #356.

What I'd recommend is that we not hack around this by messing with the order of DJANGO_SETTINGS_MODULE environment variable population. Instead, I'd suggest we bring cleanse_setting() into our codebase (perhaps the version from Django 3.0 verbatim or a modified version of the method from 4.2.2) where we're not subject to design decisions made in django.views.debug that are outside our control and likely to be undocumented. I probably should have done this in #356 rather than update to the new API in 864a352.

/cc @sbesson

Footnotes

  1. https://docs.djangoproject.com/en/3.0/howto/error-reporting/#django.views.debug.SafeExceptionReporterFilter

@chris-allan chris-allan changed the title Django 4 2 Compatibility with Django 4.2 Jul 3, 2023
@chris-allan
Copy link
Member

Few changes in addition to the settings debug stuff mentioned above. Description, etc. updated.

/cc @sbesson, @knabar

@chris-allan chris-allan requested review from knabar and sbesson July 3, 2023 14:46
Copy link
Member

@knabar knabar left a comment

Choose a reason for hiding this comment

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

Changes look good and make sense to me

@knabar knabar added this to the 5.22.0 milestone Jul 4, 2023
Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

The code changes are relatively minimal and sizeable, in part due to the preliminary work that has been taken place as separate PRs.

Deployed the application both locally with a development server and a file-based session store and on a testing server with Nginx and Redis. In both cases the application started without issue and I was able to achieve the minimal workflows (login, image browsing, admin tab, image opening).
Finally, this has been successfully integrated in the nightly build of the OME Jenkins CI and deployed alongside various OMERO.web apps including OMERO.iviewer, OMERO.iviewer. Here again , a quick round of testing did not highlight any outstanding issues.

In terms of roadmap, my inclination would be to
1- only support compatibility with a single Django version for any given release
2- target an upgrade of Django to 4.2 as part of the upcoming release.
Having additional reviewers would be useful but to some extent, the next logical step will probably be to get this merged and start planning the testing & release timeline.

Looking at the backwards-incompatible changes, possibly the biggest question as per the deprecation of django.contrib.sessions.serializers.PickleSerializer is whether we are also scheduling to update the default value of omero.web.session_serializer to use the JSON serializer (all the testing above was performed using the legacy pickle serializer)

@will-moore
Copy link
Member Author

Thanks @chris-allan for fixing that.
This is working fine for me testing with dev-server locally, and on merge-ci.

I don't have a strong view on whether we should support a single or multiple versions of Django in future releases.
But in bumping from 3.2 > 4.2 we drop support for python 3.6 and 3.7.

👍 (I can't approve my own PR!)

@sbesson
Copy link
Member

sbesson commented Jul 17, 2023

But in bumping from 3.2 > 4.2 we drop support for python 3.6 and 3.7.

Python 3.6 and Python 3.7 are EOL and several other dependencies in the OMERO ecosystem have already raised their minimal version to Python 3.8 if not greater like NumPy which is now Python 3.9+. I don't perceive this as an issue, we are simply updating our stack to comply with the latest supported base requirements.

In all cases, thanks for raising this point. Definitely worth including both in the release notes as well as in the upgrading section.

@knabar knabar merged commit 50d29ed into ome:master Jul 25, 2023
@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/release-of-omero-server-5-6-8-and-omero-web-5-22-0/84157/1

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/omero-figure-dev-install-with-docker-fails/86022/2

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.

5 participants