⚡ [#4229] Cache KVKRemoteValidator result to speed up validation #4244

stevenbal · 2024-04-29T12:53:58Z

Closes #4229

Changes

Cache the result of KVK search in the KVKRemoteValidatorMixin
Add missing VCR cassette for valid RSIN

Checklist

Check off the items that are completed or not relevant.

Impact on features
- Checked copying a form
- Checked import/export of a form
- Config checks in the configuration overview admin page -> no, caching is applied in the validator and not on the KVKClient level
- Problem detection in the admin email digest is handled
Release management
- I have labelled the PR as "needs-backport" accordingly
I have updated the translations assets (you do NOT need to provide translations)
- Ran ./bin/makemessages.sh
- Ran ./bin/compilemessages_js.sh
Commit hygiene
- Commit messages refer to the relevant Github issue
- Commit messages explain the "why" of change, not the how

codecov · 2024-04-29T13:15:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.15%. Comparing base (01d6df0) to head (4687b58).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4244      +/-   ##
==========================================
+ Coverage   96.14%   96.15%   +0.01%     
==========================================
  Files         732      733       +1     
  Lines       23501    23538      +37     
  Branches     2752     2760       +8     
==========================================
+ Hits        22596    22634      +38     
  Misses        639      639              
+ Partials      266      265       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/openforms/conf/locale/nl/LC_MESSAGES/django.po

stevenbal · 2024-04-29T14:14:11Z

@sergei-maertens I just saw #4239 where you update the translation stuff as well, if I should remove my commit that does the same, let me know

sergei-maertens · 2024-04-30T07:54:55Z

@sergei-maertens I just saw #4239 where you update the translation stuff as well, if I should remove my commit that does the same, let me know

Yeah I made that PR so that I could set up this template, so please undo that commit 😬 I had hoped someone would have reviewed my PR by now :(

sergei-maertens · 2024-04-30T07:58:26Z

src/openforms/contrib/kvk/tests/test_validators.py

@@ -49,6 +50,7 @@ def test_invalid_branchNumber(self):
            validate_branchNumber("11223-445566")


+@freeze_time("2024-01-01T12:00:00")


I think I'd prefer a separate test for the caching with this decorator applied, rather than updating an existing test. Decorating the entire test case wasn't needed before, so this incorrectly makes it look like it is.

I moved them to separate tests

sergei-maertens · 2024-04-30T07:59:02Z

src/openforms/contrib/kvk/tests/test_validators.py

+        with self.subTest("caching KVK result"):
+            self.cassette.rewind()
+
+            validator("69599084")
+
+            # No request has been made because the result was cached
+            self.assertEqual(self.cassette.play_count, 0)
+
+            with freeze_time("2024-01-01T12:01:01"):
+                validator("69599084")
+
+            # Request is made, because cached result is timed out
+            self.assertEqual(self.cassette.play_count, 1)


as mentioned above, let's move this into its own test: def test_kvk_lookups_are_cached(self):

I'd also test that doing a call with a different RSIN does create a new request (this tests that the argument is part of the cache key)

added checks for requests with different KVK/RSIN/branchnumber

sergei-maertens · 2024-04-30T08:01:56Z

src/openforms/contrib/kvk/validators.py

@@ -12,6 +14,8 @@

 from .client import NoServiceConfigured, SearchParams, get_kvk_search_client

+KVK_LOOKUP_CACHE_TIMEOUT = 60


I think we can go for 5 minutes easily :)

changed it to 5 mins

sergei-maertens · 2024-05-03T07:27:55Z

src/openforms/contrib/kvk/tests/test_validators.py

+    def setUp(self) -> None:
+        super().setUp()
+
+        clear_caches()


Suggested change

def setUp(self) -> None:

super().setUp()

clear_caches()

def setUp(self) -> None:

super().setUp()

clear_caches()

self.addCleanup(clear_caches)

this ensures that after a test, the cache is cleared too

sergei-maertens · 2024-05-03T07:31:03Z

...KRemoteValidatorTestCase/KvKRemoteValidatorTestCase.test_branchNumber_validator_caching.yaml

+    status:
+      code: 200
+      message: ''
+- request:


this request seems to be duplicated, it's the same as the one before, I think it's because of a particular VCR record mode that just records them again even though they're already present.

Probably best to just delete the cassettes and run the tests again :)

Ah okay, I didn't know you had to delete them if you record new cassettes 👍

sergei-maertens · 2024-05-03T08:26:08Z

src/openforms/contrib/kvk/tests/test_validators.py

