-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Conversation
ccc9476
to
e1cd93d
Compare
@kamilmysliwiec: This is a start. Let me know what you think ! |
Pull Request Test Coverage Report for Build 3536
💛 - Coveralls |
@tychota I have manually triggered the build and it passed now :) |
I left a comment here explaining the "Idem shall be an available exception ?" The question behind is that the closest HTTP Exception that match
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
You mean:
|
Exactly :) I also answered to your comment about the canceled error. |
Thanks.
Thanks also, lets discuss it in the thread. |
e1cd93d
to
d165795
Compare
Logs (click to expand)➜ nest git:(feat/grpc-exception-mapping) ✗ sh ./scripts/run-integration.sh
/Users/tychotatitscheff/Code/open-source/nest/node_modules/ts-node/src/index.ts:338 npm ERR! A complete log of this run can be found in:
|
d165795
to
5dd17d4
Compare
(I messed up with rebase, post above edited with new commits) |
Yes :)
Exactly
I think that the current solution is better (no inheritance)
Integration test is great. Regarding unit tests, I'd say that we should add 2 tests that would check thrown error (from |
(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.
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). |
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 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 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 With Proto + GRPC we really no need to share anything more than set of Just IMHO ;) |
Hi, still won't have time to continue the dev on this this week. --
@anton-alation thanks a lot for this comment and I think I agree with the general idea that GRPC strength is expressiveness.
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 and 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
The problem I try to solve is the following one. https://cloud.google.com/apis/design/errors?hl=en#http_mapping Tl;Dr
I agree on the principe but I have fear that:
I'm now a bit lost, would like to have your point of view after the link I send. |
@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 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 |
ping 🏓 |
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"). |
So what I can say for now is that I'm definitely against forcing everyone to add |
Nice to know we are on the same page. I will:
Then we will be good to go. |
5dd17d4
to
28c2a04
Compare
Well I did manage to pass the error to Grpc client: (see but I'm not sure the way I did it is what you expect @kamilmysliwiec I also try to do a Can you help me so I can go forward, @kamilmysliwiec? |
aa078f5
to
f6fdf2d
Compare
f6fdf2d
to
9bd91f3
Compare
const message = isObject(res) ? res : { status, message: res }; | ||
return _throw(message); | ||
} | ||
if (exception instanceof GrpcException) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Dear @tychota |
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. 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. |
@kamilmysliwiec / @tychota any chance to revive this? |
@tychota I will be more than happy to pick this up for you this weekend. I'll see what I can get done ;) |
@kamilmysliwiec It seems that most client libraries have the concept of the "richer error model" built in. Thus, the
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. |
Hello. Sorry for not answering, didn't get @guyisra notification.
I don't know. Long time I haven't touched this and not working on any project with this anymore.
import grpc from 'grpc';
export class GrpcFailedPreconditionError extends Error {
public code = grpc.status.FAILED_PRECONDITION;
}
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));
}),
);
}
}
Nice I was a bit gulty to have this pending so please take the lead :)
Impressive research. |
Closing due to inactivity |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1953
What is the new behavior?
After this PR you will have:
Does this PR introduce a breaking change?
Other information
Still WIP.
Missing: