ome / omero-web

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

Review and possible deprecate NonASCIIForm #468

Closed sbesson closed 1 year ago

sbesson commented 1 year ago

As part of the investigation of #464, https://github.com/ome/omero-web/issues/464#issuecomment-1533272993 reminded of the existence of omeroweb.custom_forms.NonASCIIForm which is used as the superclass of several forms of webclient and webadmin.

This class simply extends django.forms.Form and overrides the full_clean method which is invoked when calling Form.is_valid:

https://github.com/ome/omero-web/blob/c706c42fb5495dd40833ec7f3be0450c2fab280b/omeroweb/custom_forms.py#L34-L82

is largely a copy of the upstream implementation which logic is split into several internal methods _clean_fields(), _clean_form() and _post_clean() - see https://github.com/django/django/blob/ca5d3c99efb1bcf181e923dcd00c4679ab6174ef/django/forms/forms.py#L314-L412 and https://github.com/django/django/blob/722e9f8a38f5b34f2423059a75f8a59bb8eb931a/django/forms/forms.py#L306-L351.

The primary divergence is the special logic associated with the validation of CharField https://github.com/ome/omero-web/blob/c706c42fb5495dd40833ec7f3be0450c2fab280b/omeroweb/custom_forms.py#L58-L66

It is worth noting that the implementation of these validation methods has changed upstream between 3.2 and 4.2 - see https://github.com/django/django/blob/fea17b76150688056d78818ea1ef47f1122dc165/django/forms/forms.py#L437-L451 for instance. So at minimum, these changes will need to be reviewed in the context of the upgrade of OMERO.web to Django 4.2 LTS.

Our custom form was introduced over a decade ago in https://github.com/ome/omero-web/commit/ccd4c057d9e2f7c3f5e225262b4b8655801e93f7 in order to address encoding errors with non ASCII characters. Since then, Unicode support of Unicode has changed a lot both with Python 3.x and recent versions of Django and should be much more native. We should review whether our logic is still necessary and if not, deprecate the class and update all forms to directly subclass django.forms.Form.