Http -> HttpScheduler -> HttpCore

[amihaiemil]

We have the classes Http`{{, }}`HttpScheduler`{{ and }}`HttpCore.

It is my understanding that class Http is the highest abstraction and the API for making HTTP requests throughout the library.

HttpScheduler's Java Doc reads:

* HttpScheduler schedules HttpCore operations to an Executor, exposing a generic async API.
*
* Internal; use Http instead. <-- THIS!

and HttpCore's Java Doc reads:

HttpCore performs authenticated HTTP synchronously.
Internal; use Http or HttpScheduler instead. <-- THIS

So, an instance of Http`{{ encapsulates }}`HttpScheduler`{{ which in turn encapsulates and works with }}`HttpCore`{{. The last 2 are supposed to be "internal" yet they are present and used directly throughout the library, which means the architecture is a bit broken. These 2 classes should be **package-protected** (have the default access modifier instead of **public**) and only usable inside the package }}`io.ably.lib.http.

I had a look and the refactoring can be done punctually, 1 step at a time (no need to refactor the whole lib at once).

What do you think? I'd say it's better to have only a visible Http instance and avoid having more different ways of performing http calls throughout the codebase. This will also provide better decoupling.

┆Issue is synchronized with this Jira Task by Unito

[amihaiemil]

For instance, the constructor of Http could be redesigned to simply take an Auth and ClientOptions and instantiate HttpCore, AsyncHttpScheduler and SynHttpScheduler inside.

Http constructor now:

public Http(AsyncHttpScheduler asyncHttp, SyncHttpScheduler syncHttp) {
    this.asyncHttp = asyncHttp;
    this.syncHttp = syncHttp;
}

and current instantiation in AblyBase's constructor

public AblyBase(ClientOptions options) throws AblyException {
    ...
    auth = new Auth(this, options);
    httpCore = new HttpCore(options, auth);
    http = new Http(new AsyncHttpScheduler(httpCore, options), new SyncHttpScheduler(httpCore));
    ...
}

Redesigned Http constructor:

public Http(Auth auth, ClientOptions options) {
    this.asyncHttp = new AsyncHttpScheduler(
        new HttpCore(options, auth),
        options
    );
    this.syncHttp = new SyncHttpScheduler(
        new HttpCore(options, auth)
    );
}

and simplified AblyBase constructor:

public AblyBase(ClientOptions options) throws AblyException {
    ...
    http = new Http(new Auth(this, options), options);
    ...
}

[paddybyers]

and simplified AblyBase constructor ...

You're suggesting that AblyBase no longer needs a reference to auth ?

If it's easily done, then reducing the visibility of HttpCore and HttpScheduler would be good to do, but I don't want to generate a ton of work for something that's not causing us any issues.

[amihaiemil]

@paddybyers

I don't want to generate a ton of work for something that's not causing us any issues.

I understand. I'll spend some time digging in more detail (I'm not going to bill that) and let you know.

The point is that the http package is a little framework, as you said in the other ticket. And it should have (ideally) one public entry point (the Http class) for 2 reasons: decoupling and ease of usage (right now new developers might have no idea what to use: Http, HttpCore or HttpScheduler). And this kind of issue is easily overseen in Code Review.