Refactor `Requester` into multiple smaller classes #697

bitwiseman · 2020-02-13T05:24:04Z

Description

Refactoring from GitHub and Requester being monolithic centers of work to having the work split among smaller classes. GitHub is still the root of interactions, but holds a reference to a GitHubClient that has all the connection and communication. Requester still remains as a thin helper that will be removed in a future change.

Improvements

Abstraction/encapsulation of HttpUrlConnection interactions in GitHubHttpUrlConnectionClient
Builders and mostly immutable instances - better thread safety and clarity
Client/Request/Response model in line with modern Http frameworks
PagedIterable is the sole entry point for retrieval of all item arrays, lists, or iterables.
*Iterable and *Iterator classes encapsulate all the work done to retrieve pages of items

This change encapsulates most of the dependencies on HttpUrlConnection as much as possible, to allow moving to other more modern frameworks at some point. This also separates the concerns more clearly.

There's still some work to be done here, but this change is more than enough for now.

bitwiseman · 2020-02-13T06:16:32Z

src/main/java/org/kohsuke/github/Requester.java


 /**
- * A builder pattern for making HTTP call and parsing its output.
+ * A thin helper for {@link GitHubRequest.Builder} that includes {@link GitHubClient}.


This is the really the summary of this change - a refactoring from GitHub and Requester being monolithic centers of work to having the work split between GitHubClient, GitHubRequest, GitHubRequest.Builder, GitHubResponse, GitHubResponse.ResponseInfo, and various *Iterables and *Iterators. Requester still remains as a thin helper and GitHub is still large but is mostly customer facing methods with GitHubClient handling internals.

src/main/java/org/kohsuke/github/GitHub.java

… HttpUrlConnection

kshultzCB · 2020-02-16T20:35:04Z

src/main/java/org/kohsuke/github/GitHubRequest.java

+     */
+    @Nonnull
+    static URL getApiURL(String apiUrl, String tailApiUrl) throws MalformedURLException {
+        if (tailApiUrl.startsWith("/")) {


Is there a circumstance where tailApiUrl will not start with a "/"? I didn't see any tests in GitHubConnectionTest that behave this way, so just wondering how we'd run up against this, and what it looks like when we do.

Good question. There are a small number of cases where the API returns a URL that is then used for other things.

https://github.com/github-api/github-api/pull/697/files#diff-64dfb4dfbe21520e8735454a113c45a7R548-R560

https://github.com/github-api/github-api/blob/ff3136df7069d22d937daa8f2cddc357c777e07e/src/main/java/org/kohsuke/github/GHContent.java#L225-L227

https://github.com/github-api/github-api/blob/ff3136df7069d22d937daa8f2cddc357c777e07e/src/main/java/org/kohsuke/github/GHContent.java#L236-L240

Also, for paginated result, the full url for the next page is provided in the Link header , so when we call getApiURL we want to use that url, whatever it happens to be.

https://github.com/github-api/github-api/blob/dc33e28452532b94c6d46e435c192ae043849ca9/src/main/java/org/kohsuke/github/GitHubPageResponseIterator.java#L68-L77

I don't love this lack of clarity, but I didn't feel confident that I had tests for all cases where that URL didn't start with /, so I've kept the behavior where this method just handles it. That said, I should change L77 to use setRawUrl. Thanks!

kshultzCB · 2020-02-16T20:53:51Z

src/main/java/org/kohsuke/github/GitHubRequest.java

+        // by convention Java constant names are upper cases, but github uses
+        // lower-case constants. GitHub also uses '-', which in Java we always
+        // replace with '_'
+        return en.toString().toLowerCase(Locale.ENGLISH).replace('_', '-');


Comments like this are super helpful. :)

Are there other places where we might need to do such a conversion? I only see one instance of replace('_', '-') in the whole plugin. Might be fine, it just surprised me that there aren't more.

The other places are mostly done automatically by Jackson.

https://github.com/github-api/github-api/pull/697/files#diff-04099021093ec34af9d55a855cc1e6fdR85-R90

Hm, the toLowerCase may not be needed due to line 88. And line 89 means that, given a field like create_date, Jackson will automatically look for a field with the name createdDate and then fall back to create_date if it doesn't find one.

However, this setting was only added recently, there are still a lot private fields with underscores in them.

Opened #700 to clean this up.

kshultzCB · 2020-02-16T20:58:36Z

src/main/java/org/kohsuke/github/GitHubRequest.java

+            }
+            return (B) this;
+        }
+


This is a somewhat arbitrary place for me to put this question, but it's as good as any. There are a lot of with(thing, thing) methods here with different parameters. Why is that? I mean, I'm sure there's a perfectly good reason for it, I just am having trouble putting it together on my own.

I'm not sure. These were here when I arrived. I'm not sure it they are still all needed, per se. I think I tried to remove them at one point and started getting errors ... but I don't remember now.

Opened #699 to come back to this.

kshultzCB · 2020-02-16T21:14:34Z

src/main/java/org/kohsuke/github/PagedIterator.java

