Skip to content

Commit

Permalink
Merge pull request #4768 from open-formulieren/issue/4744-performance…
Browse files Browse the repository at this point in the history
…-regression

Fix performance regression in FormioData data structure
  • Loading branch information
sergei-maertens authored Oct 22, 2024
2 parents a755cd5 + d2bd3cf commit 12a4c89
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 10 deletions.
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
42 changes: 35 additions & 7 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,45 @@ 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):
return cast(JSONValue, glom(self.data, key))
def __init__(self, *args, **kwargs):
self._keys = set()
super().__init__(*args, **kwargs)

def __setitem__(self, key: Hashable, value: JSONValue):
def __getitem__(self, key: str):
if "." not in key:
return self.data[key]
try:
return cast(JSONValue, glom(self.data, key))
except PathAccessError as exc:
raise KeyError(f"Key '{key}' is not present in the data") from exc

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]
except PathAccessError:
return True
except KeyError:
return False
return True
28 changes: 28 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,34 @@ 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

def test_keyerror_for_absent_keys(self):
formio_data = FormioData({})
bad_keys = (
"foo",
"bar.baz",
)

for key in bad_keys:
with (
self.subTest(key=key),
self.assertRaises(KeyError),
):
formio_data[key]


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

0 comments on commit 12a4c89

Please sign in to comment.