From 9f0418893f74ec001412f558d518e9c5085b2fe0 Mon Sep 17 00:00:00 2001 From: Iain Sproat <68657+iainsproat@users.noreply.github.com> Date: Wed, 12 Jul 2023 10:57:59 +0100 Subject: [PATCH] chore(objects upload): improves response when error due to large object size (#1685) * test(objects upload): adds a test for large object * print object too large messages in response * allows object maximum size to be configured --- packages/server/Dockerfile | 2 + packages/server/modules/core/errors/object.ts | 11 +++++ packages/server/modules/core/rest/upload.js | 42 +++++++++++++------ .../server/modules/core/services/objects.js | 10 +++-- .../server/modules/core/tests/rest.spec.js | 16 +++++++ .../modules/shared/helpers/envHelper.ts | 4 ++ .../templates/server/deployment.yml | 7 +++- utils/helm/speckle-server/values.schema.json | 5 +++ utils/helm/speckle-server/values.yaml | 2 + 9 files changed, 82 insertions(+), 17 deletions(-) create mode 100644 packages/server/modules/core/errors/object.ts diff --git a/packages/server/Dockerfile b/packages/server/Dockerfile index 742d372fa..a91770d9c 100644 --- a/packages/server/Dockerfile +++ b/packages/server/Dockerfile @@ -60,8 +60,10 @@ FROM node:18.16.1-bullseye-slim as production-stage ARG NODE_ENV ARG SPECKLE_SERVER_VERSION ARG FILE_SIZE_LIMIT_MB=100 +ARG MAX_OBJECT_SIZE_MB=10 ENV FILE_SIZE_LIMIT_MB=${FILE_SIZE_LIMIT_MB} \ + MAX_OBJECT_SIZE_MB=${MAX_OBJECT_SIZE_MB} \ NODE_ENV=${NODE_ENV} \ SPECKLE_SERVER_VERSION=${SPECKLE_SERVER_VERSION} diff --git a/packages/server/modules/core/errors/object.ts b/packages/server/modules/core/errors/object.ts new file mode 100644 index 000000000..a0da9e1c7 --- /dev/null +++ b/packages/server/modules/core/errors/object.ts @@ -0,0 +1,11 @@ +import { BaseError } from '@/modules/shared/errors' +import { Options } from 'verror' + +export class ObjectHandlingError extends BaseError { + static defaultMessage = 'Failed to handle object.' + static code = 'OBJECT_HANDLING_ERROR' + + constructor(message?: string | undefined, options?: Options | Error | undefined) { + super(message, options) + } +} diff --git a/packages/server/modules/core/rest/upload.js b/packages/server/modules/core/rest/upload.js index 3f46d2e56..aae646ff2 100644 --- a/packages/server/modules/core/rest/upload.js +++ b/packages/server/modules/core/rest/upload.js @@ -5,7 +5,8 @@ const Busboy = require('busboy') const { validatePermissionsWriteStream } = require('./authUtils') -const { createObjectsBatched } = require('../services/objects') +const { createObjectsBatched } = require('@/modules/core/services/objects') +const { ObjectHandlingError } = require('@/modules/core/errors/object') const MAX_FILE_SIZE = 50 * 1024 * 1024 @@ -113,12 +114,21 @@ module.exports = (app) => { const promise = createObjectsBatched(req.params.streamId, objs).catch((e) => { req.log.error(e, `Upload error.`) - if (!requestDropped) - res - .status(400) - .send( - 'Error inserting object in the database. Check server logs for details' - ) + if (!requestDropped) { + switch (e.constructor) { + case ObjectHandlingError: + res + .status(400) + .send(`Error inserting object in the database: ${e.message}`) + break + default: + res + .status(400) + .send( + 'Error inserting object in the database. Check server logs for details' + ) + } + } requestDropped = true }) promises.push(promise) @@ -194,11 +204,19 @@ module.exports = (app) => { const promise = createObjectsBatched(req.params.streamId, objs).catch((e) => { req.log.error(e, `Upload error.`) if (!requestDropped) - res - .status(400) - .send( - 'Error inserting object in the database. Check server logs for details' - ) + switch (e.constructor) { + case ObjectHandlingError: + res + .status(400) + .send(`Error inserting object in the database. ${e.message}`) + break + default: + res + .status(400) + .send( + 'Error inserting object in the database. Check server logs for details' + ) + } requestDropped = true }) promises.push(promise) diff --git a/packages/server/modules/core/services/objects.js b/packages/server/modules/core/services/objects.js index 3ee1489ef..194a1da55 100644 --- a/packages/server/modules/core/services/objects.js +++ b/packages/server/modules/core/services/objects.js @@ -5,6 +5,8 @@ const { set, get, chunk } = require('lodash') const knex = require(`@/db/knex`) const { servicesLogger } = require('@/logging/logging') +const { getMaximumObjectSizeMB } = require('@/modules/shared/helpers/envHelper') +const { ObjectHandlingError } = require('@/modules/core/errors/object') const Objects = () => knex('objects') const Closures = () => knex('object_children_closure') @@ -586,7 +588,7 @@ module.exports = { // NOTE: Derive Object async updateObject() { - throw new Error('not implemeneted') + throw new Error('Updating object is not implemented') } } @@ -595,7 +597,7 @@ module.exports = { // we cannot provide a full response back including all object hashes. function prepInsertionObject(streamId, obj) { // let memNow = process.memoryUsage().heapUsed / 1024 / 1024 - const MAX_OBJECT_SIZE = 10 * 1024 * 1024 + const MAX_OBJECT_SIZE = getMaximumObjectSizeMB() * 1024 * 1024 if (obj.hash) obj.id = obj.hash else @@ -604,7 +606,9 @@ function prepInsertionObject(streamId, obj) { const stringifiedObj = JSON.stringify(obj) if (stringifiedObj.length > MAX_OBJECT_SIZE) { - throw new Error(`Object too large (${stringifiedObj.length} > ${MAX_OBJECT_SIZE})`) + throw new ObjectHandlingError( + `Object too large. (${stringifiedObj.length} > ${MAX_OBJECT_SIZE})` + ) } // let memAfter = process.memoryUsage().heapUsed / 1024 / 1024 diff --git a/packages/server/modules/core/tests/rest.spec.js b/packages/server/modules/core/tests/rest.spec.js index 1a68674eb..d0f17769c 100644 --- a/packages/server/modules/core/tests/rest.spec.js +++ b/packages/server/modules/core/tests/rest.spec.js @@ -237,6 +237,22 @@ describe('Upload/Download Routes @api-rest', () => { expect(res).to.have.status(400) }) + it('Should not allow upload with invalid body (object too large)', async () => { + //creating a single valid object larger than 10MB + const objectToPost = { + name: 'x'.repeat(10 * 1024 * 1024 + 1) + } + + const res = await request(app) + .post(`/objects/${testStream.id}`) + .set('Authorization', userA.token) + .set('Content-type', 'multipart/form-data') + .attach('batch1', Buffer.from(JSON.stringify([objectToPost]), 'utf8')) + + expect(res).to.have.status(400) + expect(res.text).contains('Object too large') + }) + let parentId const numObjs = 5000 const objBatches = [ diff --git a/packages/server/modules/shared/helpers/envHelper.ts b/packages/server/modules/shared/helpers/envHelper.ts index 727dfda5a..6b0ea70c1 100644 --- a/packages/server/modules/shared/helpers/envHelper.ts +++ b/packages/server/modules/shared/helpers/envHelper.ts @@ -37,6 +37,10 @@ export function getFileSizeLimitMB() { return getIntFromEnv('FILE_SIZE_LIMIT_MB', '100') } +export function getMaximumObjectSizeMB() { + return getIntFromEnv('MAX_OBJECT_SIZE_MB', '10') +} + export function getIntFromEnv(envVarKey: string, aDefault = '0'): number { return parseInt(process.env[envVarKey] || aDefault) } diff --git a/utils/helm/speckle-server/templates/server/deployment.yml b/utils/helm/speckle-server/templates/server/deployment.yml index a78189bfd..9d7bd9621 100644 --- a/utils/helm/speckle-server/templates/server/deployment.yml +++ b/utils/helm/speckle-server/templates/server/deployment.yml @@ -107,7 +107,7 @@ spec: {{- else }} value: http://{{ .Values.domain }} {{- end }} - + - name: ONBOARDING_STREAM_ID value: {{ .Values.server.onboardingStreamId }} @@ -120,6 +120,9 @@ spec: - name: FILE_SIZE_LIMIT_MB value: {{ .Values.file_size_limit_mb | quote }} + - name: MAX_OBJECT_SIZE_MB + value: {{ .Values.server.max_object_size_mb | quote }} + - name: SPECKLE_AUTOMATE_URL value: {{ .Values.server.speckleAutomateUrl }} @@ -276,7 +279,7 @@ spec: - name: EMAIL_FROM value: "{{ .Values.server.email.from }}" {{- end }} - + # *** Newsletter *** {{- if (default false (.Values.server.mailchimp).enabled) }} - name: MAILCHIMP_ENABLED diff --git a/utils/helm/speckle-server/values.schema.json b/utils/helm/speckle-server/values.schema.json index f1d48934e..0b297f9aa 100644 --- a/utils/helm/speckle-server/values.schema.json +++ b/utils/helm/speckle-server/values.schema.json @@ -491,6 +491,11 @@ "description": "The id of the stream to be used for onboarding new users", "default": "" }, + "max_object_size_mb": { + "type": "number", + "description": "The maximum size of an individual object which can be uploaded to the server", + "default": 10 + }, "speckleAutomateUrl": { "type": "string", "description": "The url of the Speckle Automate instance", diff --git a/utils/helm/speckle-server/values.yaml b/utils/helm/speckle-server/values.yaml index 74a4a27b6..ea438a242 100644 --- a/utils/helm/speckle-server/values.yaml +++ b/utils/helm/speckle-server/values.yaml @@ -387,6 +387,8 @@ server: adminOverrideEnabled: false ## @param server.onboardingStreamId The id of the stream to be used for onboarding new users onboardingStreamId: '' + ## @param server.max_object_size_mb The maximum size of an individual object which can be uploaded to the server + max_object_size_mb: 10 ## @param server.speckleAutomateUrl The url of the Speckle Automate instance speckleAutomateUrl: 'https://automate.speckle.systems' sessionSecret: