Skip to content

Commit

Permalink
Simplify WriteBatch implementation in line with internal guidance
Browse files Browse the repository at this point in the history
- Create never specifies an update mask
- Set specifies an update mask if and only if the merge option is true
- Update always specifies an update mask

In all cases there's a single Write (so I've renamed the method).

All tests still pass, and I'm now happy to merge this.
  • Loading branch information
jskeet committed Sep 25, 2020
1 parent 310a1cb commit ef55349
Showing 1 changed file with 10 additions and 28 deletions.
38 changes: 10 additions & 28 deletions apis/Google.Cloud.Firestore/Google.Cloud.Firestore/WriteBatch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,7 @@ public WriteBatch Create(DocumentReference documentReference, object documentDat
var sentinels = FindSentinels(fields);
GaxPreconditions.CheckArgument(!sentinels.Any(sf => sf.IsDelete), nameof(documentData), "Delete sentinels cannot appear in Create calls");
RemoveSentinels(fields, sentinels);
// Force a write if we've not got any sentinel values. Otherwise, we end up with an empty transform instead,
// just to specify the precondition.
AddUpdateWrites(documentReference, fields, s_emptyFieldPathList, Precondition.MustNotExist, sentinels, forceWrite: sentinels.Count == 0, includeEmptyUpdateMask: false);
AddUpdateWrite(documentReference, fields, updatePaths: null, Precondition.MustNotExist, sentinels);
return this;
}

Expand Down Expand Up @@ -137,7 +135,7 @@ public WriteBatch Update(DocumentReference documentReference, IDictionary<FieldP
RemoveSentinels(expanded, sentinels);

var nonDeletes = sentinels.Where(sf => !sf.IsDelete).ToList();
AddUpdateWrites(documentReference, expanded, updates.Keys.ToList(), precondition ?? Precondition.MustExist, nonDeletes, forceWrite: false, includeEmptyUpdateMask: false);
AddUpdateWrite(documentReference, expanded, updates.Keys.ToList(), precondition ?? Precondition.MustExist, sentinelFields: nonDeletes);
return this;
}

Expand All @@ -159,7 +157,6 @@ public WriteBatch Set(DocumentReference documentReference, object documentData,
var deletes = sentinels.Where(sf => sf.IsDelete).ToList();
var nonDeletes = sentinels.Where(sf => !sf.IsDelete).ToList();

bool forceWrite = false;
IDictionary<FieldPath, Value> updates;
IReadOnlyList<FieldPath> updatePaths;
if (options.Merge)
Expand All @@ -172,7 +169,6 @@ public WriteBatch Set(DocumentReference documentReference, object documentData,
// - Deletes are allowed anywhere
// - All timestamps converted to transforms
// - Each top-level entry becomes a FieldPath
forceWrite = fields.Count == 0;
RemoveSentinels(fields, nonDeletes);
// Work out the update paths after removing server timestamps but before removing deletes,
// so that we correctly perform the deletes.
Expand Down Expand Up @@ -210,11 +206,10 @@ public WriteBatch Set(DocumentReference documentReference, object documentData,
GaxPreconditions.CheckArgument(deletes.Count == 0, nameof(documentData), "Delete cannot appear in document data when overwriting");
RemoveSentinels(fields, nonDeletes);
updates = fields.ToDictionary(pair => new FieldPath(pair.Key), pair => pair.Value);
updatePaths = s_emptyFieldPathList;
forceWrite = true;
updatePaths = null;
}

AddUpdateWrites(documentReference, ExpandObject(updates), updatePaths, null, nonDeletes, forceWrite, includeEmptyUpdateMask: options.Merge);
AddUpdateWrite(documentReference, ExpandObject(updates), updatePaths, precondition: null, sentinelFields: nonDeletes);
return this;
}

Expand All @@ -234,28 +229,13 @@ internal async Task<IList<WriteResult>> CommitAsync(ByteString transactionId, Ca
.ToList();
}

private void AddUpdateWrites(
private void AddUpdateWrite(
DocumentReference documentReference,
IDictionary<string, Value> fields,
IReadOnlyList<FieldPath> updatePaths,
IReadOnlyList<FieldPath> updatePaths, // Null if an update mask isn't required
Precondition precondition,
IList<SentinelField> sentinelFields,
bool forceWrite,
bool includeEmptyUpdateMask)
IList<SentinelField> sentinelFields)
{
updatePaths = updatePaths.Except(sentinelFields.Select(sf => sf.FieldPath)).ToList();
if (!forceWrite && fields.Count == 0 && updatePaths.Count == 0 && sentinelFields.Count == 0)
{
return;
}

// If we've only got sentinels, but we have a precondition that requires the document to exist,
// include the empty update mask, to avoid removing all other data.
if (!includeEmptyUpdateMask && fields.Count == 0 && sentinelFields.Count > 0 && precondition?.Exists == true)
{
includeEmptyUpdateMask = true;
}

var write = new Write
{
CurrentDocument = precondition?.Proto,
Expand All @@ -264,7 +244,9 @@ internal async Task<IList<WriteResult>> CommitAsync(ByteString transactionId, Ca
Fields = { fields },
Name = documentReference.Path,
},
UpdateMask = includeEmptyUpdateMask || updatePaths.Count > 0 ? new DocumentMask { FieldPaths = { updatePaths.Select(fp => fp.EncodedPath) } } : null,
UpdateMask = updatePaths is null
? null
: new DocumentMask { FieldPaths = { updatePaths.Except(sentinelFields.Select(sf => sf.FieldPath)).Select(fp => fp.EncodedPath) } },
UpdateTransforms = { sentinelFields.Select(p => p.ToFieldTransform()) }
};
Writes.Add(write);
Expand Down

0 comments on commit ef55349

Please sign in to comment.