11583 leverage cache for related resource relationship preflabel lookup

[whatisgalen]

…ed_resources, re #11583

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

significant speedup, addresses #11583 however it might even warrant a separate PR to just lookup the preflabels in bulk but the underlying logic is a bit more complex for that approach

Issues Solved

addresses #11583

Checklist

• I targeted one of these branches:
  • dev/8.0.x (under development): features, bugfixes not covered below
  • dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
  • dev/6.2.x (extended support): major security issues, data loss issues
• I added a changelog in arches/releases
• I submitted a PR to arches-docs (if appropriate)
• Unit tests pass locally with my changes
• I added tests that prove my fix is effective or that my feature works
• My test fails on the target branch

Accessibility Checklist

Developer Guide

Topic	Changed	Retested
Color contrast
Form fields
Headings
Links
Keyboard
Responsive Design
HTML validation
Screen reader

Ticket Background

• Sponsored by:
• Found by: @
• Tested by: @
• Designed by: @

Further comments

[chiatt]

This seems like a nice performance fix, but something for v8 rather than 7.6.

[whatisgalen]

@chiatt the reason why I'm targeting it as a bugfix is because the difference for a report with +200 related resource relationships, the load time (even with npm run build_production) is ~20 seconds. If a vanilla report takes 60 seconds to load, to me that's pretty much a bug (ditto for #11576)

[chiatt]

@chiatt the reason why I'm targeting it as a bugfix is because the difference for a report with +200 related resource relationships, the load time (even with npm run build_production) is ~20 seconds. If a vanilla report takes 60 seconds to load, to me that's pretty much a bug (ditto for #11576)

The issue isn't whether or not it addresses a bug, but whether it fixes bug that: causes the system to crash, causes data loss, is a regression since 7.5, is a security vulnerability, or is in a new 7.6.0 feature. A performance fix can be justified if it addresses one of those concerns (generally if the system will crash without the fix). #11576 and #11572 are examples.

[chiatt]

If this were reordered a bit, there wouldn't be a need to check the dictionary for the value twice (once to check if the value is not in the preflable_lookup and a second time to retrieve it):

try: 
    preflabel = preflabel_lookup[f'{relation["relationshiptype"]}{lang}']
except KeyError:
    preflabel = get_preflabel_from_valueid(relation["relationshiptype"], lang)
    preflabel_lookup[f'{relation["relationshiptype"]}{lang}'] = preflabel

[whatisgalen]

Given this code below:

    relation["relationshiptype_label"] = preflabel["value"] or ""
except:
    relation["relationshiptype_label"] = (
        relation["relationshiptype"] or ""
    )

while there's some ambiguity as to which exceptions it was meant to handle, at a minimum there's an anticipation that either preflabel["value"] could raise an error (KeyError?) or that relation["relationshiptype"] is None (causing perhaps an ES 404 error downstream in get_preflabel_from_valueid).

[whatisgalen]

If you check the bottom of this trace, you'll see the original KeyError reproduced that the original except was meant to handle, as one outcome

Internal Server Error: /resource/related/b6d2d1e1-9db8-4787-92ca-048b34bf6d66
Traceback (most recent call last):
  File "/Users/galenmancino/repos/arches/arches/app/models/resource.py", line 905, in get_related_resources
    preflabel = preflabel_lookup[f'{relation["relationshiptype"]}{lang}']
                ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: 'Noneen'



During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/galenmancino/repos/venv_312_arches7.6/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/venv_312_arches7.6/lib/python3.12/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/venv_312_arches7.6/lib/python3.12/site-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/venv_312_arches7.6/lib/python3.12/site-packages/django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/arches/arches/app/utils/decorators.py", line 109, in wrapper
    return function(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/venv_312_arches7.6/lib/python3.12/site-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/arches/arches/app/views/resource.py", line 1064, in get
    ret = resource.get_related_resources(
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/arches/arches/app/models/resource.py", line 908, in get_related_resources
    preflabel = get_preflabel_from_valueid(relation["relationshiptype"], lang)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/galenmancino/repos/arches/arches/app/models/concept.py", line 1734, in get_preflabel_from_valueid
    if concept_label["found"]:
       ~~~~~~~~~~~~~^^^^^^^^^
KeyError: 'found'

[chiatt]

Could you add a test for the get_related_resources method to get the coverage check to pass?

[chiatt]

Normally a performance fix would not go in a patch, but this change can prevent a report request, given a resource with a large number of relationships, from failing. It's also a pretty simple change that is unlikely to introduce any bugs.