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

Exception classes ought to inherit from Error class #178

Open
jlai opened this issue Nov 21, 2019 · 3 comments
Open

Exception classes ought to inherit from Error class #178

jlai opened this issue Nov 21, 2019 · 3 comments

Comments

@jlai
Copy link

jlai commented Nov 21, 2019

Currently, this library generates exception objects as regular thrift structs which do not extend Error. This is problematic because it doesn't have a stack trace, and behaves weirdly for frameworks that expect that throw is only used with Error, such as graphql-js.

This is also different from the Apache thrift generator, which has exception types inheriting from thrift.TException.

Overall, it would be a lot cleaner (and in line with standard JS coding practices) to make exceptions inherit from Error.
@markdoliner markdoliner mentioned this issue Jan 10, 2020
Open
@markdoliner
Copy link

I spent some time looking at this today because I think it would address the related issue #181, and I thought it might be helpful to share my findings.

Currently the code generation for exceptions is identical to structs. It wouldn't be too difficult to tweak that code so that Thrift exception classes derive from Error. There's a small wrinkle that structs extend the StructLike class (see the extendsAbstract() function call here, which is defined here) and we wouldn't want to change StructLike to derive from Error so we might need to add some sort of ErrorStructLike class that does the same thing as StructLike and also extends Error, but that's fine. No big deal.

The bigger question is that the decode functions don't actually instantiate objects. They return plain old JavaScript objects that match the exception's interface. For example, if you have CustomException with an errorCode field then the decode function will return {errorCode: 12345}. It won't return new CustomException({errorCode: 12345}). I don't know the reasoning that went into the decision to return plain objects vs. instantiating classes.

I only investigated --target thrift-server. I didn't investigate --target apache.

So now I have some questions for the fine folks at Credit Karma. cc @kevin-greene-ck

  1. Anyone have thoughts on this? Should Thrift exceptions derive from Error? Should they not? No opinion?
  2. Should decode functions instantiate objects? Should they not? Is there a reason for doing it one way or the other? Is this decision documented somewhere? I'm not familiar with the source of thrift-typescript or thrift-server or thrift-client... I haven't even thought about why interfaces are used. Do they serve an important purpose?
  3. I'm surprised more people haven't had problems with this. How are other folks distinguishing between different types of exceptions? Maybe passing --withNameField to thrift-typescript to add an __name field to all structs?
  4. I noticed a GitHub issue about version 4 being in development a year ago. It doesn't mention exceptions or object instantiation... does that mean these things aren't on the roadmap?
  5. This question is getting off topic, but I'm curious about the overall status of the project. It seems like there hasn't been a lot of activity from the Credit Karma folks. Are you still using Thrift, I hope? Any plans to make improvements? Or hard to allocate people to work on it?

@markdoliner
Copy link

markdoliner commented May 29, 2020 •
edited
Loading

Other consideration: If exception classes inherit from Error class, what do we do if the exception has fields named message, name, or any other field that might exist on the Error class ('stack', 'fileName', 'lineNumber', etc)? Maybe the compiler should disallow them with a helpful message? Or it could allow them, which would result in generated ts that doesn't compile when the exception class uses a different type for the field than the Error class (like makes message optional or changes the type to a bool).

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
Change exceptions to extend Error.
62a3004 
This changes exception classes generated for `--target thrift-server` so that they extend `ErrorThriftLike` instead of `ThriftLike`. There's a corresponding `thrift-server` change to add the `ErrorThriftLike` class.

