The models save() and delete() method should probably return Result instead of bool #2414

New Issue

OVERLORD · 2025-10-09T18:04:23+03:00

OVERLORD commented

2025-10-09 18:04:23 +03:00

Originally created by @mprasil on GitHub.

I find that the bool actually masks the underlying issue, returning some sort of Result would probably be much better. Also the Result should actually be checked and API should respond appropriately. As a real world example, it took me quite a while to figure out that program couldn't write to DB due to permissions.

Here's the list of functions that are left to be updated:

attachment.rs:
- Attachment::save()
cipher.rs:
- Cipher::save()
collection.rs:
- Collection::save()
- CollectionUser::save()
- CollectionUser::delete()
- CollectionCipher::save()
- CollectionCipher::delete()
device.rs:
- Device:save()
- Device::delete()
folder.rs:
- Folder::save()
organization.rs:
- Organization::save()
- UserOrganization::save()
user.rs:
- User::save()
- User::delete()

Originally created by @mprasil on GitHub. I find that the bool actually masks the underlying issue, returning some sort of Result would probably be much better. Also the Result should actually be checked and API should respond appropriately. As a real world example, it took me quite a while to figure out that program couldn't write to DB due to permissions. Here's the list of functions that are left to be updated: - attachment.rs: - [x] Attachment::save() - cipher.rs: - [x] Cipher::save() - collection.rs: - [x] Collection::save() - [x] CollectionUser::save() - [x] CollectionUser::delete() - [ ] CollectionCipher::save() - [ ] CollectionCipher::delete() - device.rs: - [ ] Device:save() - [ ] Device::delete() - folder.rs: - [ ] Folder::save() - organization.rs: - [ ] Organization::save() - [ ] UserOrganization::save() - user.rs: - [ ] User::save() - [ ] User::delete()

OVERLORD added the enhancement good first issue labels 2025-10-09 18:04:23 +03:00

OVERLORD closed this issue

2025-10-09 18:04:24 +03:00

OVERLORD commented

2025-10-09 18:04:26 +03:00

@Baelyk commented on GitHub:

@mprasil I created a pr (#161) for the Attachment model.

In the future (and I guess present) what is the etiquette regarding good first issues? Now having completed a first issue (and part of this one), should I leave them in case other people are interested in contributing for the first time? Would it have been okay to do several of the models here?

And then regarding the bug issue previously, if I see something like that, is it okay for me to immediately work on that? Or should I ask permission in the issue/matrix?

Sorry thats a lot of questions but I am interested in contributing more but have never made it beyond the stage of "good first issue" of a project before and I guess I'm just not sure on how it usually works :/

@Baelyk commented on GitHub: @mprasil I created a pr (#161) for the Attachment model. In the future (and I guess present) what is the etiquette regarding good first issues? Now having completed a first issue (and part of this one), should I leave them in case other people are interested in contributing for the first time? Would it have been okay to do several of the models here? And then regarding the bug issue previously, if I see something like that, is it okay for me to immediately work on that? Or should I ask permission in the issue/matrix? Sorry thats a lot of questions but I am interested in contributing more but have never made it beyond the stage of "good first issue" of a project before and I guess I'm just not sure on how it usually works :/

OVERLORD commented

2025-10-09 18:04:26 +03:00

@mprasil commented on GitHub:

It looks like @janost finished the last ones whit his PRs today and this can be closed now. Great work everyone involved.

@mprasil commented on GitHub: It looks like @janost finished the last ones whit his PRs today and this can be closed now. Great work everyone involved.

OVERLORD commented

2025-10-09 18:04:27 +03:00

@mprasil commented on GitHub:

These are generally good first issues if someone feels like taking one function at a time.

@mprasil commented on GitHub: These are generally good first issues if someone feels like taking one function at a time.

OVERLORD commented

2025-10-09 18:04:28 +03:00

@dani-garcia commented on GitHub:

Yeah, I figured that as we are using an integrated SQLite instance that we control, it was a safe-ish asumption that there wouldn't be any problems, but it would be a good idea to handle those kinds of errors in a better way.

@dani-garcia commented on GitHub: Yeah, I figured that as we are using an integrated SQLite instance that we control, it was a safe-ish asumption that there wouldn't be any problems, but it would be a good idea to handle those kinds of errors in a better way.

OVERLORD commented

2025-10-09 18:04:28 +03:00

@mprasil commented on GitHub:

That's great!

Generally speaking grab as many bugs as you want. We aren't really trying to keep bugs around just in case someone appears one day, the idea is to resolve the bugs. 😄 The "good first issue" usually means it's easy to resolve but also not urgent and very often (like in this case) there are other things to attend to and no one else really has time to work on this right now. We've left couple of them lingering around intentionally (especially around the Vault 2 beta branch) but those usually come with some sort of deadline when we need to have them resolved and hence we also don't want them hanging around for too long, so again if you resolve more/all, feel free to. As much as we'd al like to, we aren't going to run out of bugs anytime soon. 😆

In terms of bug ownership, for simple bugs (let's say something you believe you can resolve in hour or so) just work away and then submit a PR. It's very unlikely that someone else is just working on this and even when it happens, there's not much work lost. Feel free to add a comment that you're working on it, if you want to be sure, but make sure you'll have enough time to actually resolve the issue. There's nothing worse than lingering bug that someone promised to work on, but then didn't get around to do that. ⌛️

For bigger piece of code, that might take a while it is good idea to open a PR early on and mark it as [wip]. That way you can also get some early feedback and other people get some idea about the progress, so there's less anxiety around it.

TL/DR: Work on what you feel like working. All bugs are equal and for grabs. Don't leave people waiting for you without some progress updates or ETA if it's bigger change. Enjoy.

Hope that helps.

@mprasil commented on GitHub: That's great! Generally speaking grab as many bugs as you want. We aren't really trying to keep bugs around just in case someone appears one day, the idea is to resolve the bugs. 😄 The "good first issue" usually means it's easy to resolve but also not urgent and very often (like in this case) there are other things to attend to and no one else really has time to work on this right now. We've left couple of them lingering around intentionally (especially around the Vault 2 beta branch) but those usually come with some sort of deadline when we need to have them resolved and hence we also don't want them hanging around for too long, so again if you resolve more/all, feel free to. As much as we'd al like to, we aren't going to run out of bugs anytime soon. 😆 In terms of bug ownership, for simple bugs (let's say something you believe you can resolve in hour or so) just work away and then submit a PR. It's very unlikely that someone else is just working on this and even when it happens, there's not much work lost. Feel free to add a comment that you're working on it, if you want to be sure, but make sure you'll have enough time to actually resolve the issue. There's nothing worse than lingering bug that someone promised to work on, but then didn't get around to do that. ⌛️ For bigger piece of code, that might take a while it is good idea to open a PR early on and mark it as [wip]. That way you can also get some early feedback and other people get some idea about the progress, so there's less anxiety around it. **TL/DR:** Work on what you feel like working. All bugs are equal and for grabs. Don't leave people waiting for you without some progress updates or ETA if it's bigger change. Enjoy. Hope that helps.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/vaultwarden#2414