Refactor process_own_message to not mutate intent state directly #985
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This stack of pull requests is managed by Graphite. Learn more about stacking. |
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
51dbd6e to
38f29c6
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
4ffa958 to
9bc52b9
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
38f29c6 to
d8dd680
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
9bc52b9 to
18f59d8
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
d8dd680 to
492e673
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
18f59d8 to
03677ae
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
492e673 to
58bbcd2
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
03677ae to
c2a3bd8
Compare
Closed
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
58bbcd2 to
5902193
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
c2a3bd8 to
a01133c
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
5902193 to
0e1c68f
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
a01133c to
2cec1d2
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
0e1c68f to
2c6cb9e
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
2cec1d2 to
c855664
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
2c6cb9e to
1e535db
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
c855664 to
89c3113
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
1e535db to
5d277d6
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
89c3113 to
cb49a01
Compare
neekolas
force-pushed
the
08-22-move_staged_commit_to_intents
branch
from
5d277d6 to
c9f0f10
Compare
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
cb49a01 to
3492510
Compare
cameronvoell
approved these changes
Aug 23, 2024
neekolas
changed the base branch from
08-22-move_staged_commit_to_intents
to
graphite-base/985
neekolas
force-pushed
the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
from
3492510 to
5afdbc3
Compare
| @@ -345,7 +343,7 @@ impl MlsGroup { | |||
| ); | |||
| if let Err(err) = openmls_group.merge_staged_commit(&provider, pending_commit) { | |||
| log::error!("error merging commit: {}", err); | |||
| conn.set_group_intent_to_publish(intent.id)?; | |||
There was a problem hiding this comment.
Interesting, I wonder if this was a bug before? In the previous code it would have continued to L371 and set it to committed
neekolas
deleted the
08-22-refactor_process_own_message_to_not_mutate_intent_state_directly
branch
| Ok(provider.conn_ref().set_group_intent_committed(intent_id)?) | ||
| } | ||
| IntentState::Published => { | ||
| log::warn!("Unexpected behaviour: returned intent state published from process_own_message"); |
There was a problem hiding this comment.
For unexpected behavior, I think we should log errors, so that we are more likely to see them. Especially if we choose not to update the state here
There was a problem hiding this comment.
The problem is that we aren't returning the error in a lot of cases. We only return the error if we want to hard fail without changing the intent status (like a DB connection error). That'll get returned to the calling function, which does log it.
So, we have to log inside process_own_message before returning OK
There was a problem hiding this comment.
What I meant is, as a general rule, we should do log::error instead of log::warn for cases we expect never to happen - not that we need to return an error to the caller
There was a problem hiding this comment.
Ahh, I see. I can update that
richardhuaaa
approved these changes
Aug 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tl;dr
process_own_messageto return the desired intent state, instead of having a bunch of DB operations happen in the method