feat(community)_: add version to image url to let clients update #6118
Conversation
Jenkins BuildsClick to see older builds (39)
|
protocol/communities/community.go
Outdated
| for imageType, newImage := range description.Identity.Images { | ||
| oldImage, ok := o.config.CommunityDescription.Identity.Images[imageType] | ||
| if ok { | ||
| // FIXME this does not work!!!!! |
server/server_media.go
Outdated
| u := s.MakeBaseURL() | ||
| u.Path = communityDescriptionImagesPath | ||
| u.RawQuery = url.Values{ | ||
| "communityID": {communityID}, | ||
| "name": {name}, | ||
| "version": {strconv.FormatUint(uint64(version), 10)}, |
There was a problem hiding this comment.
uint64 here and uint32 everywhere else?
There was a problem hiding this comment.
FormatUint accepts uint64 only 🤷
There was a problem hiding this comment.
🙄 go doesn't have some sort of generics or method overloads? 😮
| @@ -58,6 +58,10 @@ message IdentityImage { | |||
| // encrypted signals the encryption state of the payload, default is false. | |||
| bool encrypted = 5; | |||
|
|
|||
| // version iterator of the image since images use a localUrl to be shown, | |||
| // this iterator makes sure the image URL changes when a new image is given | |||
| uint32 version = 6; | |||
There was a problem hiding this comment.
Wait, so version is not just local, it becomes part of CommunityDescription? 🤔
Sounds a bit weird to be like that, like this problem should be solved solely on client side, not affecting network messages
There was a problem hiding this comment.
Yeah I agree it should be local only, but I didn't find an easy way to do so, since locally, we use the IdentityImage protobuf in the image server.
I'm very open for suggestions to make it locally, but apart from creating some sort of new column or table, I don't know how to make it work.
In the end, this solution is pretty unobtrusive anyway IMO.
There was a problem hiding this comment.
@jrainville I think I'm not convinced, modifying protocol for local needs sounds counter-intuitive to me.
We should probably wrap the IdentityImage when passing it to ServerMedia, and add the Version property to the wrapper. Dunno, or make some method of the media server to update the version of specific image.
creating some sort of new column or table
We don't need to persist the version, it's enough to have it only during a single run. After restart, version can go again from 1, should work, right?
There was a problem hiding this comment.
We don't need to persist the version, it's enough to have it only during a single run. After restart, version can go again from 1, should work, right?
That's true. I'll try to see if I can do as you say
There was a problem hiding this comment.
I agree with Igor here.
At first glance, the best would be to use/extend CommunityChanges with image change recognition and then populate it to client.
There was a problem hiding this comment.
We don't need to persist the version, it's enough to have it only during a single run. After restart, version can go again from 1, should work, right?
Ok, I tried something simple and I realized that we indeed need to save it somewhere. At least a local cache. However, we do not have a local cache for communities. The closest we have is the encrypted_community_description_cache which is a table. It is also what the image server uses to serve the images.
Just to make sure the problem is clear, I can't just pass the updated version to the image server, since the Edit happens in one thread/timeframe and the image serving is a completely different one, so the image server knows nothing about image updates.
It serves using the community cache table, which is populated from the decrypted community description. So we need the version or at least an indicator that the images were updated in there.
So either I create a new local cache for image versions per community, or I add a new column encrypted_community_description_cache to save the version, or leave it as I did it. Or you guys have a better option?
| @@ -58,6 +58,10 @@ message IdentityImage { | |||
| // encrypted signals the encryption state of the payload, default is false. | |||
| bool encrypted = 5; | |||
|
|
|||
| // version iterator of the image since images use a localUrl to be shown, | |||
| // this iterator makes sure the image URL changes when a new image is given | |||
| uint32 version = 6; | |||
There was a problem hiding this comment.
I agree with Igor here.
At first glance, the best would be to use/extend CommunityChanges with image change recognition and then populate it to client.
jrainville
force-pushed
the
fix/community-image-not-updating
branch
from
dfc4dc8
to
65d153d
Compare
jrainville
changed the base branch from
develop
to
refactor/dont-use-cache-for-image-server
|
@igor-sirotin @osmaczko I rebased this branch on top of #6127 as I refactored the media sever to not use the community cache. As for the version being in the protobuf, I sadly didn't find a good solution. I'm still hoping you guys can think of something 😕 |
|
Patryk gave me a good suggestion:
Using a local map in the manager |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor/dont-use-cache-for-image-server #6118 +/- ##
=========================================================================
Coverage 0.00% 0.00%
=========================================================================
Files 811 811
Lines 108069 108080 +11
=========================================================================
- Misses 108069 108080 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
fix/community-image-not-updating
branch
2 times, most recently
from
64d4e66
to
16a9843
Compare
jrainville
requested review from
osmaczko,
qfrank,
ilmotta,
igor-sirotin and
caybro
|
I updated the PR to go with Patryk's recommendation. I no longer need the protobuf changes and the version isn't saved anymore. Please let me know what you think 😄 |
jrainville
force-pushed
the
refactor/dont-use-cache-for-image-server
branch
from
d197857
to
064d880
Compare
jrainville
force-pushed
the
fix/community-image-not-updating
branch
from
16a9843
to
b8d2544
Compare
jrainville
force-pushed
the
refactor/dont-use-cache-for-image-server
branch
from
064d880
to
bc4b467
Compare
Fixes status-im/status-desktop#16688 Since we use the local image server to show the community image, the URL never changes when we update the image, since it's served using a query string containing the community ID. eg: `https://Localhost:46739/communityDescriptionImages?communityID=0x03c5ece7da362d31199fb02d632f85fdf853af57d89c3204b4d1e90c6ec13bb23c&name=thumbnail` Because of that, the clients cannot know if the image was updated, so they had to force update the image every time, which was inefficient. We discovered this issue when I refactored the community client code in Desktop so that we only update the changed properties of a community instead of reseting the whole thing. The solution I came up with in the PR is to add a `version` to the URL when we detect that the image changed. This let's the clients detect when the image was updated without having to do any extra logic.
jrainville
force-pushed
the
fix/community-image-not-updating
branch
from
b8d2544
to
b3659a1
Compare
| version, ok := manager.communityImageVersions[communityID] | ||
| if !ok { | ||
| return 0 | ||
| } | ||
| return version |
There was a problem hiding this comment.
| version, ok := manager.communityImageVersions[communityID] | |
| if !ok { | |
| return 0 | |
| } | |
| return version | |
| return manager.communityImageVersions[communityID] |
| @@ -1616,6 +1625,15 @@ func (m *Manager) UpdatePubsubTopicPrivateKey(topic string, privKey *ecdsa.Priva | |||
| return m.transport.RemovePubsubTopicKey(topic) | |||
| } | |||
|
|
|||
| func (m *Manager) incrementCommunityImageVersion(communityID string) { | |||
There was a problem hiding this comment.
Would be great to add a comment explaining why this is needed.
| @@ -243,6 +255,7 @@ func (s *MediaServer) MakeCommunityImageURL(communityID, name string) string { | |||
| u.RawQuery = url.Values{ | |||
| "communityID": {communityID}, | |||
| "name": {name}, | |||
| "version": {strconv.FormatUint(uint64(s.getCommunityImageVersion(communityID)), 10)}, | |||
There was a problem hiding this comment.
| "version": {strconv.FormatUint(uint64(s.getCommunityImageVersion(communityID)), 10)}, | |
| "version": {fmt.Sprintf("%d", s.getCommunityImageVersion(communityID))}, |
| version, ok := m.communityImageVersions[communityID] | ||
| if !ok { | ||
| // First time the image is updated, so update the version to 1 (it was 0 by default) | ||
| m.communityImageVersions[communityID] = 1 | ||
| } | ||
| m.communityImageVersions[communityID] = version + 1 |
There was a problem hiding this comment.
| version, ok := m.communityImageVersions[communityID] | |
| if !ok { | |
| // First time the image is updated, so update the version to 1 (it was 0 by default) | |
| m.communityImageVersions[communityID] = 1 | |
| } | |
| m.communityImageVersions[communityID] = version + 1 | |
| m.communityImageVersions[communityID] = m.communityImageVersions[communityID] + 1 |
Fixes status-im/status-desktop#16688
Since we use the local image server to show the community image, the URL never changes when we update the image, since it's served using a query string containing the community ID. eg:
https://Localhost:46739/communityDescriptionImages?communityID=0x03c5ece7da362d31199fb02d632f85fdf853af57d89c3204b4d1e90c6ec13bb23c&name=thumbnailBecause of that, the clients cannot know if the image was updated, so they had to force update the image every time, which was inefficient.We discovered this issue when I refactored the community client code in Desktop so that we only update the changed properties of a community instead of reseting the whole thing.
The solution I came up with in the PR is to add a
versionto the URL when we detect that the image changed. This let,s the clients detect when the image was updated without having to do any extra logic.