Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PB-845]: feat/calculate-folder-size #259

Merged
merged 1 commit into from
Jan 18, 2024
Merged

[PB-845]: feat/calculate-folder-size #259

merged 1 commit into from
Jan 18, 2024

Conversation

edisonjpadilla
Copy link
Contributor

@edisonjpadilla edisonjpadilla commented Jan 11, 2024

I'm using a recursive raw query to get each file of each folder belonging to folder father.

the properties that I used to filter in the query all have indexes; file.folder_uuid, folder.uuid, folder.parent_uuid, so improving the performance for each request.

@edisonjpadilla edisonjpadilla requested a review from sg-gs January 11, 2024 05:40
@edisonjpadilla edisonjpadilla changed the title PB-845: Calculate folder size and expose it via shared & Drive PB-845: Calculate folder size Jan 11, 2024
@edisonjpadilla edisonjpadilla self-assigned this Jan 11, 2024
@sg-gs
Copy link
Member

sg-gs commented Jan 11, 2024

Hey @edisonjpadilla, we can not list "all" the folders/files in a given folder. That does not scale well, as with more folders/files, the longer the query.

I do not see any abort option in case the time to make the sum is more than X seconds. This makes the proposed changes a security issue. Use the recursive query to traverse the tree and calculate a sum there. The database should do that. Also, add a timeout control to avoid long lasting queries to keep the database busy all the time.

@sg-gs
Copy link
Member

sg-gs commented Jan 11, 2024

And please, provide a description of the PR + the testing coverage for the new code >80%

@edisonjpadilla
Copy link
Contributor Author

Hey @edisonjpadilla, we can not list "all" the folders/files in a given folder. That does not scale well, as with more folders/files, the longer the query.

I do not see any abort option in case the time to make the sum is more than X seconds. This makes the proposed changes a security issue. Use the recursive query to traverse the tree and calculate a sum there. The database should do that. Also, add a timeout control to avoid long lasting queries to keep the database busy all the time.

I'm just getting the folders uuids and sum file sizes by folders uuids, I thought to do the sum in the recursive query, but I didn't because of the readability of the code and the raw query.

TO DO:

  1. Do the sum in the recursive query
  2. Add a timeout in case it's taking more time than expected

src/lib/add-timeout-to-promise.ts Outdated Show resolved Hide resolved
src/lib/add-timeout-to-promise.spec.ts Outdated Show resolved Hide resolved
src/modules/folder/folder.repository.spec.ts Show resolved Hide resolved
src/modules/folder/folder.repository.ts Outdated Show resolved Hide resolved
src/modules/folder/folder.repository.ts Outdated Show resolved Hide resolved
src/modules/folder/folder.usecase.spec.ts Show resolved Hide resolved
@sg-gs sg-gs added the enhancement New feature or request label Jan 12, 2024
@sg-gs sg-gs changed the title PB-845: Calculate folder size [PB-845]: feat/calculate-folder-size Jan 12, 2024
@edisonjpadilla edisonjpadilla requested a review from sg-gs January 14, 2024 02:13
src/modules/folder/folder.repository.ts Outdated Show resolved Hide resolved
src/modules/folder/folder.usecase.spec.ts Show resolved Hide resolved
src/modules/folder/folder.repository.spec.ts Show resolved Hide resolved
@edisonjpadilla
Copy link
Contributor Author

@sg-gs Why should we test the sequelize library? it has already been tested by the owners, we just should mocking the response and send the same response as is at the sequelize doc and test our code by that.

#259 (comment)

@edisonjpadilla edisonjpadilla requested review from sg-gs and removed request for sg-gs January 17, 2024 02:31
Copy link

@sg-gs sg-gs merged commit 8fcd547 into master Jan 18, 2024
6 of 7 checks passed
@sg-gs sg-gs deleted the feature/PB-845 branch January 18, 2024 14:56
apsantiso pushed a commit that referenced this pull request Jul 11, 2024
[PB-845]: feat/calculate-folder-size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants