keep cached avatar&c when refresh fails to get new values

Created by: dakkar

when the remote explicitly tells us a user image is gone, we remove our cached value, but if we fail to get the image, we keep whatever value we already have

this should minimise the problem of avatars randomly disappearing

tested this way:

• fetched a remote user, avatar was set ok
• removed avatar id+url from the db, refreshed remote data, avatar was set again correctly
• set background url in the db to a random value, refreshed remote data, background url was null-ed correctly
• faked an error in the code that fetches remote images, refreshed remote data, the avatar was not removed from the database

Checklist

• Read the contribution guide
• Test working in a local environment
• (If needed) Add story of storybook
• (If needed) Update CHANGELOG.md
• (If possible) Add tests

if the database fields are already null, nothing changes;

This would break the unset avatar and unset banner feature for admins as well as the remove banner, remove background and remove avatar feature as it sets all of those to null when they get removed if I understood the comment correctly.

As the fake out could also break a users account potentionally as all it does is push the data provided into the user's column

so if it just returns id, url, blurhash it would fail on blurhash would not fail on url and would not fail on id but since blurhash doesn't exist the entire update query just breaks already.

Also if ((img == null) || (typeof img === 'object' && img.url == null)) { is basically the same check resolveImage does

(the top resolve fails if object/image is null)

Created by: dakkar

hmm

I think this code only applies to remote users, right? and I tested that, when we have some value in the db for (e.g.) the background, but the remote instance doesn't, refreshing data correctly removes the value from the database. so I'm not sure how this can break "unset avatar"&c

my change does not produce any more "null"s than the current code. In the current code, if (for example) we fail to retrieve the avatar image, avatar is going to be null, so the resolveAvatarAndBanner function returns avatar: null, avatarUrl: null, avatarBlurhash: null my code would not return any of those three values (unless the person.icon==null or person.icon.url==null), and I'm not sure what you mean by "push the data provided into the user's column"

and yes, that check is the same as the one in resolveImage, but I have to distinguish "we failed to retrieve an image that we were told should exist" from "we were told there's no image", and either I do that check, or match the error message, which is usually a bad idea. better ways to distinguish the two cases are very welcome

I feel like this should generally be PR'd upstream rather than here cause I feel like they would be also able to check this stuff better and it would be a general help for misskey

Created by: dakkar

yes, I agree! I'll send them a PR probably tomorrow, referencing this one

changed the description

requested review from @Marie

assigned to @dakkar

added 15 commits

• d02667b9...aabf168d - 14 commits from branch develop
• 6a5ff041 - keep cached avatar&c when refresh fails to get new values

Compare with previous version

https://github.com/misskey-dev/misskey/pull/13145 upstreamed (minus the background image bits, obvs)

approved this merge request

should we wait for upstream to merge it first?

considering the speed of PRs being merged sometimes from misskey's side in my opinion not really.

resolved all threads

merged upstream! once those changes get imported here, I'll rebase this to get the backgroundUrl bits in.

mentioned in merge request !408 (merged)

@Marie has already incorporated these changes into !408 (merged) so this can be closed

closed

added federation label