Rework ConnectError to ConnectException #120

jzbrooks · 2023-09-28T22:27:07Z

Exceptions that might be caught should not extend throwable. Throwable
is the subtype of both Exception and Error. The latter indicates a
critical failure and should only be caught in very rare circumstances.
This change renames the class and makes the supertype more specific
to suggest that catching this throwable is reasonable.

Before submitting your PR: Please read through the contribution guide at https://github.com/connectrpc/connect-kotlin/blob/main/.github/CONTRIBUTING.md

Note: This is a source-breaking change, which was briefly discussed in slack.

Exceptions that might be caught should not extend throwable. Throwable is the subtype of both Exception and Error. The latter indicates a critical failure and should only be caught in very rare circumstances. This change renames the class and makes the supertype more specific to suggest that catching this throwable is reasonable.

CLAassistant · 2023-09-28T22:27:12Z

All committers have signed the CLA.

pkwarren

There are several other APIs that we should consider updating to use consistent names everywhere - i.e. connectError() -> connectException().

Since we just made an API change in the last release, I wonder if we shouldn't hold off on changing ConnectError -> ConnectException everywhere so we can accumulate other API changes as well prior to stabilizing the API for a v1. Maybe we split this PR into two - first doing the Throwable -> Exception change and then saving another with a full rename to merge along with other API changes.

library/src/main/kotlin/com/connectrpc/StreamResult.kt

Co-authored-by: Philip K. Warren <pkwarren@users.noreply.github.com>

jzbrooks · 2023-09-28T22:48:04Z

Changing the super type isn't a source breaking change, but it does break the API contract because some people might be depending on the super type being Throwable. If we make that breaking change, in my opinion we might as well make the source breaking change to get them all out of the way quickly.

I'll look through a little more carefully for other parts of the API surface that need a name update.

I get the concern with a breaking change (especially successive ones). I understand if we want to leave this open a while. Have y'all considered keeping a changelog in the repo? It might make changes like this one more palatable for early adopters.

Edit: the super type change could be a breaking change in certain generic type bound situations, upon further thought.

pkwarren

A few more spots need to be updated:

grpcCompletionToConnectError -> grpcCompletionToConnectException.
connectErr -> connectExcception in conformance tests and the Main.kt examples.
ServerStreamingResult.error -> exception in the conformance tests.
parseConnectUnaryError -> parseConnectUnaryException.

I think we should also change the following for consistency:

ResponseMessage.Failure.error -> exception or cause.
StreamResult.Complete.error -> exception or cause.
HTTPResponse.error -> exception or cause.

In most (3 of 4) uses of this method, the first argument was known to be non-null at the call site. Since in the event that the first argument _was_ null, we basically create a `ConnectException` without much helpful information about any error, it made sense to me to remove the concept of a nullable completion from the method entirely. The function is also a little more idiomatic now, following the typical `to<Type>OrNull` pattern established by the language for conversion between types. It also reduces the classes generated by the compiler since the method is a proper member of `GRPCCompletion`. I would probably push this idea a little further in how error states are modeled more generally, but that's creeping outside the scope of this change.

jzbrooks · 2023-09-29T18:06:02Z

I think the remaining issues are addressed. I was able to verify the conformance tests compile, but I'm not able to run them. I'll keep an eye on the PR checks when they're run. Thanks!

Note the extended commit message in 4c10702

pkwarren

Looks great!

pkwarren · 2023-09-29T19:35:29Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCCompletion.kt

