A single flow for all outgoing HTTP requests

[tareksha]

phase 1 - builder, runner, all json requests

An injected interface, HttpRequestFactory, allows managing all outgoing
HTTP requests. HttpJsonRequest factory should use it internally. The
defult DefaultHttpJsonRequest as reusable as it accepts the general HTTP
factory in its constructor. Extensions can replace HttpJsonRequestFactory
and provide DefaultHttpJsonRequest as-is, given a different impl.

Signed-off-by: Tareq Sharafy [email protected]
Change-Id: I2f334b7bb133b3f136d2b8e5b18301602f427355

[tareksha]

@skabashnyuk @evoevodin please review

[garagatyi]

Can you explain why we still need old field, looks like it is unused.

[tareksha]

@garagatyi the old field is final protected, not private. i saw it too risky to just remove it or hide non-incrementally.

[garagatyi]

I didn't notice that, thanks.

[tareksha]

@garagatyi the change now uses the same builder pattern. please review

[skabashnyuk]

@tareqhs Quick response is -1 to make HttpJsonRequest something not related to json. We understand that you want to make some tooling to make general request but making it in HttpJsonRequest is not that we want to achieve. I hope @garagatyi @evoevodin can add more to this statement.

[voievodin]

Hi @tareqhs.
Generally speaking HttpJsonRequest was designed to work with json only, there are several reasons for that

• 90% requests are requests between our API services where json is a main format, so it fits almost all the needs
• when the tool is json specialized we can add useful methods related to DTO processing, which is also json based, so the tool becomes even more convenient and usable.

The idea of you changes is clear for me and makes total sense, but the implementation strategy should be different, as HttpJsonRequest becomes not really Json and the client of the class may be confused as the class contains general and json request logic mixed.

As for me the best solution here is to create general class like HttpRequest with all the general methods for requesting stuff through http and then change the current HttpJsonRequest implementation to use HttpRequest in this way the logic is clearly separated between components.

[tareksha]

hi @evoevodin @skabashnyuk ,

Well, that was the original approach in the PR but I tried to minimize code changes. The main issue is not whether to extend HttpJsonRequest itself or not, but rather the factory HttpJsonRequestFactory. I'm seeking a solution that doesn't require injecting a new factory everywhere yet again, which conceptually is a superset of HttpJsonRequestFactory.

This is my suggestion : HttpJsonRequestFactory extends a general HttpRequestFactory that has general request methods, so that the existing injections stay identical except for receiving the additional HTTP APIs that are in turn used instead of HttpURLConnection where needed.

Agree?