Remove buffer from file #10
Conversation
| (err, data) => { | ||
| if (err) return reject(err); | ||
| file.url = converter.getUrl(data); | ||
| delete file.buffer; |
There was a problem hiding this comment.
I think the issue comes from line 99 and not this one.
Strapi uses uploadStream (using streams) if it exists, otherwise upload (using buffers).
When Strapi uses upload, Strapi adds the buffer to the file before calling upload and removes it just after.
So I think this change won't have any impact. Also I think it's better to let the responsability to remove it to Strapi.
I see that the uploadStream is not really using the streams as it's converting it to buffer. There are some issues with Buffers in general (memory leaks), that's why Strapi moved forward streams instead.
I would say that the best fix I can see is to re-write a bit the provider to follow the same logic as the aws one : https://github.com/strapi/strapi/blob/main/packages/providers/upload-aws-s3/lib/index.js
There was a problem hiding this comment.
Here uploadStream uses upload under the hood and strapi never removes the buffer (since it assumes it uses a stream). I simply added the deletion to line 86 since it requires the least code changes (and from my understanding would remove the buffer after upload is called by uplaodStream, but it is not unlikely that I overlooked something).
I agree that the uploadStream implementation would benefit from a overhaul like suggested.
There was a problem hiding this comment.
I created the issue #11 to keep track of this. Feel free to close this PR if another approach is taken.
There was a problem hiding this comment.
Yes you're right I forgot it was called by uploadStream. I would advise to delete the buffer in uploadStream however, in order to not mix the usecases and to not change too much things. However I still recommand to rewrite the provider as describe in my first comment
There was a problem hiding this comment.
@gcascio Thanks for the contribution !
@petersg83 I totally agree. This provider could be improved by using the same pattern as the S3 provider, therefore making full use of the stream instead of loosely concatenating some bytes. I worry about the potential breaking changes but I doubts if there will be any.
Feel free to apply the changes you advised in this PR and once reviewed, i'll publish the changes.
There was a problem hiding this comment.
I will merge those changes in the meantime and publish soon. Feel free to provide a new PR for the changes you suggested.
|
Thanks for the fix guys 🙏 @shorwood any idea when this could be published to npm? |
|
It is now ;) |
4 participants
Currently the buffer is never removed from the file object causing strapi to store the buffer in the database.
Here an example of a response (located in the formats field of a media response)
