You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 PR attempts to improve the error handling introduced in #3607.
About the changes
tl;dr:
Make UnleashError constructor protected
Make all custom errors inherit from UnleashError.
Add tests to ensure that all special error cases include their relevant data
Remove PasswordMismatchError and BadRequestError. These don't exist.
Add a few new error types: ContentTypeError, NotImplementedError, UnauthorizedError
Remove the ...rest parameter from error constructor
Add an unexported GenericUnleashError class
Move OpenAPI conversion function to BadDataError clas
Remove explicit Error.captureStackTrace. This is done automatically.
Extract getPropFromString function and add tests
In a more verbose fashion
The main thing is that all our internal errors now inherit fromUnleashError. This allows us to simplify the UnleashError constructor and error handling in general while still giving us the extra benefits we added to that class. However, it does also mean that I've had to update all existing error classes.
The constructor for UnleashError is now protected and all places that called that constructor directly have been updated. Because the base error isn't available anymore, I've added three new errors to cover use cases that we didn't already have covered: NotImplementedError, UnauthorizedError, ContentTypeError. This is to stay consistent in how we report errors to the user.
There is also an internal class, GenericUnleashError that inherits from the base error. This class is only used in conversions for cases where we don't know what the error is. It is not exported.
In making all the errors inherit, I've also removed the ...rest parameter from the UnleashError constructor. We don't need this anymore.
Following on from the fixes with missing properties in #3638, I have added tests for all errors that contain extra data.
Some of the error names that were originally used when creating the list don't exist in the backend. BadRequestError and PasswordMismatchError have been removed.
The BadDataError class now contains the conversion code for OpenAPI validation errors. In doing so, I extracted and tested the getPropFromString function.
Main files
Due to the nature of the changes, there's a lot of files to look at. So to make it easier to know where to turn your attention:
The changes in api-error.ts contain the main changes: protected constructor, removal of OpenAPI conversion (moved into BadDataError.
api-error.test.ts contains tests to make sure that errors work as expected.
Aside from get-prop-from-string.ts and the tests, everything else is just the required updates to go through with the changes.
Discussion points
I've gone for inheritance of the Error type over composition. This is in large part because throwing actual Error instances instead of just objects is preferable (because they collect stack traces, for instance). However, it's quite possible that we could solve the same thing in a more elegant fashion using composition.
For later / suggestions for further improvements
The api-error files still contain a lot of code. I think it might be beneficial to break each Error into a separate folder that includes the error, its tests, and its schema (if required). It would help decouple it a bit.
We don't currently expose the schema anywhere, so it's not available in the openapi spec. We should look at exposing it too.
Finally, it would be good to go through each individual error message and update each one to be as helpful as possible.
After enabling strictNullChecks this PR would be increasing the number of null check errors from 294 to 296. Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.
After enabling strictNullChecks this PR would be increasing the number of null check errors from 294 to 296. Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.
After enabling strictNullChecks this PR would be increasing the number of null check errors from 294 to 296. Make sure your branch is up-to-date with main and check the diff in the console output to pinpoint the offending files.
The reason will be displayed to describe this comment to others. Learn more.
I started reviewing the first two points from the TL;DR and found something that should be addressed before I continue the review so I'm submitting a request changes proposal.
This PR adds the missing serialization of the AuthenticationRequired
response back in. It was mistakenly removed in #3633.
This PR also adds another test to verify that it the options property is
present.
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.
thomasheartman
force-pushed
the
chore/clean-errors
branch
from
This PR attempts to improve the error handling introduced in #3607.
About the changes
tl;dr:
UnleashErrorconstructor protectedUnleashError.PasswordMismatchErrorandBadRequestError. These don't exist.ContentTypeError,NotImplementedError,UnauthorizedError...restparameter from error constructorGenericUnleashErrorclassBadDataErrorclasError.captureStackTrace. This is done automatically.getPropFromStringfunction and add testsIn a more verbose fashion
The main thing is that all our internal errors now inherit from
UnleashError. This allows us to simplify theUnleashErrorconstructor and error handling in general while still giving us the extra benefits we added to that class. However, it does also mean that I've had to update all existing error classes.The constructor for
UnleashErroris now protected and all places that called that constructor directly have been updated. Because the base error isn't available anymore, I've added three new errors to cover use cases that we didn't already have covered:NotImplementedError,UnauthorizedError,ContentTypeError. This is to stay consistent in how we report errors to the user.There is also an internal class,
GenericUnleashErrorthat inherits from the base error. This class is only used in conversions for cases where we don't know what the error is. It is not exported.In making all the errors inherit, I've also removed the
...restparameter from theUnleashErrorconstructor. We don't need this anymore.Following on from the fixes with missing properties in #3638, I have added tests for all errors that contain extra data.
Some of the error names that were originally used when creating the list don't exist in the backend.
BadRequestErrorandPasswordMismatchErrorhave been removed.The
BadDataErrorclass now contains the conversion code for OpenAPI validation errors. In doing so, I extracted and tested thegetPropFromStringfunction.Main files
Due to the nature of the changes, there's a lot of files to look at. So to make it easier to know where to turn your attention:
The changes in
api-error.tscontain the main changes: protected constructor, removal of OpenAPI conversion (moved intoBadDataError.api-error.test.tscontains tests to make sure that errors work as expected.Aside from
get-prop-from-string.tsand the tests, everything else is just the required updates to go through with the changes.Discussion points
I've gone for inheritance of the Error type over composition. This is in large part because throwing actual Error instances instead of just objects is preferable (because they collect stack traces, for instance). However, it's quite possible that we could solve the same thing in a more elegant fashion using composition.
For later / suggestions for further improvements
The
api-errorfiles still contain a lot of code. I think it might be beneficial to break each Error into a separate folder that includes the error, its tests, and its schema (if required). It would help decouple it a bit.We don't currently expose the schema anywhere, so it's not available in the openapi spec. We should look at exposing it too.
Finally, it would be good to go through each individual error message and update each one to be as helpful as possible.