add limit on API request body size configurable #1818
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
First of all, thank you for your first contribution and addressing that issue! 🙏
Here are few comments on my side.
Please note that my remarks are those of a regular contributor of the project, but I have no authority at the contrary of Grist Labs employees who approve the code to merge.
| maxUploadSizeAttachment?: number; | ||
|
|
||
| // Max allowed size for API request bodies, in bytes; 0 or omitted for unlimited. | ||
| maxApiRequestBodySize?: number; |
There was a problem hiding this comment.
This object is used to pass environment settings values to the client.
I don't see this property used anywhere. Do you intend to use it client-side?
| function getApiRequestBodyLimit(): string { | ||
| const limitMB = Number(process.env.GRIST_MAX_API_REQUEST_BODY_MB); | ||
| return limitMB > 0 ? `${limitMB}mb` : '1mb'; // Default to 1mb if not set or invalid | ||
| } |
There was a problem hiding this comment.
This is the same logic as in the FlexServer, right?
I guess this could somehow be factorized. I see that attachEarlyEndpoints() is passed the gristServer property (which is the instance of FlexServer), I guess this class could be added a public readonly property that could be read here. Do you think it would make sense?
Also there exists an appSettings variable in FlexServer, that helps to parse the env variables correctly with the readInt function that is passed a AppSettingQueryNumber. This way, you could use a line like this:
yourProperty = appSettings.section('<section name>').flag('<flag name>').readInt({
....
});The <section name> and <flag name> is up to you.
For the section name, these names preexists: access, assistant, attachmentStores, boot, docApi, externalStorage, features, history, integrations, locale, log, login, proxy, telemetry, templates, tutorials.
You can create another one (I don't feel like a relevant one can be reused)
For the flag name, you may use a name that describe the variable (maybe the same name as for the property?).
There was a problem hiding this comment.
Hi @Ajay-Satish-01!
It looks like there are many noises to your last commit, related to code formating.
I don't think we apply such rules in the project lint configuration (we use eslint).
Could you remove the formating changes please? 🙏
Hi, I ran the eslint lint commands in package.json and checked for documentation regarding code formatting. These do not fix the prettier styles my local machine uses. Do you have any other tools I might be missing? |
@Ajay-Satish-01 What are the commands you run for that?
I think there's only eslint for the JS code. |
Oh. Should I include TS then? Or will that be a separate issue? |
Sorry, I meant the TypeScript code. |
Context
There is a limit set on the maximum API request body size. That limit is currently hardcoded to 1mb.
Proposed solution
Added configurable maximum API request body size
Related issues
#916
Has this been tested?
Yes
Screenshots / Screencasts