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

Fix performance regression in FormioData data structure #4768

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ django-sessionprofile==2.0.0
# -c requirements/ci.txt
# -r requirements/ci.txt
# django-digid-eherkenning
django-silk==5.1.0
django-silk==5.2.0
# via -r requirements/dev.in
django-simple-certmanager==2.4.1
# via
Expand Down
35 changes: 30 additions & 5 deletions src/openforms/formio/datastructures.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import re
from collections import UserDict
from collections.abc import Hashable
from typing import Iterator, cast

from glom import PathAccessError, assign, glom
Expand Down Expand Up @@ -162,16 +161,42 @@ class FormioData(UserDict):
"""

data: dict[str, JSONValue]
_keys: set[str]
"""
A collection of flattened key names, for quicker __contains__ access
"""

def __getitem__(self, key: Hashable):
def __init__(self, *args, **kwargs):
self._keys = set()
super().__init__(*args, **kwargs)

def __getitem__(self, key: str):
if "." not in key:
return self.data[key]
return cast(JSONValue, glom(self.data, key))

def __setitem__(self, key: Hashable, value: JSONValue):
def __setitem__(self, key: str, value: JSONValue):
assign(self.data, key, value, missing=dict)
self._keys.add(key)

def __contains__(self, key: object) -> bool:
"""
Check if the key is present in the data container.

This gets called via ``formio_data.get(...)`` to check if the default needs to
be returned or not. Keys are expected to be strings taken from ``variable.key``
fields.
"""
if not isinstance(key, str):
raise TypeError("Only string keys are supported")

# for direct keys, we can optimize access and bypass glom + its exception
# throwing.
if "." not in key:
return key in self._keys

def __contains__(self, key: Hashable) -> bool:
try:
self[key]
return True
except PathAccessError:
return False
return True
14 changes: 14 additions & 0 deletions src/openforms/formio/tests/test_datastructures.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,20 @@ def test_initializing_with_dotted_paths_expands(self):

self.assertEqual(formio_data, expected)

def test_key_access_must_be_string(self):
formio_data = FormioData({"foo": "bar"})

bad_keys = (
3,
None,
4.3,
("foo",),
)

for key in bad_keys:
with self.assertRaises(TypeError):
key in formio_data # type: ignore


class FormioConfigurationWrapperTests(TestCase):

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
from django.db.models import F

import glom
from glom import PathAccessError

from openforms.authentication.service import AuthAttribute
from openforms.contrib.objects_api.clients import (
Expand Down Expand Up @@ -537,7 +536,7 @@ def get_record_data(
for key, variable in state.variables.items():
try:
submission_value = dynamic_values[key]
except PathAccessError:
except KeyError:
continue

# special casing documents - we transform the formio file upload data into
Expand Down
Loading