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.

Sign up for GitHub

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

Jump to bottom

No way to distinguish exception objects #181

Open
markdoliner opened this issue Jan 10, 2020 · 1 comment
Open

No way to distinguish exception objects #181

markdoliner opened this issue Jan 10, 2020 · 1 comment

Comments

@markdoliner
Copy link

I have a service that can throw two different exceptions:

service Whatever {
  exception ExceptionA {}
  exception ExceptionB {}
  void doSomething() throws (1: ExceptionA exceptionA, 2: ExceptionB exceptionB)
}

When the server throws one of the exceptions there's no way for the client to determine which exception was thrown. e.constructor.name is simply "Object"

I'm running thrift-typescript with --target thrift-server.

One workaround is to run thrift-typescript --withNameField, in which case exceptions can be distinguished by checking if (e.__name === 'ExceptionA'), but that's not ideal.

#178 is related.
@markdoliner markdoliner mentioned this issue May 28, 2020
Open
markdoliner-doma added a commit to StatesTitle/thrift-typescript that referenced this issue Jun 1, 2020
@markdoliner-doma
Change client to return instances of our classes.
0ee1e6b 
Have client return instances of our classes.

This changes the generated client code so that it returns instances of the generated thrift classes instead of plain objects.

This change is a step toward having exception classes derive from Error (creditkarma#178) and be distinguishable from each other (creditkarma#181).

The change itself is small and easy. The compiler already generates "MyEndpoint__Result" classes that have a read() function that instantiates the class, I just needed to call it from the appropriate place in the Client class.

It's hard to understand this change without full knowledge of how the generated code fits together, but here's a diff of a generated ping function before and after anyway:
```
     public ping(context?: Context): Promise<string> {
         const writer: thrift.TTransport = new this.transport();
         const output: thrift.TProtocol = new this.protocol(writer);
         output.writeMessageBegin("ping", thrift.MessageType.CALL, this.incrementRequestId());
         const args: IPing__ArgsArgs = {};
         Ping__ArgsCodec.encode(args, output);
         output.writeMessageEnd();
         return this.connection.send(writer.flush(), context).then((data: Buffer) => {
             const reader: thrift.TTransport = this.transport.receiver(data);
             const input: thrift.TProtocol = new this.protocol(reader);
             try {
                 const { fieldName: fieldName, messageType: messageType }: thrift.IThriftMessage = input.readMessageBegin();
                 if (fieldName === "ping") {
                     if (messageType === thrift.MessageType.EXCEPTION) {
                         const err: thrift.TApplicationException = thrift.TApplicationExceptionCodec.decode(input);
                         input.readMessageEnd();
                         return Promise.reject(err);
                     }
                     else {
-                        const result: IPing__Result = Ping__ResultCodec.decode(input);
+                        const result: Ping__Result = Ping__Result.read(input);
                         input.readMessageEnd();
                         if (result.success != null) {
                             return Promise.resolve(result.success);
                         }
                         else {
                             return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.UNKNOWN, "ping failed: unknown result"));
                         }
                     }
                 }
                 else {
                     return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.WRONG_METHOD_NAME, "Received a response to an unknown RPC function: " + fieldName));
                 }
             }
             catch (err) {
                 return Promise.reject(err);
             }
         });
     }
```

You'll notice that I also changed the type of `result` from `IPing__Result` to `Ping__Result`. This isn't important and could be reverted, but I think it's better this way. I like keeping types specific until the point where it's useful for them to be generic, and I'd advocate to make the same change to the types of struct class members (would need to change these types, I think: https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/class.ts#L293 and https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/reader.ts#L51).
markdoliner-doma added a commit to StatesTitle/thrift-typescript that referenced this issue Jun 1, 2020
@markdoliner-doma
Change client to return instances of our classes.
1eba49d 
This changes the client code generated for `--target thrift-server` so that it returns instances of the generated thrift classes instead of plain objects. I believe the client code generated for `--target apache` already behaves this way.

One advantage is this allows `instanceof` to be used on Thrift responses, which makes exceptions [distinguishable from each other](creditkarma#181) using `instanceof`. It would also increase the usefulness of having [exception classes derive from Error](creditkarma#178).

The change itself is small and easy. The compiler already generates "MyEndpoint__Result" classes that have a read() function that instantiates the class, I just needed to call it from the appropriate place in the Client class.

To review this change it'll be helpful if you're familiar with the various classes that are generated. I'll refrain from trying to describe it here--you're better off looking some generated code on your own. Here's a diff showing how this change affects a generated ping function. This is in the Client class:
```
     public ping(context?: Context): Promise<string> {
         const writer: thrift.TTransport = new this.transport();
         const output: thrift.TProtocol = new this.protocol(writer);
         output.writeMessageBegin("ping", thrift.MessageType.CALL, this.incrementRequestId());
         const args: IPing__ArgsArgs = {};
         Ping__ArgsCodec.encode(args, output);
         output.writeMessageEnd();
         return this.connection.send(writer.flush(), context).then((data: Buffer) => {
             const reader: thrift.TTransport = this.transport.receiver(data);
             const input: thrift.TProtocol = new this.protocol(reader);
             try {
                 const { fieldName: fieldName, messageType: messageType }: thrift.IThriftMessage = input.readMessageBegin();
                 if (fieldName === "ping") {
                     if (messageType === thrift.MessageType.EXCEPTION) {
                         const err: thrift.TApplicationException = thrift.TApplicationExceptionCodec.decode(input);
                         input.readMessageEnd();
                         return Promise.reject(err);
                     }
                     else {
-                        const result: IPing__Result = Ping__ResultCodec.decode(input);
+                        const result: Ping__Result = Ping__Result.read(input);
                         input.readMessageEnd();
                         if (result.success != null) {
                             return Promise.resolve(result.success);
                         }
                         else {
                             return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.UNKNOWN, "ping failed: unknown result"));
                         }
                     }
                 }
                 else {
                     return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.WRONG_METHOD_NAME, "Received a response to an unknown RPC function: " + fieldName));
                 }
             }
             catch (err) {
                 return Promise.reject(err);
             }
         });
     }
```

You'll notice that I also changed the type of `result` from `IPing__Result` to `Ping__Result`. This isn't important and could be reverted, but I think it's better this way. I like keeping types specific until the point where it's useful for them to be generic. I'd advocate for making the same change to the types of struct class members (would need to change these types, I think: https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/class.ts#L293 and https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/reader.ts#L51), though that isn't of immediately use to me personally and I don't intend to pursue it.
markdoliner-doma added a commit to StatesTitle/thrift-typescript that referenced this issue Jun 1, 2020
@markdoliner-doma
Add Error to prototype chain of exceptions.
3501c16 
This is one possible approach for making exceptions derive from Error (creditkarma#178) and be distinguishable from each other (creditkarma#181).

It adds this line to the constructor of exception classes:
```
Object.setPrototypeOf(Object.getPrototypeOf(this), Error.prototype);
```

It's sort of the half-way solution. It's an attempt to solve the above problems with minimal disruption. See the in-code comment for details.

That being said, I'm not a fan of this solution. It does make `instanceof` work correctly, which is great. But `name` still exists and there are problems if its type isn't `string`. Also `name` gets set to "Error" instead of the name of our exception class. We could fix that by setting `this.name` in our constructor but the whole point of doing it this way was to avoid polluting our exception classes with fields from Error. Worth mentioning: I have no idea what sets `name` to "Error".

Also I didn't look at the render/apache/ code at all. It's possible a similar change should be made there--I have no idea.
markdoliner-doma added a commit to StatesTitle/thrift-typescript that referenced this issue Jun 1, 2020
@markdoliner-doma
Change client to return instances of our classes.
6a532b2 
This changes the client code generated for `--target thrift-server` so that it returns instances of the generated thrift classes instead of plain objects. I believe the client code generated for `--target apache` already behaves this way.

One advantage is this allows `instanceof` to be used on Thrift responses, which makes exceptions [distinguishable from each other](creditkarma#181) using `instanceof`. It would also increase the usefulness of having [exception classes derive from Error](creditkarma#178).

The change itself is small and easy. The compiler already generates "MyEndpoint__Result" classes that have a read() function that instantiates the class, I just needed to call it from the appropriate place in the Client class.

To review this change it'll be helpful if you're familiar with the various classes that are generated. I'll refrain from trying to describe it here--you're better off looking some generated code on your own. Here's a diff showing how this change affects a generated ping function. This is in the Client class:
```
     public ping(context?: Context): Promise<string> {
         const writer: thrift.TTransport = new this.transport();
         const output: thrift.TProtocol = new this.protocol(writer);
         output.writeMessageBegin("ping", thrift.MessageType.CALL, this.incrementRequestId());
         const args: IPing__ArgsArgs = {};
         Ping__ArgsCodec.encode(args, output);
         output.writeMessageEnd();
         return this.connection.send(writer.flush(), context).then((data: Buffer) => {
             const reader: thrift.TTransport = this.transport.receiver(data);
             const input: thrift.TProtocol = new this.protocol(reader);
             try {
                 const { fieldName: fieldName, messageType: messageType }: thrift.IThriftMessage = input.readMessageBegin();
                 if (fieldName === "ping") {
                     if (messageType === thrift.MessageType.EXCEPTION) {
                         const err: thrift.TApplicationException = thrift.TApplicationExceptionCodec.decode(input);
                         input.readMessageEnd();
                         return Promise.reject(err);
                     }
                     else {
-                        const result: IPing__Result = Ping__ResultCodec.decode(input);
+                        const result: Ping__Result = Ping__Result.read(input);
                         input.readMessageEnd();
                         if (result.success != null) {
                             return Promise.resolve(result.success);
                         }
                         else {
                             return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.UNKNOWN, "ping failed: unknown result"));
                         }
                     }
                 }
                 else {
                     return Promise.reject(new thrift.TApplicationException(thrift.TApplicationExceptionType.WRONG_METHOD_NAME, "Received a response to an unknown RPC function: " + fieldName));
                 }
             }
             catch (err) {
                 return Promise.reject(err);
             }
         });
     }
```

You'll notice that I also changed the type of `result` from `IPing__Result` to `Ping__Result`. This isn't important and could be reverted, but I think it's better this way. I like keeping types specific until the point where it's useful for them to be generic. I'd advocate for making the same change to the types of struct class members (would need to change these types, I think: https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/class.ts#L293 and https://github.com/creditkarma/thrift-typescript/blob/e8931ff0d98c871360af99bbfa805cfd088d2f0e/src/main/render/thrift-server/struct/reader.ts#L51), though that isn't of immediately use to me personally and I don't intend to pursue it.

Remaining work:
- Fix tests.
@markdoliner-doma markdoliner-doma mentioned this issue Jun 1, 2020
Open
@markdoliner-doma
Copy link

I posted a PR that addresses this by changing the client to return instances of our classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@markdoliner @markdoliner-doma