Fix another sync issue with native clients

[BlackDex]

The reprompt value somehow sometimes has a value of 4. This isn't a valid value, and doesn't cause issues with other clients, but the native clients are more strict.

This commit fixes this by validating the value before storing and returning.

Fixes #5237

[BlackDex]

In theory we should change the column type from Nullable to not Nullable and have all NULL values converted to 0.
But that might cause other issues. Also, since there are more sync issues which we might need to fix on the database side, i think it's better to create some kind of convert/migrate feature to fix all these items in one go in the future once we have identified all possible issues.

[jjlin]

In theory we should change the column type from Nullable to not Nullable and have all NULL values converted to 0. But that might cause other issues. Also, since there are more sync issues which we might need to fix on the database side, i think it's better to create some kind of convert/migrate feature to fix all these items in one go in the future once we have identified all possible issues.

Why would it help to make it not nullable? If the DB value is null, it already gets converted to RepromptType::None AFAICT.

It seems to me it would be cleaner to add num_derive::FromPrimitive on RepromptType and do a general validity check on the reprompt value, logging any invalid values that are detected. Although I wouldn't be terribly surprised if people are getting invalid values due to disk corruption or something (maybe running off an SD card?) and the issue only manifests now because the mobile clients are stricter.

[BlackDex]

I do not understand why to keep something nullable when it shouldn't.

[jjlin]

I do not understand why to keep something nullable when it shouldn't.

Bitwarden most likely had it nullable in their schema originally. Also null can be useful for indicating "use the default value", which might change over time.

[BlackDex]

Since Bitwarden doesn't have it nullable anymore kinda indicates it has a default of 0/None already. Also, as that is the sane default i do not expect it to be changed. Else we could make all lot more nullable of course. While that might also be beneficial for the database maybe regarding storage, it doesn't make it easier to query it. It will then always need two compares.

Anyways, it doesn't really matter for now. More for the fixing the database defaults, and fix formats instead of doing that during sync all the time.

Logging invalid values btw doesn't really help here. It might have been broken clients or even non official clients which provided wrong values. It just needs to be fixed.

[BlackDex]

Took some look again. But adding num_derive::FromPrimitive for this seems a bit of overkill. Also casting from isize(i8) to i64 again to i32 seems a bit of overkill too me, while it probably something which will not have that much of an impact. I think the code as is looks fine too me.

[BlackDex]

I also did some more detailed checking by validating https://github.com/bitwarden/android/blob/v2024.11.7/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/network/model/SyncResponseJson.kt with our code, and i think it is fine.

The only thing i can see which is not the same is the attachment/send size, which we send as a String instead of an Int. But the none-native clients expected a String, and i think it will be converted correctly by the native clients.