Accept any string as a key for m.direct account data

[MatMaul]

This is the follow up of this Ruma PR for the SDK.

Ruma PR needs to be merged first.

[bnjbvr]

I don't think losing static typing is an option, instead we should introduce an enum that can be one of all the types we expect (owned user id OR email address, if I understand correctly).

Also, BaseRoomInfo is serialized in the database, so we'd need a test making sure that we don't need a migration there, when the previous content was serialized as a OwneduserId; if the test fails, we need a migration.

[MatMaul]

I don't think losing static typing is an option, instead we should introduce an enum that can be one of all the types we expect (owned user id OR email address, if I understand correctly).

Fine for me, but it also needs to support phone numbers I think, basically any identifier types.

Fine for you ?

[MatMaul]

Beware that it will not solve the trouble for some legacy buggy behavior of Element Web.

[bnjbvr]

Fine for you ?

Fine if we do have some type checking / upfront validation for all these other id types indeed :)

[MatMaul]

Is there a way to not fully failed m.direct parsing when only some entries are invalid ?

The entry should just be omitted in this case I believe.

Should I keep implementing FromIterator<(String, Vec<OwnedRoomId>)>, try to .from the string with the UserIdentifier enum we are talking about, and then put that in a BTreeMap<UserIdentifier, Vec<OwnedRoomId>> ?

[MatMaul]

So I think the question is, will serde uses FromIterator<(String, Vec<OwnedRoomId>)> or not to deserialize the JSON in this case ?

If not, what would be suited to do to still deserialize m.direct account data with invalid entries in it ?

[spaetz]

If introducing ENUMS can we have an "UNKNOWN" too please which could catch either nonsense, such as "[object Object]": or other id's that this SDK perhaps does not know about (yet)?

[MatMaul]

Yes I think I have a way with something like that @spaetz , thanks.

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.17%. Comparing base (22cb8a1) to head (b4e9586).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...tes/matrix-sdk-base/src/store/migration_helpers.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4228      +/-   ##
==========================================
+ Coverage   85.15%   85.17%   +0.02%     
==========================================
  Files         280      280              
  Lines       30858    30861       +3     
==========================================
+ Hits        26278    26287       +9     
+ Misses       4580     4574       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bnjbvr]

Does OwnedDirectUserIdentifier deserialize cleanly from an OwnedUserId? Otherwise this needs a migration, since we're storing the BaseRoomInfo in db

[bnjbvr]

Does OwnedDirectUserIdentifier deserialize cleanly from an OwnedUserId? Otherwise this needs a migration, since we're storing the BaseRoomInfo in db

GOOD NEWS, EVERYONE! Since both are encoded as strings, and use the IdZst derive macro, they serialize and deserialize as a string, so this shouldn't require a migration. I would be grateful if a tiny test were added for this, to confirm my understanding: it would serialize an OwnedUserId and deserialize it as a OwnedDirectUserIdentifier cleanly. With that and the other review comment I've made above, LGTM 👍

[MatMaul]

I would be grateful if a tiny test were added for this

This feels like something that should be in Ruma, here is the PR.

[bnjbvr]

lgtm, thanks.