getIgnoredUsers() only checks local cache and will often return wrong results

[tadzik]

The problem

I only noticed it because my editor told me that await has no effect on that method. Looking into it, I noticed that it doesn't actually check anything with the server, it merely looks at the local copy of the account data, ignoring the fact that it may not exist.

This can go wrong in many different ways. If the client has never initial-sync'd, the local account data will be empty, so getIgnoredUsers() will return an empty list. This is true even if setIgnoredUsers() was called earlier on the same instance, since it will update the server but not the local cache.

Any of these sequences of operations will therefore produce wrong results:

const client = sdk.createClient(...);
await client.setIgnoredUsers('@foo:bar');
const ignoredUsers = client.getIgnoredUsers(); // an empty list!

const client = sdk.createClient(...);
// just to ensure it's not setIgnoredUsers() that's wrong
await client.setAccountData('m.ignored_user_list', { ignored_users: { '@foo:bar': {} } });
const ignoredUsers = client.getIgnoredUsers(); // an empty list again

The workaround

Use getAccountDataFromServer('m.ignored_user_list') instead

The solution?

getIgnoredUsers() should be more aware of the state of local account data cache, and probably have a getIgnoredUsersFromServer() variant, following the pattern set by getAccountData().

Somewhat related: getAccountData() itself has a similar problem: it will quietly return wrong results if called before initial sync.

Since either can be easily misused, and quietly produce wrong results, I'd ideally see it flipped around: getAccountData() and getIgnoredUsers() checking the server by default, and new variants called getCachedAccountData() etc being introduced. This will make the obvious usage safe and correct by default, with the "may or may not be correct depending on your specific usecase" methods being a bit out of the way.