@@ -119,3 +123,107 @@ def test_branchNumber_validator(self):
            ValidationError, _("Expected a numerical value.")
        ):
            validator("bork")
+
+
+class KvKRemoteValidatorCachingTestCase(KVKTestMixin, SimpleTestCase):


@stevenbal I ran into test failures with VCR in either of two modes:

Re-recording the cassettes - the play_count is always set to zero because it's still building the cassette

Replaying the cassettess - the self.cassette.requests size reports all the known interactions, and is fine when re-recording the cassettes because it's still building up the history.

There's no obvious way to make checks on which requests were made and how often with VCR, so instead I opted for requests-mock here with a dummy response to check the number of outgoing requests made at any given point.

Hmm alright, I wasn't sure about the VCR approach for the caching assertions either, so this is probably the best way to fix it

yeah this didn't feel very "exact" when to use which approach to me either

and add cassette for valid RSIN

src/openforms/contrib/kvk/validators.py

stevenbal marked this pull request as draft April 29, 2024 12:54

stevenbal commented Apr 29, 2024

View reviewed changes

src/openforms/conf/locale/nl/LC_MESSAGES/django.po Outdated Show resolved Hide resolved

stevenbal requested a review from sergei-maertens April 29, 2024 14:11

stevenbal marked this pull request as ready for review April 29, 2024 14:11

sergei-maertens reviewed Apr 30, 2024

View reviewed changes

stevenbal force-pushed the feature/4229-kvk-validator-cache branch from 513de95 to ab9a5b5 Compare May 2, 2024 13:22

stevenbal marked this pull request as draft May 2, 2024 13:56

⚡ [#4229] Cache KVKRemoteValidator result to speed up validation

42829b7

stevenbal force-pushed the feature/4229-kvk-validator-cache branch from ab9a5b5 to 40b184b Compare May 2, 2024 14:23

stevenbal requested a review from sergei-maertens May 2, 2024 14:24

stevenbal marked this pull request as ready for review May 2, 2024 14:25

sergei-maertens approved these changes May 3, 2024

View reviewed changes

sergei-maertens force-pushed the feature/4229-kvk-validator-cache branch from 40b184b to 01eed37 Compare May 3, 2024 08:23

sergei-maertens reviewed May 3, 2024

View reviewed changes

✅ [#4229] Test KVK results caching for validation

4687b58

and add cassette for valid RSIN

sergei-maertens force-pushed the feature/4229-kvk-validator-cache branch from 01eed37 to 4687b58 Compare May 3, 2024 08:31

Viicos reviewed May 3, 2024

View reviewed changes

src/openforms/contrib/kvk/validators.py Show resolved Hide resolved

sergei-maertens merged commit 055beaa into master May 3, 2024
28 checks passed

sergei-maertens deleted the feature/4229-kvk-validator-cache branch May 3, 2024 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚡ [#4229] Cache KVKRemoteValidator result to speed up validation #4244

⚡ [#4229] Cache KVKRemoteValidator result to speed up validation #4244

stevenbal commented Apr 29, 2024 •

edited

codecov bot commented Apr 29, 2024 •

edited

stevenbal commented Apr 29, 2024

sergei-maertens commented Apr 30, 2024

sergei-maertens Apr 30, 2024

stevenbal May 2, 2024

sergei-maertens Apr 30, 2024

stevenbal May 2, 2024

sergei-maertens Apr 30, 2024

stevenbal May 2, 2024

sergei-maertens May 3, 2024

sergei-maertens May 3, 2024

stevenbal May 7, 2024

sergei-maertens May 3, 2024

stevenbal May 7, 2024

sergei-maertens May 7, 2024

		@@ -49,6 +50,7 @@ def test_invalid_branchNumber(self):
		validate_branchNumber("11223-445566")


		@freeze_time("2024-01-01T12:00:00")

		@@ -12,6 +14,8 @@

		from .client import NoServiceConfigured, SearchParams, get_kvk_search_client

		KVK_LOOKUP_CACHE_TIMEOUT = 60

⚡ [#4229] Cache KVKRemoteValidator result to speed up validation #4244

⚡ [#4229] Cache KVKRemoteValidator result to speed up validation #4244

Conversation

stevenbal commented Apr 29, 2024 • edited

codecov bot commented Apr 29, 2024 • edited

Codecov Report

stevenbal commented Apr 29, 2024

sergei-maertens commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenbal commented Apr 29, 2024 •

edited

codecov bot commented Apr 29, 2024 •

edited