WIP: Support changing servertypes #57
Conversation
RonjaPonja
changed the title
| involved_servertypes = chain(*[ | ||
| [change['servertype']['old'], change['servertype']['new']] | ||
| for change in servertype_changes | ||
| ]) |
There was a problem hiding this comment.
Actually this doesn't seem to get flattened without *[] 🤔
Traceback (most recent call last):
File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/exception.py", line 41, in inner
response = get_response(request)
File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/base.py", line 249, in _legacy_get_response
response = self._get_response(request)
File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/base.py", line 187, in _get_response
response = self.process_exception_by_middleware(e, request)
File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/core/handlers/base.py", line 185, in _get_response
response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/Users/space/.local/share/virtualenvs/serveradmin-m3Kn4UAw/lib/python3.5/site-packages/django/contrib/auth/decorators.py", line 23, in _wrapped_view
return view_func(request, *args, **kwargs)
File "./serveradmin/servershell/views.py", line 305, in commit
commit_query(changed=changed, deleted=deleted, user=user)
File "./serveradmin/serverdb/query_committer.py", line 80, in commit_query
_resolve_servertype_changes(changed, joined_attributes)
File "./serveradmin/serverdb/query_committer.py", line 663, in _resolve_servertype_changes
involved_servertype_ids
File "./serveradmin/serverdb/query_committer.py", line 398, in _get_servertype_attributes
for servertype_id in set(servertype_ids):
TypeError: unhashable type: 'list'
There was a problem hiding this comment.
Ah, now I see, chain() doesn't seem to be the right thing though. Some use sum(([...] for x in y), []) to concatenate items, but a simple for to append items is more efficient and more readable.
| # the real attribute changes, resulting from a servertype change, before | ||
| # sending the pre_commit signal. Alternatively we could send the | ||
| # pre_commit signal inside the commit_query transaction but sending the | ||
| # signal can take many seconds if a receiver misbehaves. |
There was a problem hiding this comment.
Check out commit_query(). There we have the real transaction committing changes to the database. You chose to put the signals before and after the transactions as the signals are blocking until all receivers are done. To expand what attributes I have to delete I first have to materialize the object though. I need to expand the changes first as otherwise the pre_commit and post_commit hook would have different values for 'changed'. That would suck.
There was a problem hiding this comment.
So before my change the behavior was:
- pre_commit hook
- transaction
- materialize the objects
- commit the objects
- post_commit hook
Now the behavior is:
- transaction
- materialize objects with changed servertypes
- expand servertype changes to real attribute changes
- pre_commit hook
- transaction
- materialize all objects
- commit the objects
- post_commit hook
There was a problem hiding this comment.
otherwise the pre_commit and post_commit hook would have different values for 'changed'. That would suck.
I think it is fair that they are different. Complicating the flow in here may have more downsides.
| # sending the pre_commit signal. Alternatively we could send the | ||
| # pre_commit signal inside the commit_query transaction but sending the | ||
| # signal can take many seconds if a receiver misbehaves. | ||
| with transaction.atomic(): |
There was a problem hiding this comment.
It is not okay to start another transaction in the same request. If things fail after committing this one, you would end up persisting part of the commit. Whatever critical need to go the .atomic() block started after calling this function.
There was a problem hiding this comment.
Everything that write to the database is "critical". There are more of them later on this function.
There was a problem hiding this comment.
I believe this function doesn't write to the database at all, except for the _fetch_servers row lock, which is why I added this transaction in the first place. I want this function to only expand the changes and then put the changes through the normal commit process. Am I missing something here?
| # commit for now. While this is not super clean, the user will be forced | ||
| # to set a correct value once he opens an edit dialog for this object in | ||
| # servershell. I think that's good enough for now as changing servertypes | ||
| # is an edgecase anyway. |
There was a problem hiding this comment.
I am not convince it would be good enough. Especially not having the defaults on the changed objects may cause real trouble.
There was a problem hiding this comment.
So I loop over all servertype_attributes of the new servertype and if it has a default value and the object does not have that attribute set, I set the default. Would that be enough?
There was a problem hiding this comment.
I guess. See get_default_attribute_values().
This adds support for changing the servertype via the normal commit API. It performs some sanity checking and finally modifies the commit to include attribute removals if an attribute was available on the old servertype and set but is not available on the new servertype. This guarantees no zombie attributes on the database and a clean history.