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

Feature: Throw errors with proper http code #89

Merged
merged 2 commits into from
Sep 25, 2022
Merged

Conversation

oscar60310
Copy link
Contributor

@oscar60310 oscar60310 commented Sep 24, 2022

Description

  1. Custom errors
    I've created 4 errors for us:
    /** Expected errors, which is caused by users */
    export class UserError extends VulcanError {
      constructor(message?: string, options?: VulcanErrorOptions) {
        super(message, {
          ...options,
          httpCode: options?.httpCode || 400,
          code: options?.code || 'vulcan.userError',
          exitCode: options?.exitCode || 2,
        });
      }
    }
    
    /** Unexpected errors, which is caused by internal issues */
    export class InternalError extends VulcanError {
      constructor(message?: string, options?: VulcanErrorOptions) {
        super(message, {
          ...options,
          httpCode: options?.httpCode || 500,
          code: options?.code || 'vulcan.internalError',
          exitCode: options?.exitCode || 3,
        });
      }
    }
    
    /** The configurations e.g. vulcan.yaml, user.yaml ... are incorrect */
    export class ConfigurationError extends InternalError {
      constructor(message?: string, options?: VulcanErrorOptions) {
        super(message, {
          ...options,
          code: options?.code || 'vulcan.configError',
        });
      }
    }
    
    /** Error from template syntax */
    export class TemplateError extends InternalError {
      constructor(message?: string, options?: VulcanErrorOptions) {
        super(message, {
          ...options,
          code: options?.code || 'vulcan.templateError',
        });
      }
    }
  2. Catch errors and set proper HTTP code and body.
  3. Corrected the order of middlewares.

Issue ticket number

closes #61

Additional Context

- add error handler middleware.
- fix the middleware order in order to get request id.
@oscar60310 oscar60310 requested a review from kokokuo September 24, 2022 11:03
@codecov-commenter
Copy link

Codecov Report

Base: 92.68% // Head: 92.60% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (155446f) compared to base (2c7888e).
Patch coverage: 79.89% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #89      +/-   ##
===========================================
- Coverage    92.68%   92.60%   -0.09%     
===========================================
  Files          257      259       +2     
  Lines         3720     3798      +78     
  Branches       462      481      +19     
===========================================
+ Hits          3448     3517      +69     
  Misses         185      185              
- Partials        87       96       +9     
Flag Coverage Δ
build 94.90% <85.71%> (+0.01%) ⬆️
cli 89.38% <ø> (ø)
core 92.99% <81.48%> (-0.24%) ⬇️
extension-dbt 97.43% <ø> (ø)
extension-debug-tools 98.11% <ø> (ø)
extension-driver-duckdb 100.00% <100.00%> (ø)
extension-driver-pg 94.87% <0.00%> (ø)
integration-testing 96.15% <ø> (ø)
serve 90.38% <76.66%> (+0.07%) ⬆️
test-utility ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ib/schema-parser/schema-reader/fileSchemaReader.ts 91.30% <0.00%> (ø)
...ckages/extension-driver-pg/src/lib/pgDataSource.ts 93.33% <0.00%> (ø)
...s/serve/src/lib/response-formatter/csvFormatter.ts 81.57% <0.00%> (ø)
.../serve/src/lib/response-formatter/jsonFormatter.ts 80.00% <0.00%> (ø)
...rc/lib/route/route-component/requestTransformer.ts 91.30% <0.00%> (ø)
packages/serve/src/lib/server.ts 72.91% <0.00%> (ø)
...ore/src/lib/data-query/builder/dataQueryBuilder.ts 88.84% <33.33%> (+0.04%) ⬆️
packages/build/src/containers/container.ts 63.15% <50.00%> (ø)
.../src/lib/artifact-builder/vulcanArtifactBuilder.ts 94.11% <50.00%> (+0.36%) ⬆️
...built-in-extensions/query-builder/parameterizer.ts 88.23% <50.00%> (+0.73%) ⬆️
... and 56 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kokokuo
Copy link
Contributor

kokokuo commented Sep 24, 2022

I discover one thing and would like to check does it was missed, The CLI init files user.yaml seems not to have sample in #86 code href . Should it also need to add sample ?

@oscar60310
Copy link
Contributor Author

I discover one thing and would like to check does it was missed, The CLI init files user.yaml seems not to have sample in #86 code href . Should it also need to add sample ?

Thanks for pointing out, that property is optional, builder won't send sample queries without setting it. So I think it's ok to leave it blank in init templates.

@kokokuo
Copy link
Contributor

kokokuo commented Sep 24, 2022

I discover one thing and would like to check does it was missed, The CLI init files user.yaml seems not to have sample in #86 code href . Should it also need to add sample ?

Thanks for pointing out, that property is optional, builder won't send sample queries without setting it. So I think it's ok to leave it blank in init templates.

If not set it, does the api spec still generated but not appear the response info in the api document? ( I would like to check for writing it to the our vulcansql document )

@oscar60310
Copy link
Contributor Author

I discover one thing and would like to check does it was missed, The CLI init files user.yaml seems not to have sample in #86 code href . Should it also need to add sample ?

Thanks for pointing out, that property is optional, builder won't send sample queries without setting it. So I think it's ok to leave it blank in init templates.

If not set it, does the api spec still generated but not appear the response info in the api document? ( I would like to check for writing it to the our vulcansql document )

Yes, we'll get an empty response {} in the document.

@kokokuo
Copy link
Contributor

kokokuo commented Sep 24, 2022

I discover one thing and would like to check does it was missed, The CLI init files user.yaml seems not to have sample in #86 code href . Should it also need to add sample ?

Thanks for pointing out, that property is optional, builder won't send sample queries without setting it. So I think it's ok to leave it blank in init templates.

If not set it, does the api spec still generated but not appear the response info in the api document? ( I would like to check for writing it to the our vulcansql document )

Yes, we'll get an empty response {} in the document.

Thank you!

Copy link
Contributor

@kokokuo kokokuo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 👍 👍
Thanks so much for helping me to develop the feature, really appreciated

@kokokuo kokokuo merged commit 4cbc840 into develop Sep 25, 2022
@kokokuo kokokuo deleted the feature/custom-errors branch September 25, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server error handling for restful APIs
3 participants