Add validation for consensus commit mutation operation #873
Conversation
Torch3333
force-pushed
the
validate_consensus_commit_mutation_op
branch
2 times, most recently
from
1f16837
to
e97521c
Compare
Torch3333
force-pushed
the
validate_consensus_commit_mutation_op
branch
from
e97521c
to
0212c8b
Compare
Torch3333
requested review from
a team,
komamitsu,
brfrn169 and
feeblefakie
and removed request for
a team
.../java/com/scalar/db/transaction/consensuscommit/ConsensusCommitMutationOperationChecker.java
Show resolved
Hide resolved
| checkArgument(puts.size() != 0); | ||
| puts.forEach(this::put); | ||
| for (Put p : puts) { |
There was a problem hiding this comment.
[just out of curiosity] Is this change for suppressing warning or something?
There was a problem hiding this comment.
this::put throws the checked exception CrudException but the Consumer functional interface does not throw any checked exception.
public void forEach(java.util.function.Consumer<? super T> action)
Our only option is to use a non-lambda for each if we want to propagate the CrudException.
| if (!(condition instanceof PutIf | ||
| || condition instanceof PutIfNotExists | ||
| || condition instanceof PutIfExists)) { | ||
| throw new IllegalArgumentException("The condition is not allowed on Put operation"); |
There was a problem hiding this comment.
I guess a user would be interested in the type of the wrong condition when receiving this exception. How about showing it in the error message?
There was a problem hiding this comment.
Yes, that is a good point, thank you. I will add it.
There was a problem hiding this comment.
Revised in 4e05db8
| ConsensusCommitMutationOperationChecker operationChecker = | ||
| new ConsensusCommitMutationOperationChecker(tableMetadataManager); |
There was a problem hiding this comment.
Instead of creating a ConsensusCommitMutationOperationChecker instance each time, can we hold it as an instance field of ConsensusCommitManager and pass it to ConsensusCommit instances?
There was a problem hiding this comment.
That is a good point, thank you.
My bad, I did it for the 2pc transaction manager but not for this one.
Revised in de06ddc
| put.getCondition() | ||
| .ifPresent( | ||
| condition -> { | ||
| if (!(condition instanceof PutIf |
There was a problem hiding this comment.
Just quick confirmation.
This check will be deleted in another PR, which makes conditions usable, right?
There was a problem hiding this comment.
Maybe there is a misunderstanding but this step validates DeleteIf and DeleteIfExists conditions are not used for Put operation so I plan to keep this code even when condition are usable. Is that ok ?
There was a problem hiding this comment.
Ah, sorry, yes, that's my misunderstanding. Thanks!
Context
There is no validation to ensure Put or Delete operations do not mutate consensus commit metadata columns. Since these columns are only for internal use, users should not be allowed to modify them.
Changes
A validation step is added to make sure Put or Delete operations do not mutate consensus commit metadata columns. Additionally, setting operation conditions (see #837 ) targeting consensus commit metadata columns are also forbidden.