Convert methods with checked exceptions to unchecked (support streaming and functional programming)

[Haarolean]

Hi,
GHObject#getCreatedAt and GHObject#getUpdatedAt throw unnecessary IOException which is not being thrown by an underlying code. Is there a reason for this?

[bitwiseman]

Most likely reason is they had code that could throw an IOException in the past and changing them now would result in API breaking changes. It is also possible that some derived classes include code that could throw IOException and this was done to support those.

[Haarolean]

@bitwiseman have you considered getting rid of all checked exceptions at all?

[bitwiseman]

@Haarolean
Not in short term. Doing so would be an API breaking change requiring us (based on semver) to create a new major version. Likely, that would involved adding a new package beside the original package to maintain backward compatibility for some period.

Also, Why would we do that? Is it particularly needed?

[Haarolean]

@bitwiseman It prevents end users from using library's calls within Java streams.

[bitwiseman]

@Haarolean
Ah, I see. Here's a discussion of workarounds. None of them seem very nice, so I can see why you'd want to avoid that.

We're not ready to introduce binary breaking changes at this time. We use "bridge methods" if needed to preserve binary compatibility but I don't think that will work here.

We could try introducing new methods called createdAt() and updatedAt() which wrap those getCreatedAt() and getUpdatedAt() method to rethrow the IOException as GHException. I'm not sure that's a great choice either, but at least it is non-breaking.

[Haarolean]

@bitwiseman
why are we refraining from breaking changes so much? All of the users are developers in the end, and bumping the library to a new major version shouldn't be a big issue considering migrating some of the things (I doubt people catch these IO exceptions and do something meaningful with them).

[bitwiseman]

@Haarolean
The removal the IOException will result in compilation failures - if not caught, then the caller must also declare throws IOException. If that caller no longer calls any other method that throw IOException, that caller will need to remove throws IOException from its signature. Then cascade that problem throughout the consuming codebase. For small self-contained projects that might not be a big deal, but it is still work. For large projects, that work can cascade to multiple components resulting in many days of work.

Also, you are assuming that all consumers of this library must compile against a new version in order to use it. Many consumers of this library, in particular Jenkins, do so via a plugin system. The exceptions thrown by a method are part of the binary method signature. This results in binary breaking changes potentially causing MethodNotFoundExceptions in two different ways - old consumer trying to call a method with the exception when the new library containing the method without the exception is loaded, or also newly compiled consumer trying to call a method without the exception when running in an environment with the old library containing the method with the exception is loaded.

The level of firestorm that has occurred in the past in the Jenkins project when this project accidentally made binary breaking changes is impressive. It wouldn't happen now because they have added runtime tests to guard against it, but let's not go there.

To be clear, I see value in the "streaming" use case and it would be great to add better support for it. But there's a number of other features an scenarios that would provide greater value to more consumers of this library.

Introducing binary breaking changes is not an option, but there are still a number of options for introducing new APIs and functionality. They're not as easy or quick, but so it goes.

[Haarolean]

@bitwiseman
While I agree that the other changes are way more important than changes like this one, in general, it sounds like refraining from introducing breaking changes and releasing major versions might drastically reduce the innovation. Moreover, the whole development progress might be held hostage for the sake of serving some big consumer (like Jenkins) to help them avoid adapting their codebase to properly consume new changes to the library.

[bitwiseman]

@Haarolean

the whole development progress might be held hostage for the sake of serving some big consumer (like Jenkins) to help them avoid adapting their codebase to properly consume new changes to the library.

You should definitely try helping Jenkins adapt their codebase to properly consume new changes to this library... 🤣 I wish you the best of luck. Also, Jenkins is not alone.

Here's an article on how to wrap method that throw checked exceptions to make them throw unchecked: https://www.oreilly.com/content/handling-checked-exceptions-in-java-streams/

That article mentions this library: https://github.com/vavr-io/vavr

If you'd like to start work on a v2.x of this library, I'd be happy to talk about features and direction.

[Haarolean]

@bitwiseman

It seems like you're missing my point. For instance, I can't imagine a situation when GitHub would avoid implementing new breaking changes solely because a major client is facing challenges in updating their code to support the new API.

I appreciate your suggestions for workarounds. However, my concern goes beyond this specific problem. There are other issues/PRs/changes in this library that could potentially introduce breaking changes. Unfortunately, it seems there's little to no traction toward doing so, though I believe this is off-topic for this particular issue.

[bitwiseman]