-
-            current = base.next();
-            wrapUp(current);
+        if ((current == null || current.length <= pos) && base.hasNext()) {


Not really specific to this PR, but, I could use a quick overview of the relationships between current, pos, and base. I thought I had it figured out for a minute but I guess I don't.

I can pick better names for these and give them a little more commenting.

kshultzCB

Mostly this is me making sure I actually understand what's going on in here. :)

bitwiseman · 2020-02-17T03:59:49Z

From my discussion of this with @kshultzCB - This is very much a refactor with no significant performance changes.

bitwiseman · 2020-02-17T05:34:19Z

@kshultzCB Thanks! Great feedback. Made updates to address and filed a couple of issue to track future work.

We don't need two layers of PageIterator just to get the final response. Also made iterators thread-safe. And added more detailed comments.

kshultzCB

Things are getting pretty desperate when I'm left to nitpick at comments. :) I love the additional clarity here, especially in PagedIterator, it's super helpful.

kshultzCB · 2020-02-17T21:24:11Z

src/main/java/org/kohsuke/github/GitHubPageIterator.java

+     * If {@link #next} is not {@code null}, no further action is need. If {@link #next} is {@code null} and
+     * {@link #nextRequest} is {@code null}, there are no more pages to fetch.


Pedantic documentation hat, engage: replace "need" with "needed."

kshultzCB · 2020-02-17T21:29:00Z

src/main/java/org/kohsuke/github/GitHubPageIterator.java

+     * {@link #nextRequest} is {@code null}, there are no more pages to fetch.
+     * </p>
+     * <p>
+     * Otherwise, a new response page is fetched using {@link #nextRequest}. The response is then check to see if there


Pedantic documentation hat once more: replace "check" with "checked."

These were not synchronized before we should leave them fix this in a future change

kshultzCB

+1 to leaving this as a refactor, and tabling the thread-safe idea for a different time.

bitwiseman force-pushed the tast/response-info branch 3 times, most recently from 62eb088 to 7591215 Compare February 13, 2020 06:10

bitwiseman commented Feb 13, 2020

View reviewed changes

bitwiseman changed the title ~~Tast/response info~~ Refactor Requester into multiple smaller classes Feb 13, 2020

bitwiseman added 9 commits February 12, 2020 22:42

Major rewrite of Requester

6b1258e

Test cleanup

bd68252

Created Client/Request/Response classes

8227683

Disable two tests due to spurious mocking failures

7310a70

Progress commit on moving to Client/Request/Response refactor

dd9245f

More refactoring

f6c75e1

Delete mocking test that is just too brittle to live

60c045a

Further clean up of refactored classes

ad45a74

JavaDocs and refactoring

90489e4

bitwiseman force-pushed the tast/response-info branch from 7591215 to 90489e4 Compare February 13, 2020 06:42

bitwiseman marked this pull request as ready for review February 13, 2020 07:27

Merge remote-tracking branch 'github-api/master' into tast/response-info

0c6959c

bitwiseman commented Feb 13, 2020

View reviewed changes

src/main/java/org/kohsuke/github/GitHub.java Outdated Show resolved Hide resolved

bitwiseman added 2 commits February 12, 2020 23:59

Update src/main/java/org/kohsuke/github/GitHub.java

9da4781

Create GitHubHttpUrlConnectionClient to encapsulate interactions with…

dc33e28

… HttpUrlConnection

kshultzCB reviewed Feb 16, 2020

View reviewed changes

bitwiseman mentioned this pull request Feb 17, 2020

Remove any unneeded with methods from Requester #699

Open

Address PR feedback

b7af635

bitwiseman mentioned this pull request Feb 17, 2020

Finish moving all full URL setting to setRawUrlPath and change withUrlPath to not require starting / #700

Open

bitwiseman added 2 commits February 17, 2020 12:55

Class clean up

acc5a89

We don't need two layers of PageIterator just to get the final response. Also made iterators thread-safe. And added more detailed comments.

Bump spotbugs to 4.0.0

dade4c4

kshultzCB approved these changes Feb 18, 2020

View reviewed changes

Revert synchronization in iterators

bd0e0cd

These were not synchronized before we should leave them fix this in a future change

kshultzCB approved these changes Feb 18, 2020

View reviewed changes

Comment tweaks

1db4fca

bitwiseman merged commit 9018d72 into hub4j:master Feb 20, 2020

bitwiseman deleted the tast/response-info branch February 20, 2020 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `Requester` into multiple smaller classes #697

Refactor `Requester` into multiple smaller classes #697

bitwiseman commented Feb 13, 2020 •

edited

Loading

bitwiseman Feb 13, 2020 •

edited

Loading

kshultzCB Feb 16, 2020 •

edited

Loading

bitwiseman Feb 17, 2020

kshultzCB Feb 16, 2020

bitwiseman Feb 17, 2020

bitwiseman Feb 17, 2020

kshultzCB Feb 16, 2020

bitwiseman Feb 17, 2020

bitwiseman Feb 17, 2020

kshultzCB Feb 16, 2020

bitwiseman Feb 17, 2020

kshultzCB left a comment

bitwiseman commented Feb 17, 2020

bitwiseman commented Feb 17, 2020

kshultzCB left a comment

kshultzCB Feb 17, 2020

kshultzCB Feb 17, 2020

kshultzCB left a comment

		* If {@link #next} is not {@code null}, no further action is need. If {@link #next} is {@code null} and
		* {@link #nextRequest} is {@code null}, there are no more pages to fetch.

Refactor Requester into multiple smaller classes #697

Refactor Requester into multiple smaller classes #697

Conversation

bitwiseman commented Feb 13, 2020 • edited Loading

Description

Improvements

bitwiseman Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

kshultzCB Feb 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshultzCB left a comment

Choose a reason for hiding this comment

bitwiseman commented Feb 17, 2020

bitwiseman commented Feb 17, 2020

kshultzCB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshultzCB left a comment

Choose a reason for hiding this comment

Refactor `Requester` into multiple smaller classes #697

Refactor `Requester` into multiple smaller classes #697

bitwiseman commented Feb 13, 2020 •

edited

Loading

bitwiseman Feb 13, 2020 •

edited

Loading

kshultzCB Feb 16, 2020 •

edited

Loading