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

firestore: not returning exact error #10061

Closed
Jason-SP-Chien opened this issue Apr 29, 2024 · 2 comments · Fixed by #10826
Closed

firestore: not returning exact error #10061

Jason-SP-Chien opened this issue Apr 29, 2024 · 2 comments · Fixed by #10826
Assignees
Labels
api: firestore Issues related to the Firestore API. triage me I really want to be triaged.

Comments

@Jason-SP-Chien
Copy link

Client

Firestore

Code

In firestore/bulkwriter.go, we saw methods of BulkWriter including Create, Delete, Set, and Update. They do not throw the error returned by newXXXWrites. The error itself is replaced by fmt.Errorf("firestore: cannot XXX doc %v", doc.ID), which cost us time to trace out what really happened inside these operations.

Expected behavior

The error itself should be either wrapped or directly returned.

Actual behavior

It is replaced by an almost useless error message.

@Jason-SP-Chien Jason-SP-Chien added the triage me I really want to be triaged. label Apr 29, 2024
@product-auto-label product-auto-label bot added the api: firestore Issues related to the Firestore API. label Apr 29, 2024
@kevpie
Copy link

kevpie commented Jun 12, 2024

@Jason-SP-Chien I was hitting this problem yesterday.

A possible work around is to call the docref.Update() to receive the full details.

	job, err := bulkWriter.Update(ref, update)
	if err != nil {
		// bulkWriter hides the error returned by newUpdatePathWrites,  call Update again to get the error
		_, err = ref.Update(ctx, update)
	}

BulkWriter error

cannot update doc <doc id>

Sample of the improved error for nested map. Passing the same path to update twice to the path { options.style }.

error: field path [options style] cannot be used in the same update as [options style]

Could perform error wrapping to include the Doc ID as well as the detail.

@kevpie
Copy link

kevpie commented Jun 12, 2024

BulkWriter.Update versus DocumentRef.Update

BulkWriter discards the Error and returns fmt.Errorf("firestore: cannot update doc %v", doc.ID)
Could simply wrap it.

// Update adds a document update write to the queue of writes to send.
// Note: You cannot write to (Create, Update, Set, or Delete) the same document more than once.
func (bw *BulkWriter) Update(doc *DocumentRef, updates []Update, preconds ...Precondition) (*BulkWriterJob, error) {
	bw.isOpenLock.RLock()
	defer bw.isOpenLock.RUnlock()
	err := bw.checkWriteConditions(doc)
	if err != nil {
		return nil, err
	}

	w, err := doc.newUpdatePathWrites(updates, preconds)
	if err != nil {
		return nil, fmt.Errorf("firestore: cannot update doc %v", doc.ID)
	}

	if len(w) > 1 {
		return nil, fmt.Errorf("firestore: too many writes sent to bulkwriter")
	}

	j := bw.write(w[0])
	return j, nil
}

DocumentRef.Update returns the provided useful Error detail

// Update updates the document. The values at the given
// field paths are replaced, but other fields of the stored document are untouched.
func (d *DocumentRef) Update(ctx context.Context, updates []Update, preconds ...Precondition) (_ *WriteResult, err error) {
	ctx = trace.StartSpan(ctx, "cloud.google.com/go/firestore.DocumentRef.Update")
	defer func() { trace.EndSpan(ctx, err) }()

	ws, err := d.newUpdatePathWrites(updates, preconds)
	if err != nil {
		return nil, err
	}
	return d.Parent.c.commit1(ctx, ws)
}

Possibly improve the existing error by wrapping

	if err != nil {
		return nil, fmt.Errorf("firestore: cannot update doc %v error: %w", doc.ID, err)
	}

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 Firestore API. triage me I really want to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants