-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: return ErrMessageSizeTooLarge when message is too large #2851
Conversation
For most of this library's existence it has returned ErrMessageSizeTooLarge when the message exceeded the configured size. For a short period in 2019 this error was renamed (IBM#1218) but shortly revered back (IBM#1262). Later in 2023 this error was changed to a ConfigurationError (IBM#2628) to fix IBM#2137, however this has caused issues with clients who rely on the previous error code being distinct from other ConfigurationError conditions (IBM#2655). This commit reverts to previous behaviour, and adds a test to pickup if this changes again in the future. Signed-off-by: Adam Eijdenberg <[email protected]>
@puellanivis - this might be a better approach? I know you like the @shermanCRL , @dnwe and @hindessm who reported #2137 and made #2628 which this effectively reverts who may be interested in this discussion and have comments. An altnernate PR which doesn't change the current types returns is proposed in #2848. Personally I prefer this approach in this PR (ie restore previous behaviour), but that unresolves #2137 which may or may not be OK. |
I mean, one of the first tests |
To be more clear, I’m more behind this revert, rather than adding a new function to test an error string “safely”. |
Signed-off-by: Adam Eijdenberg <[email protected]>
caf4a47
to
f474e6d
Compare
Pushed additional commit to fix the lint. Force-push because I forgot the DCO the first time. |
It's not clear to me why that one test failed in one env. Any chance that's a flaky test? |
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
For most of this library's existence it has returned ErrMessageSizeTooLarge when the message exceeded the configured size.
For a short period in 2019 this error was renamed (#1218) but shortly revered back (#1262). Later in 2023 this error was changed to a ConfigurationError (#2628) to fix #2137, however this has caused issues with clients who rely on the previous error code being distinct from other ConfigurationError conditions (#2655).
This commit reverts to previous behaviour, and adds a test to pickup if this changes again in the future.