Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(firestore): Replace usage of Write.transform with Write.update_transforms #8161

Merged
merged 9 commits into from
Dec 18, 2020
Merged

fix(firestore): Replace usage of Write.transform with Write.update_transforms #8161

merged 9 commits into from
Dec 18, 2020

Conversation

quartzmo
Copy link
Member

closes: #8160

@quartzmo quartzmo self-assigned this Nov 20, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 20, 2020
@product-auto-label product-auto-label bot added the api: firestore Issues related to the Firestore API. label Nov 20, 2020
@quartzmo quartzmo force-pushed the firestore-conformance-update branch from 41a59c0 to 83c8e14 Compare November 24, 2020 17:12
@quartzmo
Copy link
Member Author

quartzmo commented Nov 24, 2020 •
edited
Loading

An error in my conversion was caught by the server timestamp acceptance test, with output shown below:

  • "Before": master branch using transform, test passes.
  • "After": In this PR there is now an error. Using update_transforms, the test fails when existing nested data (the keep attribute in the test) is lost. The error in the output is in this line: update_mask: <Google::Cloud::Firestore::V1::DocumentMask: field_paths: ["b"]>. The implementation in this PR must be changed to pass an empty DocumentMask instance.
# Before
          [<Google::Cloud::Firestore::V1::Write: 
            update: nil, 
            delete: "", 
            update_mask: nil, 
            current_document: <Google::Cloud::Firestore::V1::Precondition: exists: true, update_time: nil>, 
            transform: <Google::Cloud::Firestore::V1::DocumentTransform: document: "projects/buoyant-ability-182823/databases/(default)/documents/gcloud-2020-11-24t22-26-50z-250d92c3/IN5L3FvfrwRqt96B45yu", 
              field_transforms: [
                <Google::Cloud::Firestore::V1::DocumentTransform::FieldTransform: field_path: "a", set_to_server_value: :REQUEST_TIME, increment: nil, maximum: nil, minimum: nil, append_missing_elements: nil, remove_all_from_array: nil>, 
                <Google::Cloud::Firestore::V1::DocumentTransform::FieldTransform: field_path: "e.f", set_to_server_value: :REQUEST_TIME, increment: nil, maximum: nil, minimum: nil, append_missing_elements: nil, remove_all_from_array: nil>, 
                <Google::Cloud::Firestore::V1::DocumentTransform::FieldTransform: field_path: "b.c", set_to_server_value: :REQUEST_TIME, increment: nil, maximum: nil, minimum: nil, append_missing_elements: nil, remove_all_from_array: nil>
              ]>, 
            update_transforms: []>]

# After
           <Google::Cloud::Firestore::V1::Write: 
             update: <Google::Cloud::Firestore::V1::Document: name: "projects/buoyant-ability-182823/databases/(default)/documents/gcloud-2020-11-24t22-30-49z-6c2e5bd5/AWLCi8lzEJ0aoeq2TnUC", fields: {}, create_time: nil, update_time: nil>, 
             delete: "", 
             update_mask: <Google::Cloud::Firestore::V1::DocumentMask: field_paths: ["b"]>, 
             current_document: <Google::Cloud::Firestore::V1::Precondition: exists: true, update_time: nil>, 
             transform: nil,
             update_transforms: [
               <Google::Cloud::Firestore::V1::DocumentTransform::FieldTransform: field_path: "a", set_to_server_value: :REQUEST_TIME, increment: nil, maximum: nil, minimum: nil, append_missing_elements: nil, remove_all_from_array: nil>, 
               <Google::Cloud::Firestore::V1::DocumentTransform::FieldTransform: field_path: "e.f", set_to_server_value: :REQUEST_TIME, increment: nil, maximum: nil, minimum: nil, append_missing_elements: nil, remove_all_from_array: nil>, 
               <Google::Cloud::Firestore::V1::DocumentTransform::FieldTransform: field_path: "b.c", set_to_server_value: :REQUEST_TIME, increment: nil, maximum: nil, minimum: nil, append_missing_elements: nil, remove_all_from_array: nil>
             ]>

@quartzmo quartzmo marked this pull request as ready for review November 25, 2020 20:44
@quartzmo quartzmo requested a review from a team as a code owner November 25, 2020 20:44
dazuma
dazuma approved these changes Nov 29, 2020
View reviewed changes
Copy link
Member

@dazuma dazuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a style suggestion, but otherwise this looks good.

google-cloud-firestore/lib/google/cloud/firestore/convert.rb Outdated Show resolved Hide resolved
@quartzmo quartzmo force-pushed the firestore-conformance-update branch from 4575288 to ed593ea Compare December 16, 2020 17:33
quartzmo
quartzmo commented Dec 16, 2020
View reviewed changes
Copy link
Member Author

@quartzmo quartzmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmidt-sebastian This PR is ready for your review. Unfortunately there is a lot of noise due to the large number of conformance test file updates, as well as many new style fixes required by the updated linter Rubocop. To make reviewing easier for you, I have called out the four sections of convert.rb that contain significant changes.

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
return nil if paths.empty?
paths.map do |field_path, field_value|
to_field_transform field_path, field_value
end.to_a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmidt-sebastian Please review lines 163-179 to confirm that the Write looks correct for a create operation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
)
Google::Cloud::Firestore::V1::Write.new(
update: doc,
update_transforms: field_transforms(field_paths_and_values)
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmidt-sebastian Please review lines 207-214 to confirm that the Write looks correct for a set operation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
update: doc,
update_mask: doc_mask,
update_transforms: field_transforms(field_paths_and_values)
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmidt-sebastian Please review lines 272-283 to confirm that the Write looks correct for a set merge operation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
update_time: time_to_timestamp(update_time))
write.current_document = Google::Cloud::Firestore::V1::Precondition.new(
update_time: time_to_timestamp(update_time)
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schmidt-sebastian Please review lines 328-344 to confirm that the Write looks correct for an update operation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure this is correct. It looks like it sends an empty update (with an existing but empty update mask) for a transfom-only update, which is what we want (otherwise it would delete the existing data).

schmidt-sebastian
schmidt-sebastian approved these changes Dec 17, 2020
View reviewed changes
google-cloud-firestore/lib/google/cloud/firestore/convert.rb
return nil if paths.empty?
paths.map do |field_path, field_value|
to_field_transform field_path, field_value
end.to_a
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
)
Google::Cloud::Firestore::V1::Write.new(
update: doc,
update_transforms: field_transforms(field_paths_and_values)
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
update: doc,
update_mask: doc_mask,
update_transforms: field_transforms(field_paths_and_values)
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

google-cloud-firestore/lib/google/cloud/firestore/convert.rb
update_time: time_to_timestamp(update_time))
write.current_document = Google::Cloud::Firestore::V1::Precondition.new(
update_time: time_to_timestamp(update_time)
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure this is correct. It looks like it sends an empty update (with an existing but empty update mask) for a transfom-only update, which is what we want (otherwise it would delete the existing data).

@quartzmo quartzmo merged commit 070dbb8 into googleapis:master Dec 18, 2020
@quartzmo quartzmo deleted the firestore-conformance-update branch December 18, 2020 00:08
@quartzmo quartzmo mentioned this pull request Jan 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dazuma dazuma dazuma approved these changes

@schmidt-sebastian schmidt-sebastian schmidt-sebastian approved these changes

Assignees

@quartzmo quartzmo

Labels
api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

firestore: Replace usage of Write.transform with Write.update_transforms
3 participants
@quartzmo @dazuma @schmidt-sebastian