Update Firestore and held-back dependencies

[xacrimon]

This PR takes care of updating the Firestore SDK and related dependencies that were held back by our usage of an old version. This PR remediates the usage of the old batch API which is the reason we held back this dependency.

[xacrimon]

@zmb3 Did you want backports to v12 on this or wait? Memory fails me.

[tigrato]

@xacrimon, don't forget to remove the updated dependencies from the ignored list in the .github/dependabot.yml file

[xacrimon]

@tigrato thanks! I have now run the manual FS tests using the dev API key, missed these as I closed out early yesterday.

[tigrato]

This section doesn't seem to be used except for jobs := make([]*firestore.BulkWriterJob, 0, len(mDocs)).

Probably you wanted to use the mDocs in the for loop instead of docs?

[xacrimon]

Huh, do we have a review race condition here perhaps? I did update the loop in that commit to use mDocs. In e7b5f0d

[tigrato]

I don't think we need to consider any race condition here.
Anw, a final nit to avoid ranging a map

// deleteDocuments removes documents from firestore in batches to stay within the
// firestore write limits
func (b *Backend) deleteDocuments(docs []*firestore.DocumentSnapshot) error {
	set := make(map[string]struct{})

	batch := b.svc.BulkWriter(b.clientContext)
	jobs := make([]*firestore.BulkWriterJob, 0, len(docs))

	i := 0
	for _, doc := range docs {
		// Deduplicate documents. The Firestore SDK will error if duplicates are found,
		// but existing callers of this functionassume this is valid.
		if _, ok := set[doc.Ref.Path]; ok {
			continue
		}
		set[doc.Ref.Path] = struct{}{}

		job, err := batch.Delete(doc.Ref)
		if err != nil {
			return ConvertGRPCError(err)
		}

		jobs = append(jobs, job)
		i++
		if i >= commitLimit {
			batch.Flush()
		}
	}
       batch.End()
	var errs []error
	for _, job := range jobs {
		if _, err := job.Results(); err != nil {
			errs = append(errs, ConvertGRPCError(err))
		}
	}

	return trace.NewAggregate(errs...)
}

[xacrimon]

Updated, but is there anything bad about ranging a map?

[tigrato]

I don't think we need to consider any race condition here.
Anw, a final nit to avoid ranging a map

// deleteDocuments removes documents from firestore in batches to stay within the
// firestore write limits
func (b *Backend) deleteDocuments(docs []*firestore.DocumentSnapshot) error {
	set := make(map[string]struct{})

	batch := b.svc.BulkWriter(b.clientContext)
	jobs := make([]*firestore.BulkWriterJob, 0, len(docs))

	i := 0
	for _, doc := range docs {
		// Deduplicate documents. The Firestore SDK will error if duplicates are found,
		// but existing callers of this functionassume this is valid.
		if _, ok := set[doc.Ref.Path]; ok {
			continue
		}
		set[doc.Ref.Path] = struct{}{}

		job, err := batch.Delete(doc.Ref)
		if err != nil {
			return ConvertGRPCError(err)
		}

		jobs = append(jobs, job)
		i++
		if i >= commitLimit {
			batch.Flush()
		}
	}
       batch.End()
	var errs []error
	for _, job := range jobs {
		if _, err := job.Results(); err != nil {
			errs = append(errs, ConvertGRPCError(err))
		}
	}

	return trace.NewAggregate(errs...)
}

[xacrimon]

@tigrato I meant a human/GitHub race condition :)
The comment was about a change that was addressed before the comment was made.

[xacrimon]

@tigrato I was made aware by a friend that the commit limit is no more using this API (docs seemed to indicate it was, but they are outdated). So I removed those in addition to fixing a test. Can you re-review please?

[tigrato]

LGTM

[xacrimon]

@rosstimothy Since this is a major API structure switch for how bulk deletes are done, this may need an extra eye during v13 testing.

[codingllama]

I'm a bit late to the party, but adding comments anyway.

Thanks Joel, happy to see this update get merged.

[codingllama]

Please update api/go.mod too.

[codingllama]

Suggestion: If you don't need the variable:

Suggested change

	for i := 0; i < len(req.Writes); i++ {
	for _ := range req.Writes {

[espadolini]

for range req.Writes works too.

[codingllama]

Even better ☝️

[xacrimon]

hold up, when did we get this syntax?

[codingllama]

Since forever, I think.

[codingllama]

nit: Use for-range above?

[xacrimon]

The variable i is actually used in the loop at L252. See L255, always been that way.

[codingllama]

Comment is still viable, although certainly not worth a PR.

[codingllama]

What is the consequence of not setting this? Could we get around the reflect block in some other way?

[xacrimon]

This was already merged due to @rosstimothy adding it to the queue. I was confused by errors for some time in tests for quite some time that didn't crop up in real world testing. As it turns out, the firestore SDK BulkWriter API we now use assembles batches of changes internally and submits them. Each batch is deduplicated so you can't get two deletes or an edit after a delete in the same batch, so they always succeed. The consequence of this is that it's illegal to delete a document twice with the same BulkWriter.

This would be fine, the test generates an unique ref.Path right? Well, as it turns out, the SDK does deduplication on an internal hidden variable which represents the path in a smaller encoding. Why? I have no idea. But this is stored in a hidden field that we cannot set easily. If you remove this, you get deduplication errors in the tests, even if the ref.Path is unique.

[xacrimon]

I could not find an alternative way around this easily.

[codingllama]

Fair enough, thanks for the explanation.