I can't imagine a situation when GitHub would avoid implementing new breaking changes solely because a major client is facing challenges in updating their code to support the new API.

I surprised that you would say that. They absolutely avoid making breaking changes, not just for major clients but for all clients. Look at any of the breaking changes they've made over the past ten years and you will see a history of planning those changes very carefully, implementing non-breaking side-by-side replacements whenever possible, communicating the breaking changes years in advance, and finally pulling the trigger on the changes.

Unfortunately, it seems there's little to no traction toward doing so

That's because there are very few contributors willing/able to devote the time needed to implementing any changes or enhancements to this library, and even fewer willing/able to devote the time needed to following a process like GitHub of side-by-side replacement and clear planning and communicating of eventual removal/breaking changes.

[alexec]

You cannot remove the throws clause from the method without breaking changes.

Breaking changes is a great way to stagnate a library so I don’t think you should do this

Bridge methods seem like a good compromise.

[Haarolean]

hi @bitwiseman, sorry for re-iterating this, am I right to think that this

If you'd like to start work on a v2.x of this library, I'd be happy to talk about features and direction.

means that you don't see the possibility of publishing a new major version with breaking changes without introducing new major features at the same time viable?

I'm asking this because I'm reaching the point of considering forking the repo with private patches rather than maintaining numerous workarounds which I have many (#1739, #1689, #1656, you name it).

For example, my "workaround" (with vavr from your suggestions) for checked exceptions looks like this:

Which is, for instance, absolutely horrendous, I think we can agree on that here.

Getting rid of checked exceptions sounds like a thing I might be able to contribute, it's the changes I'm personally interested in in the end, but the scope you suggested exceeds far beyond my availability. Unfortunately, I can't offer my contribution in the form of shaping a whole new version of the library with big features I have no interest in for the sake of seeing the things I need to go live alongside them.

Let me know what you think.

[bitwiseman]

hi @bitwiseman, sorry for re-iterating this, am I right to think that this

If you'd like to start work on a v2.x of this library, I'd be happy to talk about features and direction.

means that you don't see the possibility of publishing a new major version with breaking changes without introducing new major features at the same time viable?

I'm asking this because I'm reaching the point of considering forking the repo with private patches rather than maintaining numerous workarounds which I have many (#1739, #1689, #1656, you name it).

Getting rid of checked exceptions sounds like a thing I might be able to contribute, it's the changes I'm personally interested in in the end, but the scope you suggested exceeds far beyond my availability. Unfortunately, I can't offer my contribution in the form of shaping a whole new version of the library with big features I have no interest in for the sake of seeing the things I need to go live alongside them.

Forking = merge-hell

Do you really mean getting rid of all checked exceptions on all classes? If so, you're talking about a pretty big change that will have ripple effects inside this library so it can't just be done automatically via regex or some tooling. If you are talking about only removing some or most checked exceptions, you're still talking about something that can't be done automatically.

I would strongly suggest you not fork to achieve your goal here. Merging new changes from upstream given the above scenario will be an absolute nightmare.

Non-breaking AND stream-friendly

Let's talk for a minute about one way this could be done without breaking changes and without forking to a separate repo. We could create new classes that extend or wrap the current classes and implement methods that do not throw checked exceptions. These could either go be a nested Unchecked class inside the current classes. The new classes would look something like this:

public class GHPullRequest extends GHIssue implements Refreshable {

  // { ... The current GHPullRequest class code ... } 

    public Unchecked toUnchecked() {
        return new Unchecked();
    }
        
    /**
     * GHPullRequest with no checked exception methods.
     */
    public class Unchecked {

        public boolean isDraft() {
            try {
                return GHPullRequest.this.isDraft();
            } catch (IOException e) {
                throw sneakyThrow(e);
            }
        } 

        public boolean isMerged() {
            try {
                return GHPullRequest.this.isMerged();
            } catch (IOException e) {
                throw sneakyThrow(e);
            }
        } 

        // Using lombok the method above would be even simpler
        // https://projectlombok.org/features/SneakyThrows
        //
        // @SneakyThrows
        // public boolean isMerged() {
        //     return GHPullRequest.this.isMerged();
        // } 

    // ... More unchecked methods as needed. 

    }
}

It's a bunch of new code, but it is "pay-as-you-go" and the code itself is extremely simple. As you encounter methods that are throwing exceptions that you think should not, you can create the new class and add methods. On the regular classes we add a toUnchecked() method that returns the unchecked wrapper.

This solution could result in the above code looking like this:

            .stream()
            // Convert to unchecked wrapper
            .map(pr -> pr.toUnchecked())
            .filter(pr -> !pr.isMerged())
            .filter(pr -> !pr.isDraft())

I'm not loving the term "unchecked". Maybe "streamable" or "functional"? Other that that, I think this could work really well.

What do you think?

New major version would need to be more, but not a lot more.

As to a new major version, I haven't thought to deeply about it. That's why I said "talk about". I'm not set on introducing new major features, but if we are going to do a breaking change, we should at least look at other cleanup tasks to include in there.

Other changes to consider:

• Remove all code marked deprecated.
• Remove all current bridge methods.

There's a couple other API restructuring tasks that I'd want to try to include as well.
But all this would still need to be non-breaking to the current API. I'd want to basically start a new package, maybe call it org.kohsuke.github-api.v2 and add a parallel implementation.

[Haarolean]

Forking = merge-hell

Sure, I understand that. That's why whenever I have a problem I'm trying to report it in upstream.

Non-breaking AND stream-friendly

Sounds nice from a consumer perspective, but having to add dozens of methods for dozens of classes would require a lot of manual work again and a lot of copy-paste (for example, GHIssue has 48 public methods).
Not sure what do you mean by "pay-as-you-go" in this case, it's either all or nothing, shouldn't this be like that? Otherwise it's a half-baked solution.

New major version would need to be more, but not a lot more.

If we agree to limit the scope to removing bridge methods, deprecated APIs and gettind rid of the checked exceptions, that sounds like a thing I could do. Want me to try raising a PR?

[bitwiseman]

Forking = merge-hell

Sure, I understand that. That's why whenever I have a problem I'm trying to report it in upstream.

👍

Non-breaking AND stream-friendly

Sounds nice from a consumer perspective, but having to add dozens of methods for dozens of classes would require a lot of manual work again and a lot of copy-paste (for example, GHIssue has 48 public methods). Not sure what do you mean by "pay-as-you-go" in this case, it's either all or nothing, shouldn't this be like that? Otherwise it's a half-baked solution.

It's "pay-as-you-go" because you only do it for classes and fields that you need - sure, you can do all 48 public methods, but I bet you don't use all 48 methods in streams. Only implement the methods you care about.

Yes, its copy-paste code, but it is dead simple code (as shown above). It's so simple we might even be able to generate it automatically. And testing it would be about as easy.

Half-baked solution? I'd agreed that it is a "partial" or "less than ideal" solution, but it is workable. I'm looking for a way to unblock the scenario you care about within the current constraints.

Side note: there are some methods where the exceptions are meaningful. delete() can throw a IOException that consumers probably do want to be able catch. What should we do in that case? Force people to catch "Throwable"? If this question is already answered somewhere, just point me there.

New major version would need to be more, but not a lot more.

If we agree to limit the scope to removing bridge methods, deprecated APIs and gettind rid of the checked exceptions, that sounds like a thing I could do. Want me to try raising a PR?

Hold off for the moment. I'm trying to decide whether to go with #1742 (multi-module) or if we want to branch for v2. I'm leaning towards branching, but I also want to make v2 use a separate package name like org.kohsuke.github2 so the two versions could be run side-by-side (for a non-breaking migration path, they would not depend on each other or interact in any way). Branching would mean having to back/forward port PRs, but would also give us good reason to not do that extra work.

[Haarolean]

Side note: there are some methods where the exceptions are meaningful

For such cases, there's a pretty common (I think?) way to handle this -- catching checked exceptions and throwingUncheckedIOException counterpart right away.

Branching would mean having to back/forward port PRs, but would also give us good reason to not do that extra work.

As we got it released, please let me know once you're ready to continue discussing this.

[bitwiseman]

@Haarolean
I've started a main-2.x branch that you can work in.

Here's a PR that removes a bunch of deprecated code paths (but not all): #1757

Could you try doing a spike PR showing an example of what the unchecked code will look like and how it will behave?

[bitwiseman]

@Haarolean

Side note: there are some methods where the exceptions are meaningful

For such cases, there's a pretty common (I think?) way to handle this -- catching checked exceptions and throwingUncheckedIOException counterpart right away.

We have GHException that inherits from RuntimeException, but then we also have GHIOException which inherits from IOException. So, for v2 we make GHIOException inherit from UncheckedIOException instead. I'm definitely warming to this idea.

[bitwiseman]

After all this discussion, I did some more reading as part of moving to release v2.0. It appears removing checked exceptions doesn't break binary compatibility.