ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
16 stars 29 forks source link

Compatibility with Django 4.2 #480

Closed will-moore closed 12 months ago

will-moore commented 1 year ago

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 4.2.x (no past deprecates from earlier versions of Django were removed)

See:

Detail:

will-moore commented 1 year ago

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 commented 1 year ago

Django 4.1.9 works with this PR: https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/214/ but Django 2.0.0 doesn't: https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/215/console

will-moore commented 1 year ago

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: https://github.com/ome/omero-web/pull/480#issuecomment-1601085477

$ 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 commented 1 year ago

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 commented 1 year ago

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 documentation^1) 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 864a352f96f8bb9f890cd121265b9691a22d4460.

/cc @sbesson

chris-allan commented 1 year ago

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

/cc @sbesson, @knabar

will-moore commented 1 year ago

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 commented 1 year ago

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.

imagesc-bot commented 12 months ago

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 commented 10 months ago

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