As mentioned in [the GitHub issue](creditkarma#178), this is useful because it causes Thrift exceptions to have stack traces and because some frameworks expect exceptions to derive from Error. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. Here's an example:

```
await expect(thriftClient.someFunction()).rejects.toThrow()
```

`toThrow` doesn't identify Thrift exceptions as exceptions because they don't derive from Error and so it will return false in such cases. Part of the blame could fall on Jest, because perhaps `toThrow` should return true for any rejected promise regardless of the type of object being throw, but I think some of the blame still falls on Thrift.

A downside of extending Error is that there's a naming collision when developers define fields named `name`, `message`, `stack`, etc in their Thrift exceptions (because these are defined by the super class). I think this is an acceptable tradeoff. FYI `tsc` complains if someone defines a field that differs from the super class. For example, if someone has a Thrift exception with `1: required i32 message` (because message must be a string) or `1: optional string message` (because message is required).

I tried to avoid the naming collisions by manually adding Error to the exception class's prototype chain. It might avoid the tsc compile errors when there's a naming collision but it doesn't really eliminate the problem of same-named fields being used for two different things. And so I think manually changing the prototype chain is a bad solution.

Possible remaining work:
- The naming collision limitation should be mentioned in README.md.
- `tsc` complains for naming collisions, and that's fine. Maybe the Thrift compiler should check for and report on collisions? Would probably be a friendly user experience. One challenge is [different JS implementations define different properties on their Error class](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Instance_properties).
- I didn't change the code generated for `--target apache`. It looks like those classes also don't derive from Error. They probably should.
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 exceptions to extend Error.
f7a1ee2 
This changes exception classes generated for `--target thrift-server` so that they extend `ErrorThriftLike` instead of `ThriftLike`. There's a corresponding `thrift-server` change to add the `ErrorThriftLike` class.

As mentioned in [the GitHub issue](creditkarma#178), this is useful because it causes Thrift exceptions to have stack traces and because some frameworks expect exceptions to derive from Error. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. Here's an example:

```
await expect(thriftClient.someFunction()).rejects.toThrow()
```

`toThrow` doesn't identify Thrift exceptions as exceptions because they don't derive from Error and so it will return false in such cases. Part of the blame could fall on Jest, because perhaps `toThrow` should return true for any rejected promise regardless of the type of object being throw, but I think some of the blame still falls on Thrift.

A downside of extending Error is that there's a naming collision when developers define fields named `name`, `message`, `stack`, etc in their Thrift exceptions (because these are defined by the super class). I think this is an acceptable tradeoff. FYI `tsc` complains if someone defines a field that differs from the super class. For example, if someone has a Thrift exception with `1: required i32 message` (because message must be a string) or `1: optional string message` (because message is required).

I tried to avoid the naming collisions by manually adding Error to the exception class's prototype chain. It might avoid the tsc compile errors when there's a naming collision but it doesn't really eliminate the problem of same-named fields being used for two different things. And so I think manually changing the prototype chain is a bad solution.

Possible remaining work:
- Add a test?
- Update README.md to mention that end-users need to require the new version of thrift-server which includes `ErrorThrifTLike`.
- Update README.me to mention the naming collision limitation.
- `tsc` complains for naming collisions, and that's fine. Maybe the Thrift compiler should check for and report on collisions? Would probably be a friendly user experience. One challenge is [different JS implementations define different properties on their Error class](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Instance_properties).
- I didn't change the code generated for `--target apache`. It looks like those classes also don't derive from Error. They probably should.
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 added a commit to StatesTitle/thrift-typescript that referenced this issue Jun 1, 2020
@markdoliner-doma
Change exceptions to extend Error.
fd7fe5f 
This changes exception classes generated for `--target thrift-server` so that they extend `ErrorThriftLike` instead of `ThriftLike`. There's a corresponding `thrift-server` change to add the `ErrorThriftLike` class.

As mentioned in [the GitHub issue](creditkarma#178), this is useful because it causes Thrift exceptions to have stack traces and because some frameworks expect exceptions to derive from Error. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. Here's an example:

```
await expect(thriftClient.someFunction()).rejects.toThrow()
```

`toThrow` doesn't identify Thrift exceptions as exceptions because they don't derive from Error and so it will return false in such cases. Part of the blame could fall on Jest, because perhaps `toThrow` should return true for any rejected promise regardless of the type of object being throw, but I think some of the blame still falls on Thrift.

A downside of extending Error is that there's a naming collision when developers define fields named `name`, `message`, `stack`, etc in their Thrift exceptions (because these are defined by the super class). I think this is an acceptable tradeoff. FYI `tsc` complains if someone defines a field that differs from the super class. For example, if someone has a Thrift exception with `1: required i32 message` (because message must be a string) or `1: optional string message` (because message is required).

I tried to avoid the naming collisions by manually adding Error to the exception class's prototype chain. It might avoid the tsc compile errors when there's a naming collision but it doesn't really eliminate the problem of same-named fields being used for two different things. And so I think manually changing the prototype chain is a bad solution.

Remaining work:
- Fix tests.
- Update README.md to mention that end-users need to require the new version of thrift-server which includes `ErrorThrifTLike`.
- Update README.me to mention the naming collision limitation.
- `tsc` complains for naming collisions, and that's fine. Maybe the Thrift compiler should check for and report on collisions? Would probably be a friendly user experience. One challenge is [different JS implementations define different properties on their Error class](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Instance_properties).
- I didn't change the code generated for `--target apache`. It looks like those classes also don't derive from Error. They probably should.
markdoliner-doma added a commit to StatesTitle/thrift-server that referenced this issue Jun 1, 2020
@markdoliner-doma
Add a StructLike class that extends Error.
3841574 
Exception classes should extend this so that they derive from Error.

As mentioned in [the GitHub issue](creditkarma/thrift-typescript#178), this is useful because it causes Thrift exceptions to have stack traces and because some frameworks expect exceptions to derive from Error. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. Here's an example:

```
await expect(thriftClient.someFunction()).rejects.toThrow()
```

`toThrow` doesn't identify Thrift exceptions as exceptions because they don't derive from Error and so it will return false in such cases. Part of the blame could fall on Jest, because perhaps `toThrow` should return true for any rejected promise regardless of the type of object being throw, but I think some of the blame still falls on Thrift.

Possible remaining work:
- Add a test?
- Bump version number so end users can require the new version.
markdoliner-doma added a commit to StatesTitle/thrift-server that referenced this issue Jun 1, 2020
@markdoliner-doma
Add a StructLike class that extends Error.
50c661b 
Exception classes should extend this so that they derive from Error.

As mentioned in [the thrift-typescript GitHub issue](creditkarma/thrift-typescript#178), it's useful for exceptions to derive from Error so they have stack traces and because some frameworks expect it. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. See the related thrift-typescript PR for more details.

Possible remaining work:
- Add a test?
- Bump version number so end users can require the new version.
markdoliner-doma added a commit to StatesTitle/thrift-typescript that referenced this issue Jun 1, 2020
@markdoliner-doma
Change exceptions to extend Error.
628235e 
This changes exception classes generated for `--target thrift-server` so that they extend `ErrorThriftLike` instead of `ThriftLike`. There's a corresponding `thrift-server` change to add the `ErrorThriftLike` class.

As mentioned in [the GitHub issue](creditkarma#178), this is useful because it causes Thrift exceptions to have stack traces and because some frameworks expect exceptions to derive from Error. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. Here's an example:

```
await expect(thriftClient.someFunction()).rejects.toThrow()
```

`toThrow` doesn't identify Thrift exceptions as exceptions because they don't derive from Error and so it will return false in such cases. Part of the blame could fall on Jest, because perhaps `toThrow` should return true for any rejected promise regardless of the type of object being throw, but I think some of the blame still falls on Thrift.

A downside of extending Error is that there's a naming collision when developers define fields named `name`, `message`, `stack`, etc in their Thrift exceptions (because these are defined by the super class). I think this is an acceptable tradeoff. FYI `tsc` complains if someone defines a field that differs from the super class. For example, if someone has a Thrift exception with `1: required i32 message` (because message must be a string) or `1: optional string message` (because message is required).

I tried to avoid the naming collisions by manually adding Error to the exception class's prototype chain. It might avoid the tsc compile errors when there's a naming collision but it doesn't really eliminate the problem of same-named fields being used for two different things. And so I think manually changing the prototype chain is a bad solution.

Remaining work:
- Fix tests.
- Update README.md to mention that end-users need to require the new version of thrift-server which includes `ErrorThriftLike`.
- Update README.me to mention the naming collision limitation.
- `tsc` complains for naming collisions, and that's fine. Maybe the Thrift compiler should check for and report on collisions? Would probably be a friendly user experience. One challenge is [different JS implementations define different properties on their Error class](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Instance_properties).
- I didn't change the code generated for `--target apache`. It looks like those classes also don't derive from Error. They probably should.
This was referenced Jun 1, 2020
Open
Open
Open
@markdoliner-doma
Copy link

I took a stab at this in #185 and #184 and creditkarma/thrift-server#132

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
3 participants
@markdoliner @jlai @markdoliner-doma