Server: 201 response-code avatars are not cache-able

[mario]

A few releases back we introduced 201 response code on the avatars endpoint meaning: "You didn't set a custom avatar, so we generated one for you". This was awesome because it allowed us to do custom logic for the incoming call screen.

Unfortunately, it came to my attention recently that 201 is not cache-able according to the relevant HTTP specs so I'm looking for solutions here. Can we go back to 200 and maybe introduce a JSON response in addition to image payload? Something along the lines of
{ "custom": true} would be enough.

Poking @nickvergessen and @rullzer as relevant parties

[kesselb]

Hmm. Caching a 201 response seems to be weird. It should be still possible to cache this response but you probably cannot use a library for that. Appending some json to the image is complex. You have to strip it from the image.

Index: core/Controller/AvatarController.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- core/Controller/AvatarController.php    (revision 88a24d7d120fc30e3655f371d64fbdbe02a93241)
+++ core/Controller/AvatarController.php    (date 1577632716484)
@@ -142,8 +142,8 @@
            $avatarFile = $avatar->getFile($size);
            $resp = new FileDisplayResponse(
                $avatarFile,
-               $avatar->isCustomAvatar() ? Http::STATUS_OK : Http::STATUS_CREATED,
-               ['Content-Type' => $avatarFile->getMimeType()]
+               Http::STATUS_OK,
+               ['Content-Type' => $avatarFile->getMimeType(), 'X-NC-IsCustomAvatar' => (int)$avatar->isCustomAvatar()]
            );
        } catch (\Exception $e) {
            return new JSONResponse([], Http::STATUS_NOT_FOUND);

This patch will change the response code to 200 and send a new header with the response X-NC-IsCustomAvatar. 1 custom, 0 otherwise. Are you able to use a custom header? Or does this introduce more work than parsing a json with custom and the image data.

[mario]

@kesselb I did manage to cache it by overwriting the response code, but still...

If the patch is acceptable to others, I'd say go for it - using a header is not a problem at all.

If I want offline functionality, then I have to cache everything :)

[kesselb]

cc @nextcloud/android @nextcloud/ios @nextcloud/desktop @nickvergessen @rullzer

Should we add a new endpoint for this? Like /avatar/v2/{userId}/{size}? Or just change the method.

[mario]

I'd avoid new endpoint if possible, but that's just me :)

[marinofaggiana]

... me too .... all avatar are lost ...

[tobiasKaminsky]

... me too .... all avatar are lost ...

No, avatars are identified by name and eTag. This should not change when switching to a new endpoint.

Technically I am a bit sceptical if out of a sudden the same endpoint behaves different.

[marinofaggiana]

No, avatars are identified by name and eTag. This should not change when switching to a new endpoint.

right but ... I'm investigating ...

[nickvergessen]

I'm fine either way (v2 endpoint or modifying)

Any preference @rullzer

[rullzer]

I mean I think the goal originally of the 201 was that the apps could distinguis between generated and real avatars. But in the end how usefull is that.

I guess just always returning a 200 is just fine.

[mario]

It's useful because based on that I decide whether to show color or blurred
avatar background.

On Fri, 7 Feb 2020, 09:06 Roeland Jago Douma, [e-mail]
wrote:

I mean I think the goal originally of the 201 was that the apps could
distinguis between generated and real avatars. But in the end how usefull
is that.

I guess just always returning a 200 is just fine.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nextcloud/server/issues/18603?email_source=notifications&email_token=AAABNMUQEUSY7DE24U6MIOLRBUJBDA5CNFSM4KA3SBVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELCCEMQ#issuecomment-583279154,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAABNMWXHU4GZKQA6WLHONDRBUJBDANCNFSM4KA3SBVA
.

[nickvergessen]

So 200 and we add the header?
It's an acceptable misbehaviour and not breakign something too hard. You will show a blurred version of the circle background, should be just as fine.

[mario]

Yes, with the header it's fine.

On Fri, 7 Feb 2020, 09:54 Joas Schilling, [e-mail] wrote:

So 200 and we add the header?
It's an acceptable misbehaviour and not breakign something too hard. You
will show a blurred version of the circle background, should be just as
fine.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/nextcloud/server/issues/18603?email_source=notifications&email_token=AAABNMXM6CAFP7JWJNHNI4LRBUOVHA5CNFSM4KA3SBVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELCFZNY#issuecomment-583294135,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAABNMV47AGAEOUCU65A4ITRBUOVHANCNFSM4KA3SBVA
.