Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_service): disallow large files from being parsed and analyzed #3339

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Oct 5, 2022

Summary

Fixes #3330

This PR adds a maximum size threshold to the Workspace after which files are no longer parsed, analyzed or formatted. At the moment I've hard-coded this limit to 1 MiB, but it should be trivial to expose this limit in the configuration file or the CLI if users really want to process very large files.

As part of the new message for this new error, I've added a new Bytes utility to rome_console that prints a quantity in bytes using the correct binary unit (B, KiB, MiB, GiB or TiB)

Test Plan

I've added new test cases for the CLI that ensures files larger than the size threshold are not checked or formatted, and the correct diagnostic is emitted.

@leops leops requested a review from ematipico as a code owner October 5, 2022 13:08
@netlify
Copy link

netlify bot commented Oct 5, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit 47e475e
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/633e813ad6085c0008652dc2

@leops leops temporarily deployed to netlify-playground October 5, 2022 13:08 Inactive
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

@leops leops force-pushed the fix/file-size-limit branch from 9478178 to 568655d Compare October 5, 2022 14:09
@leops leops temporarily deployed to netlify-playground October 5, 2022 14:09 Inactive
```block
ci.js lint ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

× The file ci.js could not be parsed because it's too large (the file is 1.0 MiB long, but the size limit is 1.0 MiB)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a coincidence that the input file is 1.0 Megabyte like the limit? If not, probably that's because we truncated some digits. I am not sure what to do... surely an inexperienced user might think that's a bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of both, in this case the test file is a few bytes larger than 1 MiB but that doesn't show since the values higher than 1 KiB are not printed at full precision. However the file size limit is checked using >= so even a file that's precisely 1 MiB in size would be rejected and print the error anyway.

@leops leops force-pushed the fix/file-size-limit branch from 568655d to 47e475e Compare October 6, 2022 07:18
@leops leops temporarily deployed to netlify-playground October 6, 2022 07:18 Inactive
@leops leops merged commit bc4393e into main Oct 6, 2022
@leops leops deleted the fix/file-size-limit branch October 6, 2022 07:29
@leops leops added the A-CLI Area: CLI label Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 rome check . seems to run in an infinite loop with 99% CPU usage
3 participants