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-557] fix/file size as number #334

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

jzunigax2
Copy link
Contributor

Similar to #321, file size's type is changed from BigInt to Number. This would be a breaking change for the mac desktop client, but convertSizeMiddleware is applied to problematic routes as reported by @PixoDev in order to convert the numeric size into string. These routes are:

  • PUT /files/:fileUUID
  • GET /files/:fileUUID/meta
  • GET /folders/:folderId/files
  • GET /files?updatedAt=ISODate

sg-gs
sg-gs previously approved these changes Jun 18, 2024
Copy link
Collaborator

@apsantiso apsantiso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jzunigax2 There are better ways to achieve this without mutating functions, which could have unforeseen effects in future NestJS versions. Let's implement this in a more standard NestJS way.

How about using NestJS Interceptors? They allow you to execute code before and after the handler (where you can implement your current solution). Additionally, interceptors can be bound to controllers.

With the execution context you should have access to the headers too

Copy link

sonarcloud bot commented Jun 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
4 Security Hotspots

See analysis details on SonarCloud

@jzunigax2 jzunigax2 requested a review from apsantiso June 18, 2024 21:48
@jzunigax2
Copy link
Contributor Author

Unable to merge until we can confirm this doesn't break web and mobile clients @CandelR / @PixoDev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants