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

(wip): GRPC exception and mapping #1957

Closed
wants to merge 7 commits into from

Conversation

tychota
Copy link
Contributor

@tychota tychota commented Apr 8, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1953

What is the new behavior?

After this PR you will have:

  • HTTP CanceledException
  • HTTP TooManyRequestsException
  • GRPC exceptions
  • A mapping of the closest http exception that match the Grpc Exception

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Still WIP.

Missing:

  • GrpcExceptionFiler that would catch all these exceptions and serialize to gRPC error objects => I will try but probably will need help.
  • test (?) => Shall i test Grpc Exceptions ?
  • doc

@tychota tychota changed the title (wip): GRPC exception mapping (wip): GRPC exception and mapping Apr 8, 2019
@tychota tychota force-pushed the feat/grpc-exception-mapping branch from ccc9476 to e1cd93d Compare April 8, 2019 17:05
@tychota
Copy link
Contributor Author

tychota commented Apr 8, 2019

@kamilmysliwiec: This is a start. Let me know what you think !

@coveralls
Copy link

coveralls commented Apr 8, 2019

Pull Request Test Coverage Report for Build 3536

  • 20 of 21 (95.24%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 95.151%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/common/utils/shared.utils.ts 1 2 50.0%
Totals Coverage Status
Change from base Build 3532: 0.0005%
Covered Lines: 3552
Relevant Lines: 3733

💛 - Coveralls

@tychota
Copy link
Contributor Author

tychota commented Apr 8, 2019

Is that expected

image

@kamilmysliwiec
Copy link
Member

@tychota I have manually triggered the build and it passed now :)

@tychota
Copy link
Contributor Author

tychota commented Apr 9, 2019

I left a comment here explaining the "Idem shall be an available exception ?"
#1957 (comment)

The question behind is that the closest HTTP Exception that match GrpcStatus.CANCELLED is 499, which is introduce by nginx : http://lxr.nginx.org/source/src/http/ngx_http_request.h?v=nginx-1.51.11#0127

I'd say that we probably shouldn't introduce non-standard HTTP status codes. Better hardcode it inside a corresponding gRPC expection.

It looks smart to "polute" the main http exception status but It looks weird to have a single http exception defined in Grpc.

I mean for the user that want the exception in an handler it will look like this:

import { ConflictException, HttpException } from '@nestjs/common';
// That looks a bit weird for me but I can leave with it
import { CanceledException, GrpcException } from '@nestjs/microservice/grpc';

///....

Before I create GrpcExceptionFiler, to be clear about what vision you have ?

(from #1953 (comment))
GrpcExceptionFiler that would catch all these exceptions and serialize to gRPC error objects

You mean:

  1. editing https://github.com/nestjs/nest/blob/master/packages/microservices/exceptions/rpc-exceptions-handler.ts or even https://github.com/nestjs/nest/blob/master/packages/microservices/exceptions/base-rpc-exception-filter.ts
  2. (or) creating another stack of filter for gRPC: GrpcProxy -> GrpcContext -> GrpcExceptionHandler
  3. (or) just create a GrpcExceptionFiler and let the user use it (global filter or decorator)

@kamilmysliwiec
Copy link
Member

(or) just create a GrpcExceptionFiler and let the user use it (global filter or decorator)

Exactly :) I also answered to your comment about the canceled error.

@tychota
Copy link
Contributor Author

tychota commented Apr 10, 2019

Exactly :)

Thanks.

I also answered to your comment about the canceled error.

Thanks also, lets discuss it in the thread.

@tychota tychota force-pushed the feat/grpc-exception-mapping branch from e1cd93d to d165795 Compare April 10, 2019 17:46
@tychota
Copy link
Contributor Author

tychota commented Apr 10, 2019

@kamilmysliwiec

  • e2f3a34: I've remove the non standard exception
  • 572eb21: I've remove the non standard exception import and use a 503 instead
  • 84ff66a: simple refacto, is it the good place for isError
  • 2ec60ce: i tried to run integration locally with sh ./scripts/run-integration.sh but i had to run npm install in each subdir of the integration (for instance for @nest/graphqlthat was extractedand have still an issue. I still commited the diff after npm install`
Logs (click to expand)

➜ nest git:(feat/grpc-exception-mapping) ✗ sh ./scripts/run-integration.sh
integration_rabbit_1 is up-to-date
integration_mysql_1 is up-to-date
test-redis is up-to-date
test-mqtt is up-to-date
test-nats is up-to-date
integration_mongodb_1 is up-to-date
npm info it worked if it ends with ok
npm verb cli [ '/Users/tychotatitscheff/n/bin/node',
npm verb cli '/Users/tychotatitscheff/n/bin/npm',
npm verb cli 'run',
npm verb cli 'integration-test' ]
npm info using [email protected]
npm info using [email protected]
npm verb run-script [ 'preintegration-test',
npm verb run-script 'integration-test',
npm verb run-script 'postintegration-test' ]
npm info lifecycle [email protected]preintegration-test: [email protected]
npm info lifecycle [email protected]
integration-test: [email protected]

[email protected] integration-test /Users/tychotatitscheff/Code/open-source/nest
mocha integration/**/*.spec.ts --reporter spec --require ts-node/register --require 'node_modules/reflect-metadata/Reflect.js'

/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:338
throw new TypeError(${relative(cwd, fileName)}: Emit skipped)
^
TypeError: packages/microservices/index.js: Emit skipped
at getOutput (/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:338:15)
at Object.compile (/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:368:11)
at Module.m._compile (/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:414:43)
at Module._extensions..js (internal/modules/cjs/loader.js:745:10)
at Object.require.extensions.(anonymous function) [as .js] (/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:417:12)
at Module.load (internal/modules/cjs/loader.js:626:32)
at tryModuleLoad (internal/modules/cjs/loader.js:566:12)
at Function.Module._load (internal/modules/cjs/loader.js:558:3)
at Module.require (internal/modules/cjs/loader.js:663:17)
at require (internal/modules/cjs/helpers.js:20:18)
at Object. (/Users/tychotatitscheff/Code/open-source/nest/integration/microservices/e2e/broadcast-mqtt.spec.ts:2:1)
at Module._compile (internal/modules/cjs/loader.js:734:30)
at Module.m._compile (/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:414:23)
at Module._extensions..js (internal/modules/cjs/loader.js:745:10)
at Object.require.extensions.(anonymous function) [as .ts] (/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:417:12)
at Module.load (internal/modules/cjs/loader.js:626:32)
at tryModuleLoad (internal/modules/cjs/loader.js:566:12)
at Function.Module._load (internal/modules/cjs/loader.js:558:3)
at Module.require (internal/modules/cjs/loader.js:663:17)
at require (internal/modules/cjs/helpers.js:20:18)
at /Users/tychotatitscheff/Code/open-source/nest/node_modules/mocha/lib/mocha.js:231:27
at Array.forEach ()
at Mocha.loadFiles (/Users/tychotatitscheff/Code/open-source/nest/node_modules/mocha/lib/mocha.js:228:14)
at Mocha.run (/Users/tychotatitscheff/Code/open-source/nest/node_modules/mocha/lib/mocha.js:514:10)
at Object. (/Users/tychotatitscheff/Code/open-source/nest/node_modules/mocha/bin/_mocha:480:18)
at Module._compile (internal/modules/cjs/loader.js:734:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:745:10)
at Module.load (internal/modules/cjs/loader.js:626:32)
at tryModuleLoad (internal/modules/cjs/loader.js:566:12)
at Function.Module._load (internal/modules/cjs/loader.js:558:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:797:12)
at executeUserCode (internal/bootstrap/node.js:526:15)
at startMainThreadExecution (internal/bootstrap/node.js:439:3)
npm verb lifecycle [email protected]integration-test: unsafe-perm in lifecycle true
npm verb lifecycle [email protected]
integration-test: PATH: /Users/tychotatitscheff/n/lib/node_modules/npm/node_modules/npm-lifecycle/node-gyp-bin:/Users/tychotatitscheff/Code/open-source/nest/node_modules/.bin:/Users/tychotatitscheff/.yarn/bin:/Users/tychotatitscheff/.config/yarn/global/node_modules/.bin:/usr/local/opt/qt/bin:/Users/tychotatitscheff/google-cloud-sdk/bin:/usr/local/opt/llvm/bin:/Users/tychotatitscheff/git-subrepo/lib:/Users/tychotatitscheff/.bin:/anaconda3/bin:/Users/tychotatitscheff/.cargo/bin:/Users/tychotatitscheff/.rvm/gems/ruby-2.4.0/bin:/Users/tychotatitscheff/.rvm/gems/ruby-2.4.0@global/bin:/Users/tychotatitscheff/.rvm/rubies/ruby-2.4.0/bin:/Users/tychotatitscheff/Library/Python/3.6/bin/:/Users/tychotatitscheff/.cargo/bin:/Users/tychotatitscheff/Library/Android/sdk/platform-tools:/Users/tychotatitscheff/Library/Android/sdk/tools:/bin:/bin:/bin:/usr/local/bin:/Users/tychotatitscheff/.cargo/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Library/TeX/texbin:/opt/X11/bin:/Applications/Wireshark.app/Contents/MacOS:/Users/tychotatitscheff/.go/bin:/Users/tychotatitscheff/Library/Android/sdk/tools/bin:/Users/tychotatitscheff/Library/Android/sdk/tools:/Users/tychotatitscheff/Library/Android/sdk/platform-tools:/Users/tychota/.cargo/bin/:/Users/tychota/bin/FDK/Tools/osx:/Users/tychotatitscheff/.rvm/bin:/Users/tychotatitscheff/n/bin:/Users/tychotatitscheff/.rvm/bin:/Users/tychotatitscheff/.fabric8/bin:/usr/local/opt/fzf/bin
npm verb lifecycle [email protected]integration-test: CWD: /Users/tychotatitscheff/Code/open-source/nest
npm info lifecycle [email protected]
integration-test: Failed to exec integration-test script
npm verb stack Error: [email protected] integration-test: mocha integration/**/*.spec.ts --reporter spec --require ts-node/register --require 'node_modules/reflect-metadata/Reflect.js'
npm verb stack Exit status 1
npm verb stack at EventEmitter. (/Users/tychotatitscheff/n/lib/node_modules/npm/node_modules/npm-lifecycle/index.js:301:16)
npm verb stack at EventEmitter.emit (events.js:197:13)
npm verb stack at ChildProcess. (/Users/tychotatitscheff/n/lib/node_modules/npm/node_modules/npm-lifecycle/lib/spawn.js:55:14)
npm verb stack at ChildProcess.emit (events.js:197:13)
npm verb stack at maybeClose (internal/child_process.js:978:16)
npm verb stack at Process.ChildProcess._handle.onexit (internal/child_process.js:265:5)
npm verb pkgid [email protected]
npm verb cwd /Users/tychotatitscheff/Code/open-source/nest/integration
npm verb Darwin 18.2.0
npm verb argv "/Users/tychotatitscheff/n/bin/node" "/Users/tychotatitscheff/n/bin/npm" "run" "integration-test"
npm verb node v11.9.0
npm verb npm v6.5.0
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] integration-test: mocha integration/**/*.spec.ts --reporter spec --require ts-node/register --require 'node_modules/reflect-metadata/Reflect.js'
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] integration-test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm verb exit [ 1, true ]
npm timing npm Completed in 50604ms

npm ERR! A complete log of this run can be found in:
npm ERR! /Users/tychotatitscheff/.npm/_logs/2019-04-10T17_36_58_971Z-debug.log

  • 5dd17d4 my try at ExceptionFilter and an integration test. Is it globally how you see it ?
    In peculiar:
    • Shall I @Catch() all (other exception handler don't but I guess it is because they are injected by default and don't require the @UseFilter().
    • Shall I instantiate the logger here (I have the feeling that it will be logged twice) or extend BaseRpcExceptionFilter and use this.catch() (but then I can't prevent the underlying Base to log as error Handled GrpcException).
    • Is integration test enough ? or shall I unit test it ? (if so, how ?)

@tychota tychota force-pushed the feat/grpc-exception-mapping branch from d165795 to 5dd17d4 Compare April 10, 2019 18:06
@tychota
Copy link
Contributor Author

tychota commented Apr 10, 2019

(I messed up with rebase, post above edited with new commits)

@kamilmysliwiec
Copy link
Member

Is it globally how you see it ?

Yes :)

Shall I @catch() all (other exception handler don't but I guess it is because they are injected by default and don't require the @UseFilter().

Exactly

Shall I instantiate the logger here (I have the feeling that it will be logged twice) or extend BaseRpcExceptionFilter and use this.catch() (but then I can't prevent the underlying Base to log as error Handled GrpcException).

I think that the current solution is better (no inheritance)

Is integration test enough ? or shall I unit test it ? (if so, how ?)

Integration test is great. Regarding unit tests, I'd say that we should add 2 tests that would check thrown error (from catch() method) depending on the exception type (UNKNOWN when !== GrpcException, and otherwise [..])

@tychota
Copy link
Contributor Author

tychota commented Apr 19, 2019

(I had limited availability last week and I will still be pretty busy next week but I have not abandoned this PR).

I think I should wait on this (https://github.com/nestjs/nest/pull/1568/files) to be merged, as it it closer to DONE, and I will modify the same files.


Regarding unit tests, I'd say that we should add 2 tests that would check thrown error (from catch() method) depending on the exception type (UNKNOWN when !== GrpcException, and otherwise [..])

Will do


I have a big question for the last step however.

In my side project, I have done https://github.com/tychota/shitake/blob/master/packages/microservice-auth/infrastructure/api/auth.proto#L20-L24 to send back the status which is suboptimal (Why force users to adopt this peculiar syntax, looks the way to go).

(cc @anton-alation, since he seems to have pretty good exp with GRPC)

The right way to do will be to send from microservice to gateway the status in grpc.Metadata !?

I have however no idea how to do that.

AFAIK, it should be modifying:

But not sure which. I'm a bit lost. I don't see exactly how/in which file the status is send back when the an error is thrown.

I tried to go through some PRs, commits and the codebase but I think I would like so code throughput to fill the missing step (not super urgent):

Understanding this in a clearer way will help me producing a good modification on first try (rather that hacking bad stuff).

@anton-alation
Copy link
Contributor

In my side project, I have done https://github.com/tychota/shitake/blob/master/packages/microservice-auth/infrastructure/api/auth.proto#L20-L24 to send back the status which is suboptimal (Why force users to adopt this peculiar syntax, looks the way to go).

Maybe it isn't easy to catch a context on the first try but as far as I got what you described is: You really trying to bring messaging closer to REST conceptions, while those two different areas with a really different mindset (streams for example).

Why are we trying to use GRPC? Probably best answer: we looking to share interfaces and provide every client ability to decide which platform they want to use while having a network layer almost 'for free'.

As for the main conception of relying on some interface, we can assume that person using the interface will require no more no less knowledge than the interface describing in its definition files. While trying to introduce metadata interaction is almost equal to the habit of enriching HTTP headers in non-standard ways just to not to sacrifice precious JSON or another object with certain metadata.

I thinking that there is nothing wrong in introducing clear and clean and only one type of object of status per any message (you can derive the same Status for every message type rather than placing new Status in every message) and that actually will make your interface clear on exact behavior which someone working with that one — can expect.

With Proto + GRPC we really no need to share anything more than set of proto files. And there best to be almost close to none explanations on any side-effects of an API, especially if we say will be using Go or CPP connecting to your Nest backend.

Just IMHO ;)

@tychota
Copy link
Contributor Author

tychota commented Apr 24, 2019

Hi,

still won't have time to continue the dev on this this week.

--

Why are we trying to use GRPC? Probably best answer: we looking to share interfaces and provide every client ability to decide which platform they want to use while having a network layer almost 'for free'.

@anton-alation thanks a lot for this comment and I think I agree with the general idea that GRPC strength is expressiveness.

As for the main conception of relying on some interface, we can assume that person using the interface will require no more no less knowledge than the interface describing in its definition files. While trying to introduce metadata interaction is almost equal to the habit of enriching HTTP headers in non-standard ways just to not to sacrifice precious JSON or another object with certain metadata.

However I think this isn't how big implementation are implemented. After looking in the spec, I found: https://grpc.io/grpc/core/md_doc__p_r_o_t_o_c_o_l-_h_t_t_p2.html

image

and

image

I found that in the implementation confirming that it goes for headers : https://github.com/grpc/grpc-node/blob/04395a0bc88a64aa67f1c299ad6d1504b6af32bc/packages/grpc-js/src/metadata-status-filter.ts#L24-L46

That still think to be an hot topic : grpc/grpc-web#399

Maybe it isn't easy to catch a context on the first try but as far as I got what you described is: You really trying to bring messaging closer to REST conceptions, while those two different areas with a really different mindset (streams for example).

The problem I try to solve is the following one.
I have a gateway that implement a RESTful Api and a few microservices using gRPC.
I want to profit from gRPC standard errors (https://cloud.google.com/apis/design/errors?hl=en#error_codes) so it is easy to differentiate error from exception.
Eg I want to return an error that "User already exist" to denote a conflict.
Lastly, I want the gateway to be smart so it can parse the gRPC error and by default react in a smart way. For instance return a 409 if there was a conflict.

https://cloud.google.com/apis/design/errors?hl=en#http_mapping

Tl;Dr

I thinking that there is nothing wrong in introducing clear and clean and only one type of object of status per any message

I agree on the principe but I have fear that:

  • other implementation aren't doing that
  • it is painful to write status in every message because it makes simple flat message nested and for me the status by it self looks more something that is a metadata than the data itself,
  • I would like that nest don't return GRPC unknown by default (and 500 when converted to gRPC) so it is easier for user to implement Gateway with HTTP talking to gRPC

I'm now a bit lost, would like to have your point of view after the link I send.

@anton-alation
Copy link
Contributor

anton-alation commented Apr 24, 2019

@tychota I read the answer and it's had a lot of good details. However, everyone is busy to dive in full into those, so let me ask it this way (abstracted a bit but hopefully understandable):

Would this pattern work for you or not so?

@GrpcMethod("ns.Service")
public async create(data: any): Promise<Observable<any>> {
    if (data.user.length === 0) {
        throw new RpcException({
            code: 400,
            details: "User field required to be of non-zero length for User",
        });
    }
    return of({
        user: "test",
    })
}

And on the client something like (I am here expecting Observable, but can be jsut err check properly wrapped)

const result = service.create({
       user: "",
}).toPromise();

try {
      return await result;
} catch (e) {
      if (e.code === 400)
          // React on malformed user
          console.log("Error statement", e.details);
      else
          // React on other errors
          console.log("You don't wanna know what happened here...");
}

If no exception === 200
If exception -> read the code

@kamilmysliwiec
Copy link
Member

ping 🏓

@tychota
Copy link
Contributor Author

tychota commented Jul 1, 2019

Hello @kamilmysliwiec

I think the discussion was left in progress and we will need some help to agree on what to do.

@anton-alation was not found of using headers to pass status at first (#1957 (comment)) but then proposed something closer to my initial idea (#1957 (comment)).

@kamilmysliwiec, do you agree with this design?

If so, #1957 (comment) (end of comment) applies. I don't see what to modify to make sure that the correct status is passed from microservice to client (now I think it is "UNKNOWN").

@kamilmysliwiec
Copy link
Member

So what I can say for now is that I'm definitely against forcing everyone to add Status message type to their proto definitions and passing this property within an actual response. I don't have enough time to dive into the gRPC sdk api now, but isn't this answer still up-to-date https://stackoverflow.com/a/42831446/6696689? Perhaps, we could just pass status property down to the gRPC library, and then, read this information from the client.

@tychota
Copy link
Contributor Author

tychota commented Jul 1, 2019

Nice to know we are on the same page.

I will:

  • rebase this week (as there are conflicts)
  • modify the callback to try to pass the status
  • create a GrpcExceptionFiler that would catch all these exceptions and serialize to gRPC error objects

Then we will be good to go.

@tychota tychota force-pushed the feat/grpc-exception-mapping branch from 5dd17d4 to 28c2a04 Compare July 8, 2019 08:39
@tychota
Copy link
Contributor Author

tychota commented Jul 8, 2019

I did rebase it. But I got:

image

Anything broken on my side or that is expected?

@tychota
Copy link
Contributor Author

tychota commented Jul 8, 2019

Well I did manage to pass the error to Grpc client:

image

(see [Nest] 7999 - 07/08/2019, 12:20 PM [ExceptionsHandler] 10 ABORTED: test +80618ms and not unknown)

but I'm not sure the way I did it is what you expect @kamilmysliwiec

image

I also try to do a GRPCExceptionFilter but I don't know how to use it:

image

Can you help me so I can go forward, @kamilmysliwiec?

@tychota tychota force-pushed the feat/grpc-exception-mapping branch from aa078f5 to f6fdf2d Compare July 8, 2019 12:31
@tychota tychota force-pushed the feat/grpc-exception-mapping branch from f6fdf2d to 9bd91f3 Compare July 8, 2019 12:54
const message = isObject(res) ? res : { status, message: res };
return _throw(message);
}
if (exception instanceof GrpcException) {
Copy link
Member

@kamilmysliwiec kamilmysliwiec Jul 17, 2019

Choose a reason for hiding this comment

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

We shouldn't modify the existing BaseRpcExceptionFilter but instead, just create another filter which can be used by developers separately. Everyone would have to explicitly use this filter in the application (by either using APP_FILTER token or app.useGlobalFilters()). This class should remain the same (untouched). Hence, we'd avoid breaking changes + transport-specific logic in a generic Rpc filter class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super clear, i will made the change next week.

@zuohuadong
Copy link
Contributor

Dear @tychota
Is there any progress in this PR?

@tychota
Copy link
Contributor Author

tychota commented Aug 8, 2019

Dear @tychota
Is there any progress in this PR?

Umm sadly no.

After @kamilmysliwiec did ask me to change the handler, i have started a project for a new client and i'm pretty much busy.
For instance, next week, I must help 5 devs to implement in app billing for ios and android, plus the server part and that leave me hardly any place to do open source.
So i can try to have a bit of time to finish this next week but that is unreliable.
After that, I have two weeks on vacations without internet.

I know that it leaves us to september until i can finaly work on that. I'm sad about too 😕

That being said if tou want to help, you can fork my fork and do a PR with the last steps or some parts.

@guyisra
Copy link

guyisra commented Feb 24, 2020

@kamilmysliwiec / @tychota any chance to revive this?
what is left to merge this?

@mkaufmaner
Copy link
Contributor

@tychota I will be more than happy to pick this up for you this weekend. I'll see what I can get done ;)

@mkaufmaner
Copy link
Contributor

So what I can say for now is that I'm definitely against forcing everyone to add Status message type to their proto definitions and passing this property within an actual response. I don't have enough time to dive into the gRPC sdk api now, but isn't this answer still up-to-date https://stackoverflow.com/a/42831446/6696689? Perhaps, we could just pass status property down to the gRPC library, and then, read this information from the client.

@kamilmysliwiec It seems that most client libraries have the concept of the "richer error model" built in. Thus, the Status message type doesn't need to be included in the proto definitions of NestJS developers using the gRPC microservice.

This richer error model is already supported in the C++, Go, Java, Python, and Ruby libraries, and at least the grpc-web and Node.js libraries have open issues requesting it. Other language libraries may add support in the future if there’s demand, so check their github repos if interested.

This documentation seems a little outdated because it seems to me that the richer error model is included with the node library. See the server.js and the common.js which is used by client.js.

@tychota
Copy link
Contributor Author

tychota commented Mar 8, 2020

Hello. Sorry for not answering, didn't get @guyisra notification.

what is left to merge this?

I don't know. Long time I haven't touched this and not working on any project with this anymore.
We manage to get it working in the previous project without any changes in Nest by:

  1. defining base exception (this was done in the PR hand could be merged)
import grpc from 'grpc';

export class GrpcFailedPreconditionError extends Error {
  public code = grpc.status.FAILED_PRECONDITION;
}
  1. inheriting to have some business oriented exception (this is project specific)
export class EmailAlreadyVerifiedError extends GrpcFailedPreconditionError {}

3)) using a Interceptor (this could be part of nest but wasn't done)

export function makeGrpcError(err: Error & {code: number}): { code: number; message: string } {
  const code = err.code !== undefined ? err.code : 2;
  const message = err.details ? String(err.details) : err.constructor.name;
  return { code, message };
}

@Injectable()
export class HandleErrorsInterceptor implements NestInterceptor {
  public intercept(context: ExecutionContext, next: CallHandler): Observable<unknown> {
    return next.handle().pipe(
      catchError(err => {
        const grpcError = makeGrpcError(err);
        return throwError(new RpcException(grpcError));
      }),
    );
  }
}

@tychota I will be more than happy to pick this up for you this weekend. I'll see what I can get done ;)

Nice I was a bit gulty to have this pending so please take the lead :)

@kamilmysliwiec It seems that most client libraries have the concept of the "richer error model" built in. Thus, the Status message type doesn't need to be included in the proto definitions of NestJS developers using the gRPC microservice.

Impressive research.

@kamilmysliwiec
Copy link
Member

Closing due to inactivity

leeklife pushed a commit to leeklife/nest that referenced this pull request Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants