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

More specific Errors #611

Closed
chrisbrantley opened this issue Jun 20, 2020 · 3 comments
Closed

More specific Errors #611

chrisbrantley opened this issue Jun 20, 2020 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@chrisbrantley
Copy link

chrisbrantley commented Jun 20, 2020

Is your feature request related to a problem? Please describe.

MikroOrm currently uses ValidationError as a catch-all for many different types of errors. The same ValidationError is thrown when an entity is not found (from findOrFail) or if an entity is referenced that is not registered.

I would argue that these are very different types of errors. Referencing an entity that isn't managed is a fatal error that represents a fundamental issue with the application. Not finding an entity in the database is a runtime error that may be expected if using user input to retrieve the entity.

Using the same class for all of these errors, with the only difference being the message string, makes it difficult to write error handling code. For example, I may want to catch "NotFound" errors and provide a friendly message to the user. But something like "wrongPropertyType" is a bug and I'd want to log it and give the user a generic "SERVER ERROR" message as to not leak details about the application via the API.

With the current error structure I can't use typical strategies like checking the instanceof an error and handling it accordingly. The best I can do is hacky fuzzy string matches on the error message string.

Describe the solution you'd like
My preference would be to use specific error classes for specific errors. ValidationError has lots of static factory methods for various error types. That indicates to me that you could turn each of those static methods into subclasses specific to that error.

I'd also differentiate between runtime errors like "NotFound" and more fatal errors like problems like mismatching schemas/types or entities not being registered.

Describe alternatives you've considered
If for some reason there is resistance to having more specialized error classes can we at least have a specific code set on the ValidationError to make it easier to determine what type of error it is? This would be less ideal than using subclasses but at least it would allow me to handle errors in a more robust way than checking that the message contains "not found".

Additional context
For my specific use-case: I'm using MikroORM to drive a GraphQL API. I'd like to use findOneOrFail when retrieving entities from user input. If someone provides an ID that doesn't exist that's not a bug in my application, it's a user error and should be handled as such. But right now I can't easily differentiate between a "NotFound" error or a bug like I changed a property name and forgot to update my retrieval code.

I could forgo findOneOrFail and just use findOne, guard against null values, and throw my own error... but that's a lot of boilerplate code.

@chrisbrantley chrisbrantley added the enhancement New feature or request label Jun 20, 2020
@B4nan
Copy link
Member

B4nan commented Jun 20, 2020

FYI this is implemented for runtime errors in v4 already: #539

I don't see much value in dividing everything, but I am open for discussion for concrete errors.

Sure we can apply this to the default findOneOrFail error, note that you can also specify custom error handler for that, both locally and globally.

https://mikro-orm.io/docs/entity-manager/#handling-not-found-entities

@chrisbrantley
Copy link
Author

I don't see much value in dividing everything, but I am open for discussion for concrete errors.

Sure we can apply this to the default findOneOrFail error, note that you can also specify custom error handler for that, both locally and globally.

I agree. Perhaps I was a little over-zealous in wanting concrete errors for everything. Honestly just a default NotFoundError would be great.

I was not aware of the custom error handling for FindOneOrFail so thank you for that.

@B4nan
Copy link
Member

B4nan commented Jun 21, 2020

Closing as implemented in v4 via 08f508f

@B4nan B4nan closed this as completed Jun 21, 2020
@B4nan B4nan added this to the 4.0 milestone Jun 21, 2020
@B4nan B4nan mentioned this issue Jun 21, 2020
46 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants