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

Review and possible deprecate NonASCIIForm #468

Closed
sbesson opened this issue May 5, 2023 · 0 comments · Fixed by #473
Closed

Review and possible deprecate NonASCIIForm #468

sbesson opened this issue May 5, 2023 · 0 comments · Fixed by #473
Assignees
Milestone

Comments

@sbesson
Copy link
Member

sbesson commented May 5, 2023

As part of the investigation of #464, #464 (comment) 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:

def full_clean(self):
"""
Cleans all of self.data and populates self._errors and
self.cleaned_data.
"""
self._errors = ErrorDict()
if not self.is_bound: # Stop further processing.
return
self.cleaned_data = {}
# If the form is permitted to be empty, and none of the form data has
# changed from the initial data, short circuit any validation.
if self.empty_permitted and not self.has_changed():
return
for name, field in self.fields.items():
# value_from_datadict() gets the data from the data dictionaries.
# Each widget type knows how to retrieve its own data, because
# some widgets split data over several HTML fields.
value = field.widget.value_from_datadict(
self.data, self.files, self.add_prefix(name)
)
try:
if isinstance(field, FileField):
initial = self.initial.get(name, field.initial)
value = field.clean(value, initial)
elif isinstance(field, CharField):
if (
value is not None
and isinstance(value, basestring)
and len(value) > 0
):
value = str(smart_str(value))
else:
value = field.clean(value)
else:
value = field.clean(value)
self.cleaned_data[name] = value
if hasattr(self, "clean_%s" % name):
value = getattr(self, "clean_%s" % name)()
self.cleaned_data[name] = value
except ValidationError as e:
self._errors[name] = self.error_class(e.messages)
if name in self.cleaned_data:
del self.cleaned_data[name]
try:
self.cleaned_data = self.clean()
except ValidationError as e:
self._errors[forms.Form.NON_FIELD_ERRORS] = self.error_class(e.messages)
if self._errors:
delattr(self, "cleaned_data")

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

elif isinstance(field, CharField):
if (
value is not None
and isinstance(value, basestring)
and len(value) > 0
):
value = str(smart_str(value))
else:
value = field.clean(value)

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 ccd4c05 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.

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 a pull request may close this issue.

1 participant