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

merge [Draft]Reply models #1634

Closed
8 of 9 tasks
cfm opened this issue Mar 7, 2023 · 4 comments
Closed
8 of 9 tasks

merge [Draft]Reply models #1634

cfm opened this issue Mar 7, 2023 · 4 comments
Assignees

Comments

@cfm
Copy link
Member

cfm commented Mar 7, 2023

This ticket tracks the implementation of the refactoring proposed in freedomofpress/securedrop-engineering#8 and formally modeled in #1632, specifically:

  1. Merge the DraftReply SQLAlchemy model into the Reply SQLAlchemy model, including:
  2. Write an Alembic migration for DraftReply into Reply objects.
  3. Update securedrop_client.storage functions.
  4. Update securedrop_client.api_job.{download,upload}_jobs.
  5. Update securedrop_client.logic.Controller methods, signals, and slots
  6. Update securedrop_client.gui.widgets.{SpeechBubble,ReplyWidget} methods, signals, and slots.
  7. [ongoing] (Update tests throughout.)
@cfm cfm self-assigned this Mar 7, 2023
@cfm
Copy link
Member Author

cfm commented Mar 8, 2023

I'll recap for the record my conversation with @gonzalo-bulnes about the integration strategy here.

There are three layers of the Client stack that will be aware of this state machine: broadly speaking, the database (persistence), the controller and processing jobs (logic), and the GUI (presentation). Qt has its own QStateMachine, and I'm looking into a range of FSM libraries at the controller and database layers. My initial instinct is that we should avoid libraries such as sqlalchemy-fsm that couple our FSM implementation to our ORM, since several others I'm evaluating are just as easy to persist without any such coupling (or an intermediary dependency). So probably we're looking at just two layers: models (which are persisted) and the GUI.

I was concerned that we would have to replicate this state machine between the model-level FSM and a widget-level QStateMachine and then keep them in sync. We concluded that that's not necessary: we can simply pass the Reply model object (including its state) to the Reply widget and signal the latter when the former's FSM transitions, potentially via the Qt model API's dataChanged signal and data() method. (TBD.) That way the Reply model is authoritative for its state, both persisted to the database and signaled to the Reply widget.

We agreed that, while there's room to rethink the relation between the Client's domain model (in securedrop_client.state) and its persistence layer, this approach will do for now.

@cfm
Copy link
Member Author

cfm commented Mar 23, 2023

I spent some time earlier this month (a) long-listing Python FSM libraries, (b) short-listing the top three by PyPI downloads and dependents, and (c) proving integration of each of those three. The largest diffstat (again, just for the integration, not including a complete state machine) is 110 lines.

My key finding is that, all else being equal, we should prefer an FSM library that provides a singleton machine for an arbitrary number of models. (Yes, this terminology becomes terribly confusing in the context of an ORM, which we also have.) By analogy to OOP: We want our instances (models) to reuse the functionality (transitions) provided by the class (machine), not copy it into each and every instance. In our case, we want one Reply machine that operates on n Reply models (instances), rather than n Reply instances each of which implements a complete copy of that machine. (background)

Based on this testing, I recommend we use transitions, developed since 2015 and used by 76 other packages, including the sqlalchemy-state-machine I ruled out according to #1634 (comment). Instead, we can use a tiny StateMachineMixin to link any SQLAlchemy model class to an FSM definition, which it will reuse for any number of model instances.

That's how I'll proceed with implementation, and I'll invite review of transitions as part of review of the resulting pull request. Feedback welcome in the meantime.

@cfm
Copy link
Member Author

cfm commented May 3, 2023

As of 2aa88fc, here's an example of how this approach concretely lets us sanity-check model state and application behavior:
>>> from securedrop_client.db import Reply
>>> a = Reply(filename="0-will-succeed")
>>> a.state
'Pending'
>>> a.send_queued()
True
>>> a.state
'SendPending'
>>> a.sending()
True
>>> a.state
'Sending'
>>> a.sent()
True
>>> a.state
'Ready'
>>> a.download_queued()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/user/securedrop-client/.venv/lib/python3.9/site-packages/transitions/core.py", line 402, in trigger
    return self.machine._process(func)
  File "/home/user/securedrop-client/.venv/lib/python3.9/site-packages/transitions/core.py", line 1211, in _process
    return trigger()
  File "/home/user/securedrop-client/.venv/lib/python3.9/site-packages/transitions/core.py", line 415, in _trigger
    if self._is_valid_source(event_data.state):
  File "/home/user/securedrop-client/.venv/lib/python3.9/site-packages/transitions/core.py", line 452, in _is_valid_source
    raise MachineError(msg)
transitions.core.MachineError: "Can't trigger event download_queued from state Ready!"
>>> b = Reply(filename="1-will-fail")
>>> b.state
'Pending'
>>> b.send_queued()
True
>>> b.state
'SendPending'
>>> b.sending()
True
>>> b.state
'Sending'
>>> b.send_failed()
True
>>> b.state
'SendFailed'
>>> b.download_queued()  # recovery
True
>>> b.state
'DownloadPending'
>>> b.downloading()
True
>>> b.state
'Downloading'
>>> b.downloaded()
True
>>> b.state
'Downloaded'
>>> b.decrypted()
True
>>> b.state
'Ready'

@cfm
Copy link
Member Author

cfm commented Oct 2, 2023

Abandoned per #1632 (comment).

@cfm cfm closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants