-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: provide IsMessageSizeTooLarge() function #2848
Closed
+69
−1
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really shouldn’t be something that we bake in. Checking the string value of the error is considered The Wrong Way™ to check error conditions. And once we provide this, the API is kind of stuck with it no matter what.
I think returning this as a
ConfigurationError
is itself faulty. We should be returning a type that allowserrors.Is(err, ErrMessageSizeTooLarge)
to cover all the functionality necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue encapsulating the string check as a private implementation detail of the library does the opposite of baking it in. e.g. currently users need to bake-in this logic (we have, as have others). Moving it to a function means that users won't need to update their logic each time this library chooses the change the error type that is returned (or changes the text in it).
The precise error returned for this condition has changed at least 3 times over the years so far:
I agree it's not great, but I'm not sure that there is an alterative to this change that doesn't change the error type returned for a 4th time. (Although I'd be supportive of a simple revert of #2628 which would return
ErrMessageSizeTooLarge
again).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it’s better than depending on raw strings copied into all our codebases, for sure. But I worry that it’s too much like putting wallpaper over a hole in the wall.
There will be a non-zero number of codebases that aren’t going to change their code to use this function, and just keep using the raw string, because “hey, if it ain’t broke, don’t fix it.” (Also, finding priority for maintenance work is almost always a struggle, since there’s a fairly ephemeral value to product.) And these people are still going to find themselves stuck out if/when the error text changes, and the breaking behavior change for these people will again be relatively silent until it causes the same critical/issue/alert/whatever that brought us all here.
🤷♀️ I’m just not sure… like I said, I don’t know if this improves the situation in any practical way. And in a year or two, someone will be looking at this code and wondering why we would ever double down on something much of Go considers an anti-pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have opened alternate PR #2851 and CCed you there. I prefer the approach in #2851 but the downside is that it changes the error type (again).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I kind of hope no one removed the
ErrMessageSizeTooLarge
handling right? 😰 Because this was still a value that the server could be returning, and one would likely want to handle the same as if the client refused to even send the message.