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.

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

fix: handle empty last chunk correctly in 'Query._chunkify' #489

Merged
merged 8 commits into from
Nov 15, 2021

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 3, 2021

Closes #487.
Supersedes #488.

@tseaver tseaver requested a review from a team as a code owner November 3, 2021 18:57
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Nov 3, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 3, 2021
@noxxious
Copy link

noxxious commented Nov 4, 2021

In the #488 I also had a fix for the None case of the bulk_writter being not correctly initialized.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 4, 2021

@noxxious Your change (calling self.bulk_writer rather than instantiating BulkWriter() directly) didn't change the behavior: the client's bulk_writer method takes an optional options argument, but it isn't being passed.

@noxxious
Copy link

noxxious commented Nov 4, 2021

Sure, passing options would have changed the client API interface. With the current API of bulk_writer being optional and current coded behavior of constructing bulk_writer, using a default constructor results in non-functional BulkWriter not bound to a client, hence all the recursive delete actions are not performed.

My additional tests for the None BulkWriter just show that.

So this means, either the method API needs to change to indicate that the bulk_writer is not optional or the options need to be added.

The object is unusable without a client defined.
…lete'

Use 'self.bulk_writer()' to construct the default writer, and do so in
the public method:  'bulk_writer' is now a required argument for the
private helper.
@tseaver
Copy link
Contributor Author

tseaver commented Nov 4, 2021

@noxxious Thanks for the follow-up!

It is indeed a problem that we have code constructing BulkWriter() without passing a client: that argument should not be optional (now fixed in db9599b).

a2e5469 refactors the bits which construct the bulk writer if none is passed to the publicrecursive_delete methods: the private _recursive_delete helpers now have it as a required argument, and the public methods construct it using self.bulk_writer(), as in your patch.

@noxxious
Copy link

noxxious commented Nov 4, 2021

Glad to help.

@tseaver tseaver requested review from kolea2 and removed request for crwilcox November 9, 2021 17:17
Copy link
Contributor

@chrisrossi chrisrossi left a comment

Choose a reason for hiding this comment

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

LGTM!

@tseaver tseaver added the automerge Merge the pull request once unit tests and other checks pass. label Nov 15, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit 3ddc718 into main Nov 15, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the 487-query-_chunkify-empty-last-chunk branch November 15, 2021 15:14
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/python-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recursive delete fails with no documents
3 participants