-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Omitting WriteResults for DocumentTransforms #2565
Omitting WriteResults for DocumentTransforms #2565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure I understand it right:
If the user makes N writes, we could previously return more than N responses because some writes are split. Now we always return N responses assuming everything succeeds?
@@ -484,7 +484,7 @@ public String updateCallback(Transaction transaction) { | |||
@Test | |||
public void deleteDocument() throws Exception { | |||
doReturn(beginResponse()) | |||
.doReturn(commitResponse(0, 0)) | |||
.doReturn(commitResponse(2, 0)) |
The server always returns n write responses for n write requests. The problem is that there is no 1 to 1 mapping between user requests and write requests (some user requests might turn into two writes). This PR removes the extra writes, so that the user can correlate its requests to the responses. |
I see. That makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @garrettjonesgoogle do you want to look too?
Garrett, please let me know offline if there are any concerns. |
We have to split up a write that contains a ServerTimestamp into two distinct writes. Since the user is unaware of this, we should only return one WriteResult in our response.
This is only relevant for WriteBatches and a port of googleapis/nodejs-firestore#45
I also ran google-java-format on the codebase.