feat: When photos are trashed, delete the reference to albums #3002
Conversation
src/photos/lib/onPhotoTrashed.js
Outdated
| import { DOCTYPE_ALBUMS, DOCTYPE_FILES } from 'drive/lib/doctypes' | ||
|
|
||
| const onPhotoTrashed = async client => { | ||
| log('info', `Service called with COZY_URL: ${process.env.COZY_URL}`) |
There was a problem hiding this comment.
Is this for data protection reasons? Or to keep the console clean?
There was a problem hiding this comment.
Both:
- Why do you need to log the cozy_url?
- We try to avoid to log data within the log. You can do it, you can even log with debug() if needed.
But here, I really don't see the need to log that information.
There was a problem hiding this comment.
It wasn't just a mimicking of the other service. I was asking about applying a correction to this one too, if it was a matter of privacy. I'm going to correct mine so that I don't need it any more 👍
src/photos/lib/onPhotoTrashed.js
Outdated
| 'info', | ||
| `Start deleting album references on ${photos.length} trashed photos` | ||
| ) | ||
| photos.forEach(async photo => { |
There was a problem hiding this comment.
Be careful, in that case, forEach will be call in //. So we can make a lot of requests to the stack and in that case, we open a lot of handlers (handler can be : file opening or... network request).
We have a very low limit on the number of open handlers in prod to avoid crazy memory consumption.
So you should do that differently?
Or do I've a bad understanding of how forEach with async works?
There was a problem hiding this comment.
for ... of would be the equivalent working correctly with async / await
There was a problem hiding this comment.
Yep or p-limit (https://www.npmjs.com/package/p-limit we use it in a few konnectors i think)
There was a problem hiding this comment.
Thanks for the comment, I hadn't realised the difference in performance between for and forEach. I'll think twice before using that one in the future
There was a problem hiding this comment.
In fact, to be more precise:
- forEach is way more performant because we do actions in //
for ofdo actions sync
So in fact, forEach will be more performant in a lof of cases. Except when you have too much work in //. You can fully fill the RAM or... you can reach the opened handlers limit.
My favorite solution for that, is to keep async, but with concurrency. With something like 20 async actions, it's ok. Like that, we still do 20 actions in //, so we have good perf, without killing the servers.
(in our case, you make call from nsjail to the stack, no "user" network concerned or else, so the gain is not as good as when running on the client side).
So if you want to use p-limit go for it, if you want to keep that way, go for it too ;). Your call
There was a problem hiding this comment.
Thanks for the additional explanation. I think I'll stick with for...of because I don't want to add a new dependency to the project and I think the syntax is pretty clean
BundleMonFiles updated (6)
Unchanged files (13)
Total files change -103.63KB -2.21% Groups updated (5)
Unchanged groups (3)
Final result: ✅ View report in BundleMon website ➡️ |
cballevre
force-pushed
the
feat/on-photo-trashed-service
branch
2 times, most recently
from
82fd993 to
c4f4d6a
Compare
src/photos/queries/queries.js
Outdated
| class: 'image', | ||
| trashed: true | ||
| }) | ||
| .indexFields(['class', 'trashed']), |
There was a problem hiding this comment.
since the predicate is static, should we use a partial index @paultranvan ?
There was a problem hiding this comment.
Absolutely: using
.partialIndex({
class: 'image',
trashed: true
})
will pre-filter documents entering the index, so here, only the trashed images. While using the .where will index all the docs, making it unnecessarily bigger, and slower.
I have to add some doc about this rule: when static is your predicate, a partial index you must use
There was a problem hiding this comment.
And we'll also request only the docs that have a referenced_by. This request will be optimized a lot ;). Thanks.
There was a problem hiding this comment.
Thanks for confirming Quentin's explanation. In the case of referenced_by, it doesn't seem to work because this field isn't part of the io.cozy.files document, but I may be wrong.
cballevre
force-pushed
the
feat/on-photo-trashed-service
branch
from
c4f4d6a to
72e43e0
Compare
To develop new service, we use cozy-jobs-cli. To make it easier to use, given that we have 2 apps in the same repository, I've created a specific command for each one. For example, you can start the onPhotoTrashed service with yarn service:photos build/photos/services/onPhotoTrashed/photos.js. Be careful to use the build file
cballevre
force-pushed
the
feat/on-photo-trashed-service
branch
from
72e43e0 to
c4f7bfb
Compare
When you put an image in the trash even though it was part of an album, it is not removed from the album. You can therefore access the photo in the trash via album. To avoid bugs and inconsistencies, we remove the reference to the album via a service after each deletion of a photo
Related PRs:
Remaning work:
Check if we can throttle the service execution