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

All modified and coverable lines are covered by tests ✅

Project coverage is 84.93%. Comparing base (aca83fb) to head (2022ae9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4228      +/-   ##
==========================================
+ Coverage   84.92%   84.93%   +0.01%     
==========================================
  Files         274      274              
  Lines       29805    29805              
==========================================
+ Hits        25311    25316       +5     
+ Misses       4494     4489       -5     

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

[jplatte]

Potentially stating the obvious here, but if you wanna do a crates.io release soon (I think this was planned), make sure to only merge this PR afterwards 😉

[poljar]

New policy in town, no git deps anymore. That's why cargo-deny makes the CI fail.

[jplatte]

Oh. Well I don't think we'll have the Ruma change blocking this PR out on crates.io anytime soon.

[MatMaul]

No rush on my side, it can wait.

Feel free to review in the meantime, so we can merge when we got a ruma release out.