Mutation improvements #870
Conversation
- Add a sanity check to the sign operation. - Selectively copy fields from previous entry. - Check() now returns bool, error
Codecov Report
@@ Coverage Diff @@
## master #870 +/- ##
==========================================
+ Coverage 45.64% 48.78% +3.14%
==========================================
Files 34 34
Lines 2905 2353 -552
==========================================
- Hits 1326 1148 -178
+ Misses 1379 1001 -378
- Partials 200 204 +4
Continue to review full report at Codecov.
|
core/mutator/entry/mutation.go
Outdated
| // Check verifies that an update was successfully applied. | ||
| // Returns nil if newLeaf is equal to the entry in this mutation. | ||
| func (m *Mutation) Check(newLeaf []byte) error { | ||
| // XXX Mutations are no longer stable serialized byte slices, so we need to use |
There was a problem hiding this comment.
It's more of a warning and a TODO that's related to our discussion of ObjectHash.
Storing objects is really nice because I get to avoid lots of UnMarshals, but then I need to do object comparison in a reliable way...
core/mutator/entry/mutation.go
Outdated
| } | ||
|
|
||
| if got, want := leafValue, m.entry; !proto.Equal(got, want) { | ||
| return errors.New("newLeaf does not match expected value") |
There was a problem hiding this comment.
I've seen advice that there's no special reason to use errors.New just because you have no varargs items for a fmt.Errorf() call. But I think you have pointed me to https://en.wikipedia.org/wiki/Uncontrolled_format_string on this topic, am I recalling correctly?
There was a problem hiding this comment.
That's right. A secure way of using fmt.Errorf in this case would look like:
fmt.Errorf("%v", "A string with no variables")| @@ -148,3 +153,19 @@ func (m *Mutation) sign(signers []signatures.Signer) (*pb.Entry, error) { | |||
| m.entry.Signatures = sigs | |||
| return m.entry, nil | |||
| } | |||
|
|
|||
| // Check verifies that an update was successfully applied. | |||
| // Returns nil if newLeaf is equal to the entry in this mutation. | |||
There was a problem hiding this comment.
It seems to me that the function is answering a yes/no question, but that errors can occur.
With this func signature the caller can't distinguish between 'no' and 'oops'.
A (bool, error) return would make it unambiguous but is ugly.
Better would be an error constant that the caller can check for.
You could just move the errors.New("message") to an constant in this package and return that.
Does Check need to be exported? I see it being used in this same file. If so then the error constant should be too.
There was a problem hiding this comment.
(bool, error) is what I ended up doing.
I thought about defining a public error, but then considered that it would force calling sites into a just as awkward error parsing.
if err := m.Check(); err == ErrNotEqual {
} else if err != nil {
return err
}There was a problem hiding this comment.
OK, I don't mind much either way, good you considered both options.
core/mutator/entry/mutation.go
Outdated
| @@ -59,9 +58,13 @@ func NewMutation(index []byte, domainID, appID, userID string) *Mutation { | |||
| } | |||
| } | |||
|
|
|||
| // CopyPrevious indicates whether to overwrite all values in the current entry with | |||
| // the previous entry's values. | |||
| type CopyPrevious bool | |||
There was a problem hiding this comment.
I don't think there's a readability gain from having a typedef'd bool.
In SetPrevious itself, you write 'if copyPrevious { ... }'
You'd write that whether or not you define the type here.
If you want the extra type safety / documentation then making this more of an enum so that values are named as well would be more helpful.
type CopyPrevious int
const (
copyPrevious = iota
dontCopyPrevious
)
maybe?
There was a problem hiding this comment.
Should I use a plain bool then?
core/mutator/entry/mutation.go
Outdated
| @@ -156,16 +167,13 @@ func (m *Mutation) sign(signers []signatures.Signer) (*pb.Entry, error) { | |||
|
|
|||
| // Check verifies that an update was successfully applied. | |||
| // Returns nil if newLeaf is equal to the entry in this mutation. | |||
| func (m *Mutation) Check(newLeaf []byte) error { | |||
| func (m *Mutation) Check(newLeaf []byte) (bool, error) { | |||
There was a problem hiding this comment.
oops, I didn't see you've already thought about this since the previous commit.
Do consider the error constant alternative though.
| @@ -230,9 +228,9 @@ func (c *Client) ListHistory(ctx context.Context, userID, appID string, start, e | |||
|
|
|||
| // Update creates an UpdateEntryRequest for a user, attempt to submit it multiple | |||
| // times depending on RetryCount. | |||
| func (c *Client) Update(ctx context.Context, userID, appID string, profileData []byte, | |||
| func (c *Client) Update(ctx context.Context, appID, userID string, profileData []byte, | |||
There was a problem hiding this comment.
hope you caught all the call-sites of this change!
There was a problem hiding this comment.
I think I did... the unit tests helped me track down places where I had missed it.
core/client/grpcc/grpc_client.go
Outdated
| } | ||
|
|
||
| // Retry will take a pre-fabricated request and send it again. | ||
| func (c *Client) Retry(ctx context.Context, req *pb.UpdateEntryRequest, opts ...grpc.CallOption) error { | ||
| // Retry will take a mutation and send it again. |
There was a problem hiding this comment.
nit: "Retry takes a mutation and..."
There was a problem hiding this comment.
Did you disagree? I just thought working the comment in future tense seemed odd.
There was a problem hiding this comment.
This comment slipped by. Fixed now.
| return nil, fmt.Errorf("Error unmarshaling Entry from leaf proof: %v", err) | ||
| // NewMutation creates a Mutation given the userID, desired state, and previous entry. | ||
| func (v *Verifier) NewMutation( | ||
| domainID, appID, userID string, |
There was a problem hiding this comment.
Having no stronger typing and a list of 3 different fooIDs makes me a bit nervous... it'd be easy to transpose args by mistake in a call site and get some badly broken behaviour as a result...
There was a problem hiding this comment.
Yeah... Consistency between APIs should help, but I'm not sure what else to do.
There was a problem hiding this comment.
Well, the strong typing option is available:
type DomainID string
type AppID string
type UserID string
Do you think that would cause a lot of churn and annoyance?
There was a problem hiding this comment.
I kind of think it would cause more churn and type casting than really needed.
If a call site gets it wrong all subsequent crypto proofs will fail, so there's an easy way to tell for sure if you got it right.
integration/client_test.go
Outdated
| {true, true, GetNewOutgoingContextWithFakeAuth("bob"), "bob", signers1, authorizedKeys1}, // Update | ||
| {true, true, GetNewOutgoingContextWithFakeAuth("bob"), "bob", signers2, authorizedKeys2}, // Update, changing keys | ||
| {true, true, GetNewOutgoingContextWithFakeAuth("bob"), "bob", signers3, authorizedKeys3}, // Update, using new keys | ||
| {"Empty", false, false, context.Background(), "noalice", signers1, authorizedKeys1}, |
There was a problem hiding this comment.
This is pushing the limit of what's readable and maintainable in a test table full of one-liners (and no labels!)
|
Updated. PTAL |
In the continued quest for easier account account logic...
This PR gives
entry.Mutationthe following new responsibilities:UpdateRequestproto so callers don't have to manually fix it up.Check()As a result, the client can now operate on units of
entry.Mutationrather than raw request protos.This allows us to fix an outstanding TODO that will resolve two racing clients gracefully by recreating requests from scratch on each
Retryrather than blindly resubmitting the same request.Other nits:
userIDandappIDshould do so indomainID,appID,userIDorder.