+     * Converts a completion into a [ConnectException] if the completion failed or if a throwable is passed
+     * @return a ConnectException on failure, null otherwise
+     */
+    fun toConnectExceptionOrNull(serializationStrategy: SerializationStrategy, cause: Throwable? = null): ConnectException? {


Nice refactoring here.

pkwarren · 2023-09-29T19:39:26Z

Looks like the lint check is failing - if you run make lintfix and push the changes that should fix it.

pkwarren

Blocking for now until we can address conformance tests and lint warnings.

jzbrooks · 2023-10-02T14:02:12Z

The assertions in the conformance tests don't print the assertion message. This makes debugging via CI difficult. I don't have a docker license, so I can't run them locally.

pkwarren · 2023-10-02T14:47:51Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCInterceptor.kt

+                                trailers = trailers,
+                            )
+                        } else {
+                            val exception = ConnectException(Code.UNKNOWN)


To resolve the test failure, this else condition should change to:

return@fold result

This way we'll preserve errors like DEADLINE_EXCEEDED.

That whole if-else is an expression, which should resolve to L169 if completion != null and L176 otherwise. Could you elaborate on why this callback needs a non-local return and the others don't? It seems like the expression should bubble up where it needs to go via fold, like the onHeaders and onMessage callbacks.

I'm happy to make that change, but I'm aiming to understand it better first.

So result is already instantiated in some cases (with a code like DEADLINE_EXCEEDED and a cause). Some of the tests rely on this. Alternatively to doing return@fold. you could just have result here as the return value.

I'm tracking now. Thanks for the explaination.

pkwarren · 2023-10-03T14:54:29Z

library/src/main/kotlin/com/connectrpc/protocols/GRPCInterceptor.kt

+                            )
+                        } else {
+                            val exception = ConnectException(result.code)
+                            StreamResult.Complete(exception.code, exception, trailers)


We may be losing an underlying cause here - so we should just return the result here.

I worked around the problem by using the result's code property value in the connect exception constructor, but I like that idea better.

e83429a

pkwarren reviewed Sep 28, 2023

View reviewed changes

library/src/main/kotlin/com/connectrpc/StreamResult.kt Outdated Show resolved Hide resolved

Update library/src/main/kotlin/com/connectrpc/StreamResult.kt

1fd9c28

Co-authored-by: Philip K. Warren <pkwarren@users.noreply.github.com>

Rename Complete<T>.connectError

92a8413

pkwarren reviewed Sep 28, 2023

View reviewed changes

jzbrooks added 3 commits September 28, 2023 23:05

Rename error properties to cause

bd49002

Address missing renames in conformance tests

0cfee73

jzbrooks force-pushed the connectexception branch from 3a8f81c to 0cfee73 Compare September 29, 2023 03:24

pkwarren approved these changes Sep 29, 2023

View reviewed changes

pkwarren requested changes Sep 29, 2023

View reviewed changes

jzbrooks added 2 commits October 2, 2023 07:59

Fix style issues

3ee1d98

Clarify an assertion message

652bcab

pkwarren reviewed Oct 2, 2023

View reviewed changes

jzbrooks added 2 commits October 2, 2023 12:14

Return result code in failure case

a910957

Consistently avoid non-local returns

47b9af9

pkwarren reviewed Oct 3, 2023

View reviewed changes

Return the result if the completion cannot be parsed

e83429a

pkwarren approved these changes Oct 3, 2023

View reviewed changes

pkwarren merged commit 7b7c746 into connectrpc:main Oct 3, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework ConnectError to ConnectException #120

Rework ConnectError to ConnectException #120

jzbrooks commented Sep 28, 2023

CLAassistant commented Sep 28, 2023 •

edited

Loading

pkwarren left a comment

jzbrooks commented Sep 28, 2023 •

edited

Loading

pkwarren left a comment

jzbrooks commented Sep 29, 2023

pkwarren left a comment

pkwarren Sep 29, 2023

pkwarren commented Sep 29, 2023

pkwarren left a comment

jzbrooks commented Oct 2, 2023

pkwarren Oct 2, 2023

jzbrooks Oct 2, 2023 •

edited

Loading

pkwarren Oct 3, 2023

jzbrooks Oct 3, 2023

pkwarren Oct 3, 2023

jzbrooks Oct 3, 2023

Rework ConnectError to ConnectException #120

Rework ConnectError to ConnectException #120

Conversation

jzbrooks commented Sep 28, 2023

CLAassistant commented Sep 28, 2023 • edited Loading

pkwarren left a comment

Choose a reason for hiding this comment

jzbrooks commented Sep 28, 2023 • edited Loading

pkwarren left a comment

Choose a reason for hiding this comment

jzbrooks commented Sep 29, 2023

pkwarren left a comment

Choose a reason for hiding this comment

pkwarren Sep 29, 2023

Choose a reason for hiding this comment

pkwarren commented Sep 29, 2023

pkwarren left a comment

Choose a reason for hiding this comment

jzbrooks commented Oct 2, 2023

pkwarren Oct 2, 2023

Choose a reason for hiding this comment

jzbrooks Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

pkwarren Oct 3, 2023

Choose a reason for hiding this comment

jzbrooks Oct 3, 2023

Choose a reason for hiding this comment

pkwarren Oct 3, 2023

Choose a reason for hiding this comment

jzbrooks Oct 3, 2023

Choose a reason for hiding this comment

CLAassistant commented Sep 28, 2023 •

edited

Loading

jzbrooks commented Sep 28, 2023 •

edited

Loading

jzbrooks Oct 2, 2023 •

edited

Loading