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

feat(presto-client): return back the whole error object #17

Merged
merged 3 commits into from
Jan 18, 2024

Conversation

alonsovb
Copy link
Contributor

Changes

Resolves #11

  • When a query fails, the client now returns the whole error information
  • Improved the typing of the Presto Error

@alonsovb alonsovb added the feature New feature or request label Jan 17, 2024
@alonsovb alonsovb requested review from yhwang and ManfredA January 17, 2024 17:29
@alonsovb alonsovb self-assigned this Jan 17, 2024
@jorgeramirezamora jorgeramirezamora changed the title feat(client): return back the whole error object feat(presto-client): return back the whole error object Jan 17, 2024
Copy link
Contributor

@jorgeramirezamora jorgeramirezamora left a comment

Choose a reason for hiding this comment

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

I am wondering, Does it worth adding an example of an endpoint which fails to the Nest.js example application?

@alonsovb
Copy link
Contributor Author

Good idea @jorgeramirezamora , I'll include an example of using the returned error 👍

@yhwang
Copy link
Member

yhwang commented Jan 17, 2024

I believe the error handling is deviating from the documentation. But the doc update could come later :-)
+1 for the test case.

LGTM

@alonsovb
Copy link
Contributor Author

I created an interface for PrestoError but I'm now thinking that it should be a class, so that you can do

catch (error) {
  if (error instanceof PrestoError) { ... }
}

so I'll take some time to refactor that

Comment on lines +38 to +46
if (error instanceof PrestoError) {
/* eslint-disable no-console */
// The error here contains all the information returned by Presto directly
console.info(error.message)
console.info(error.name)
console.info(error.errorCode)
console.info(error.stack)
console.info(error.failureInfo.type)
return 'A Presto error ocurred, please check the service logs'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this example usage in the app to better showcase how to use the returned error

try {
return await this.appService.getDataWithError()
} catch (err) {
console.error(err)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this would be an error other than PrestoError because the PrestoError is handled in the getDataWithError and no rethrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I guess I'm using the service file as the showcase for the error
I don't know if we should also do something else here in the controller

Copy link
Contributor

@jorgeramirezamora jorgeramirezamora left a comment

Choose a reason for hiding this comment

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

LGTM!

const prestoQuery = await client.query(query)
const results = prestoQuery.data
} catch (error) {
if (error instanceof PrestoError) {
Copy link
Member

@yhwang yhwang Jan 17, 2024

Choose a reason for hiding this comment

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

This is nice!

@alonsovb alonsovb merged commit 8221bdc into main Jan 18, 2024
4 checks passed
@alonsovb alonsovb deleted the feature/report-error branch January 18, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If a query fails, return the error back
3 participants