Skip to content

fix!: throw InvalidDataLengthError when input exceeds MAX_DATA_LENGTH#136

Merged
achingbrain merged 6 commits intoalanshaw:masterfrom
acul71:issue-135
Feb 3, 2025
Merged

fix!: throw InvalidDataLengthError when input exceeds MAX_DATA_LENGTH#136
achingbrain merged 6 commits intoalanshaw:masterfrom
acul71:issue-135

Conversation

@acul71
Copy link

@acul71 acul71 commented Jan 24, 2025

Description

Changes Made:

  • Created a new constants.ts file:

    • Added a central location for storing constants like MAX_DATA_LENGTH and MAX_LENGTH_LENGTH.
  • Modified decode.ts and encode.ts:

    • Both files now import constants from constants.ts, ensuring that the constants are defined only once and are accessible throughout the codebase.
  • Added a check for MAX_DATA_LENGTH in encode.ts:

    • Implemented a validation in encode.ts to throw an InvalidDataLengthError if the input exceeds the defined MAX_DATA_LENGTH. This helps to avoid processing data that's too large.
  • Added tests:

    • Created tests to verify that the validation correctly throws an error when input exceeds MAX_DATA_LENGTH.

Motivation:

This change improves maintainability by centralizing constant definitions in constants.ts. It also ensures that input validation for MAX_DATA_LENGTH and the custom maxDataLength is consistently applied and prevents potential issues when dealing with large data.

How to Test:

  • Run the existing tests to ensure the functionality works as expected.
  • Specifically, check that an error is thrown when the input data size exceeds MAX_DATA_LENGTH or the custom maxDataLength

Notes:

  • The InvalidDataLengthError class has been utilized to handle cases where the data exceeds the maximum allowed length.
  • Ensure that constants.ts is updated if the limits (e.g., MAX_DATA_LENGTH) need to change in the future.

@acul71
Copy link
Author

acul71 commented Jan 24, 2025

@alanshaw @achingbrain

@acul71
Copy link
Author

acul71 commented Jan 24, 2025

@achingbrain lint issue fixed.
Hope everything is right now!

Copy link
Collaborator

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM. This should be released as a major since there's now a default max input length - it'll now throw in places it didn't previously.

@achingbrain achingbrain changed the title fix: throw InvalidDataLengthError when input exceeds MAX_DATA_LENGTH fix!: throw InvalidDataLengthError when input exceeds MAX_DATA_LENGTH Jan 25, 2025
acul71 and others added 2 commits January 25, 2025 21:41
@acul71
Copy link
Author

acul71 commented Jan 28, 2025

@achingbrain In 8f31186 There are some comments so I can ask you questions:

it('should not encode message data that is too long PROMISE', async () => {
    const customMaxDataLength = MAX_DATA_LENGTH
    const input = new Uint8Array(customMaxDataLength + 1) // Create a buffer larger than the max allowed

    // Wrap in a Promise to ensure compatibility with async assertions
    await expect(
      Promise.resolve().then(() => lp.encode.single(input))
    ).to.eventually.be.rejected.with.property('code', 'ERR_MSG_DATA_TOO_LONG')
    // ).to.eventually.be.rejectedWith(InvalidDataLengthError).and.have.property('code', 'ERR_MSG_DATA_TOO_LONG')
  })

I tried to use the same matchers to be slightly more expressive, but I have to wrap encode.single in a promise, does it eventually make sense?

@SgtPooki
Copy link

@achingbrain In 8f31186 There are some comments so I can ask you questions:

it('should not encode message data that is too long PROMISE', async () => {
    const customMaxDataLength = MAX_DATA_LENGTH
    const input = new Uint8Array(customMaxDataLength + 1) // Create a buffer larger than the max allowed

    // Wrap in a Promise to ensure compatibility with async assertions
    await expect(
      Promise.resolve().then(() => lp.encode.single(input))
    ).to.eventually.be.rejected.with.property('code', 'ERR_MSG_DATA_TOO_LONG')
    // ).to.eventually.be.rejectedWith(InvalidDataLengthError).and.have.property('code', 'ERR_MSG_DATA_TOO_LONG')
  })

I tried to use the same matchers to be slightly more expressive, but I have to wrap encode.single in a promise, does it eventually make sense?

There is an example of the pattern you should be able to use at https://github.com/libp2p/js-libp2p/blob/31a15a1483e0af0f9ede24de0a7f1d24bf9d408d/packages/kad-dht/test/record/selection.spec.ts#L19-L23

basically you can expect .to.throw() with whichever properties

@acul71
Copy link
Author

acul71 commented Jan 29, 2025

@SgtPooki @achingbrain Should be ready with enhanced test assertions with expressive matchers

@SgtPooki
Copy link

cc @alanshaw I don't have access to this repo and Alex is out this week

@achingbrain achingbrain merged commit 7a56aaf into alanshaw:master Feb 3, 2025
1 check passed
github-actions bot pushed a commit that referenced this pull request Feb 3, 2025
## [10.0.0](v9.1.1...v10.0.0) (2025-02-03)

### ⚠ BREAKING CHANGES

* outgoing messages must now be shorter than the `maxDataLength` option, or 4MB if no option is passed

### Bug Fixes

* throw InvalidDataLengthError when input exceeds MAX_DATA_LENGTH ([#136](#136)) ([7a56aaf](7a56aaf))
@github-actions
Copy link

github-actions bot commented Feb 3, 2025

🎉 This PR is included in version 10.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants