More robust data handling in `svc.GetAllCollections()`?

[ryanerwin]

First off, thanks so much for writing this library! I can see that a lot of work went into it and the code looks very well organized. Thanks for this fantastic contribution to the community!

I saw there were some other go keychain interfaces, but I was not able to get good results out of my tests of each one. Yours is a very good, I think nearly complete, implementation! Nice work!

Describe the bug

	conn, _ := dbus.SessionBus()
	svc, _ := keyring.GetSecretService(conn)
	session, _ := svc.OpenSession()
	collection, _ := svc.GetAllCollections()

On one system that I tested, GetAllCollections() requires that every item added to the collections Array is tested with i.GetLabel();.

Just by commenting out the GetLabel block in item.go I was able to get an otherwise usable list back from GetAllCollections().

go-dbus-keyring/item.go

Lines 73 to 75 in a068717

	if _, err := i.GetLabel(); err != nil {
	return nil, err
	}

I suspect that it's a data corruption issue, and that's why I used the question more robust data handling?

I think that more robust data handling in this function makes sense, because GetAllCollections is returning you the entire collection list. Even if there is a problem with a single element in the list, isn't it better to push that error handling later in the process?

To Reproduce
I'm not sure how to help you reproduce this without transferring the entire ~/.local/keychain folder over to you...

Expected behaviour
I would prefer that an error on an array element in the GetAllCollections() would not zero out the entire collections array, even if some err value is returned.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• keyring-server: gnome-keyring
• desktop-environment: gnome
• Ubuntu 20.04

[ppacher]

Thanks for this details issue 👍

From a quick scan through the project files I found at least GetAllCollections, GetAllItems and SearchItems to have the same issue.

I guess instead of returning nil, err we should continue the iteration and still collect all items/collections that don't produce an error (otherwise you might end up with an empty slice just because the first item caused an error).

How would you suggest to handle the case where we encounter multiple errors? Should we only return the first one or wrap it into something like a multi-error (example here)?

For transparency I guess we should do a multi-err but I don't want to include a new dependency just for this so we need to come up with our own tiny solution. 🤔

What do you think about it?

[ryanerwin]

I think you're right about trying to avoid the dependency. That's one of the reasons I really enjoy go. Most of my favourite packages have minimal external imports.

I think this redit thread, especially after missionMeteora/toolkit is mentioned has some interesting related comments:
https://www.reddit.com/r/golang/comments/58kt6u/returning_multiple_errors_as_a_slice/

Some form of wrapping / combining the errors does seem ideal, because multiple errors may be triggered, and they may be different errors.

That said, when we think about the workflow of most go-dbus-keyring, assuming some list elements are "not completely valid", the software will either 1) pass through ignoring the error 2) hide the problem elements 3) show the problem elements along with some kind of notification.

I think for me, the ideal end user interface would be 3) show the problem elements along with some kind of notification... So when a call fails to validate an element, if there is any other background that we can find about the element that would be helpful.

Thanks for responding so quickly and for putting so much work into this great library 💪💪💪