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

Resource editor raises KeyError when switching languages #10079

Closed
jacobtylerwalls opened this issue Sep 27, 2023 · 8 comments · Fixed by #11362
Closed

Resource editor raises KeyError when switching languages #10079

jacobtylerwalls opened this issue Sep 27, 2023 · 8 comments · Fixed by #11362
Assignees

Comments

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Sep 27, 2023

  1. Enable the language switcher an additional language
  2. Save a new resource model instance
  3. Change to that language
Internal Server Error: /resource/44490b12-feb3-4b09-977c-8b15588baea4
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/views/generic/base.py", line 104, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/contrib/auth/decorators.py", line 23, in _wrapper_view
    return view_func(request, *args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/views/generic/base.py", line 143, in dispatch
    return handler(request, *args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/django/utils/decorators.py", line 46, in _wrapper
    return bound_method(*args, **kwargs)
  File "/web_root/arches/arches/app/utils/decorators.py", line 82, in wrapper
    return function(request, *args, **kwargs)
  File "/web_root/arches/arches/app/views/resource.py", line 277, in get
    serialized_graph = published_graph.serialized_graph
AttributeError: 'NoneType' object has no attribute 'serialized_graph'

Tested with afs
Version 7.5 dev

@chiatt chiatt added this to pipeline Sep 27, 2023
@jacobtylerwalls

This comment was marked as outdated.

@jacobtylerwalls jacobtylerwalls marked this as a duplicate of #10371 Jan 23, 2024
@github-project-automation github-project-automation bot moved this to ✅ Done in pipeline Jan 23, 2024
@jacobtylerwalls
Copy link
Member Author

Can still reproduce

@chrabyrd
Copy link
Contributor

Can you add the steps to reproduce?

@jacobtylerwalls
Copy link
Member Author

Looks like the OP was missing a step. (sorry!)

  1. Publish a graph
  2. Add a new language to settings.py that wasn't there before
  3. Rebuild webpack and refresh
  4. Create an instance of that graph
  5. Switch to the language you added in step 2

Now it strikes me that the problem goes away if I run manage.py graph publish --update after adding the new language. If this is a requirement, we should document it, I figure?

I'll also update the traceback, which is a little different now.

@jacobtylerwalls
Copy link
Member Author

Looks like I was a little confused about whether the error emanated from the graph designer or the resource instance editor. Looks like it's only emanating from the latter now. I'll retitle.

@jacobtylerwalls jacobtylerwalls changed the title Graph Designer raises KeyError when switching languages Resource editor raises KeyError when switching languages Feb 22, 2024
@chrabyrd
Copy link
Contributor

Looks like the OP was missing a step. (sorry!)

  1. Publish a graph
  2. Add a new language to settings.py that wasn't there before
  3. Rebuild webpack and refresh
  4. Create an instance of that graph
  5. Switch to the language you added in step 2

Now it strikes me that the problem goes away if I run manage.py graph publish --update after adding the new language. If this is a requirement, we should document it, I figure?

I'll also update the traceback, which is a little different now.

No worries!

The Graph.publish command iterates settings.LANGUAGES and not the languages table so there's not really anything to hang a trigger onto. Also automatically triggering a serialized save could result in unpredictable behavior. So yeah this looks to be a documentation issue. Alternatively I'm going to put a small timebox on this and investigate how easy it would be to gracefully fall back to the un-serialized graph.

@jacobtylerwalls
Copy link
Member Author

Nice. In terms of documentation, I bet you could throw a system check from the PublishedGraph class that tells you to run the update command if you're missing publications for some language. There's a little example here. Then you wouldn't even be able to runserver if you end up in this situation.

@chrabyrd
Copy link
Contributor

chrabyrd commented Mar 4, 2024

@jacobtylerwalls I've assigned a PR to you here #10652 that should address the issue.

Unfortunately I couldn't override the check attr on models without making it a class method, and as a class method we can't check any specifics about the actual graph. This worked locally for me and hopefully it should work on yours as well but let me know. I also removed the raise_if_missing arg since with this change there isn't a conceivable way to accidentally have an unpublished graph ( as far as I know ). However if it needs to be re-added please let me know.

chrabyrd added a commit that referenced this issue Mar 15, 2024
@jacobtylerwalls jacobtylerwalls linked a pull request Aug 19, 2024 that will close this issue
11 tasks
@jacobtylerwalls jacobtylerwalls linked a pull request Aug 19, 2024 that will close this issue
11 tasks
jacobtylerwalls added a commit that referenced this issue Aug 19, 2024
jacobtylerwalls added a commit that referenced this issue Aug 20, 2024
jacobtylerwalls added a commit that referenced this issue Sep 11, 2024
@jacobtylerwalls jacobtylerwalls self-assigned this Sep 11, 2024
chrabyrd added a commit that referenced this issue Nov 1, 2024
…age-check

Add check for published graph in every system language #10079
@github-project-automation github-project-automation bot moved this from 👀 In Review to ✅ Done in pipeline Nov 1, 2024
chrabyrd added a commit that referenced this issue Nov 2, 2024
Automate the changes to the github action template re #10079
jacobtylerwalls added a commit that referenced this issue Nov 21, 2024
This model check should have been skippped by checking
the `databases` kwarg provided when `check()` is called,
but that reduces the circumstances on which this check is
run to either:
- manage.py migrate
- manage.py check --databases

So, given how infrequently this would be run, better to just
implement this in our validation framework.
jacobtylerwalls added a commit that referenced this issue Nov 21, 2024
This model check should have been skippped by checking
the `databases` kwarg provided when `check()` is called,
but that reduces the circumstances on which this check is
run to either:
- manage.py migrate
- manage.py check --databases

So, given how infrequently this would be run, better to just
implement this in our validation framework.
jacobtylerwalls added a commit that referenced this issue Nov 21, 2024
This model check should have been skippped by checking
the `databases` kwarg provided when `check()` is called,
but that reduces the circumstances on which this check is
run to either:
- manage.py migrate
- manage.py check --databases

So, given how infrequently this would be run, better to just
implement this in our validation framework.
jacobtylerwalls added a commit that referenced this issue Nov 23, 2024
jacobtylerwalls added a commit that referenced this issue Dec 2, 2024
jacobtylerwalls added a commit that referenced this issue Dec 3, 2024
* Rewrite model check as validation check re #10079

This model check should have been skippped by checking
the `databases` kwarg provided when `check()` is called,
but that reduces the circumstances on which this check is
run to either:
- manage.py migrate
- manage.py check --databases

So, given how infrequently this would be run, better to just
implement this in our validation framework. This also prevents
the possibility of failures when migrating.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment