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

CUMULUS-2944: granules/bulkDelete endpoint has PayloadTooLargeError for 4k granules #3122

Merged
merged 12 commits into from Oct 20, 2022

Conversation

botanical
Copy link
Member

Summary: Summary of changes

Addresses CUMULUS-2944: granules/bulkDelete endpoint has PayloadTooLargeError for 4k granules

Changes

  • Updated the limit on body-parser's parser to 1mb

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Ad-hoc testing - Deploy changes and test manually
    • Tested by creating > 4k granules and hitting the bulk delete endpoint to reproduce error. Then updated and deployed the body parser limit change and tested again.
  • Integration tests

@@ -70,7 +70,7 @@ app.use(boom());
app.use(morgan('combined'));
app.use(cors());
app.use(cookieParser());
app.use(bodyParser.urlencoded({ extended: true }));
app.use(bodyParser.urlencoded({ limit: '1mb', extended: true }));
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we make this limit configurable? The current default (when you don't specify a limit) is 100kb.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any input on a reasonable number for the limit here would be much appreciated as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to set it to 6MB which is the lambda payload limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@botanical botanical marked this pull request as ready for review October 11, 2022 23:27
@@ -16,6 +16,7 @@ const {
setAuthorizedOAuthUsers,
} = require('../../../lib/testUtils');
const AccessToken = require('../../../models/access-tokens');
const { granuleIds } = require('../../helpers/granulesList');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we dynamically create the granuleId list?
Also the granuleId can be very long, e.g. SWOT_L2_HR_Raster_100m_UTM12R_N_x_x_x_007_162_051F_20220809T224919_20220809T224939_Dx0000_01

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jennyhliu jennyhliu left a comment

Choose a reason for hiding this comment

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

👍

@botanical botanical merged commit 878d396 into master Oct 20, 2022
@botanical botanical deleted the CUMULUS-2944 branch October 21, 2022 15:41
npauzenga added a commit that referenced this pull request Oct 27, 2022
* initial commit

* add postgres query to LIST endpoints

* add tests for granules and collections

* Release 13.3.x with master (#3121)

* Release 13.3.1 (#3106)

* merge CUMULUS-2557

* merge CUMULUS-2971

* CUMULUS-3021: Support Collections with a collection.version containing slashes (#3096)

* Support Collections with a collection.version containing slashes

* merge CUMULUS-3021

* merge CUMULUS-3024

* bump version to 13.3.1

* update documentation

* update changelog

* remove duplicate api user

* Sort results in unit test before comparison

Co-authored-by: Charles Huang <charleshuang80@users.noreply.github.com>
Co-authored-by: jennyhliu <34660846+jennyhliu@users.noreply.github.com>
Co-authored-by: Jonathan Kovarik <kovarik@nsidc.org>

* CUMULUS-3027 -- Tightly constrain typescript version due to knex typing (#3095)

* Tightly constrain typescript verseion due to knex typing

Unpinned dependency resulted in typescript 4.8 being installed,
however knex (all versions) has an outstanding bug:
knex/knex#5279

* Add CHANGELOG 🔔

* Update generate-ts-build-cache to always 'npm install'

* Update CHANGELOG

* Release 13.3.2 (#3117)

* Update CNM lambdas to utilize newer releases (#3094)

* 13.3.2 version bump

* update changelog

Co-authored-by: Jonathan Kovarik <kovarik@nsidc.org>

* update changelog

* re-add comma

Co-authored-by: Charles Huang <charleshuang80@users.noreply.github.com>
Co-authored-by: jennyhliu <34660846+jennyhliu@users.noreply.github.com>
Co-authored-by: Jonathan Kovarik <kovarik@nsidc.org>

* fix lint error

* CUMULUS-3104:Update async operation image Dockerfile (#3130)

* CUMULUS-3104:Update async operation image Dockerfile to use node:14.19.3-buster
Upgraded saml2-js 4.0.0, rewire to 6.0.0 to address security vulnerabilities

* add xmldom to audit allowlist

* update default async_operation_image

* update changelog skip-integration-tests

* CUMULUS-2944: granules/bulkDelete endpoint has PayloadTooLargeError for 4k granules (#3122)

* Update body parser configuration to increase limit

* Add unit test, update changelog

* Update unit test and fix lint

* Fix lint errors

* Fix lint error

* Relocate granulesList.js and update reference in unit test

* Update helper function to dynamically generate IDs and move to different file. Update unit test

* Fix lint errors

* Increase limit to match lambda limit

* CUMULUS-3102: Update broken link (#3125)

* Fix docs that has bad link

* Update url to reference markdown file directly

* CUMULUS-3104: Fixed TS compilation error caused by @aws-sdk/client-s3 upgrade (#3132)

* Fixed TS compilation error caused by @aws-sdk/client-s3 3.190->3.193 upgrade

* fix cumulus/package.json

* update lerna to fix security vulnerability

* fix lzards-api-client package

* temporarily skip deleteS3Objects test

* remove @Cumulus from root, skip test deleteObjects, add missing package

* fix ci post build failure

* put xmldom back to allow list

* add comments for skipped test

Co-authored-by: Charles Huang <charleshuang80@users.noreply.github.com>
Co-authored-by: jennyhliu <34660846+jennyhliu@users.noreply.github.com>
Co-authored-by: Jonathan Kovarik <kovarik@nsidc.org>
Co-authored-by: Jennifer Tran <12633533+botanical@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants