Providing API version header from server to caller when available.

[OrangeAndGreen]

Attempts to safely retrieve the x-api-current-version header from API response payloads so calling code can do version checking.
https://dimagi.atlassian.net/browse/CCCT-239

cross-request: dimagi/commcare-android#2757

[shubham1g5]

is apiVersion still required here ?

[OrangeAndGreen]

This is how I planned to expose the value to the calling code. Specifically, after gets/posts in ConnectNetworkHelper I plan to pass the expected version into the getResponseProcessor call, and check it against the received value included in the processSuccess call here. Does that sound like the right approach?

[shubham1g5]

I see, I don't think we need to expose it specifically in processSuccess, instead implementations of processSucess can directly use the responseData to get apiVersion ?

Also I am confused if processSuccess is where callers will be handling apiVersion why are we adding public String getApiVersion() { in here and ModernHTTPTask ?

[OrangeAndGreen]

I think the difference is between the ResponseStreamAccessor and InputStream. The responseData passed up in processSuccess is an InputStream which does not include knowledge of the API version. The ResponseStreamAccessor on the other hand contains both the InputStream and access to the API version. So perhaps the best solution is to have processSuccess include the ResponseStreamAccessor instead of just the InputStream?

The other option I can see is to have code in processSuccess call back to the task to access the API version, but this feels a little odd since existing interaction with the task is mostly about setup rather than handling results. In other words, calling code has to know not to attempt to retrieve the API version until after the call has completed.

[OrangeAndGreen]

Maybe a better way to phrase my concern with the second approach: If ModernHttpTask is a ResponseStreamAccessor that allows us access to the InputStream and API string, shouldn't we be accessing both the same way?

So if we're going to pass the InputStream via processSuccess instead of calling myModernHttpTask.getResponseStream, I think the API string should be provided the same way.

[OrangeAndGreen]

Something to consider: perhaps instead of implementing ResponseStreamAccessor, both ModernHttpRequester and ModernHttpTask should create and return generic implementations of that interface when calling processResponse.

That way access to the response stream and API version would only be available in the callback (I'm going with the proposal to have processSuccess pass the ResponseStreamAccessor). In other words, it wouldn't be possible for calling code to attempt to access the response stream or API string from the task/requester before actually executing the request.

[shubham1g5]

That way access to the response stream and API version would only be available in the callback (I'm going with the proposal to have processSuccess pass the ResponseStreamAccessor). In other words, it wouldn't be possible for calling code to attempt to access the response stream or API string from the task/requester before actually executing the request.

This sounds like a good design choice to me although

perhaps instead of implementing ResponseStreamAccessor, both ModernHttpRequester and ModernHttpTask should create and return generic implementations of that interface when calling processResponse.

I am not super clear what you mean by this and having some pseudo code to look at might be helpful.

[OrangeAndGreen]

No problem. The generic ResponseStreamAccessor would be created when calling processResponse in ModernHttpTask and ModernHttpRequester, rather than returning this as the last function input.

For ModernHttpTask, the new code would look like:

The class would no longer implement ResponseStreamAccessor, and the getResponseStream and getApiVersion methods at the end of the file would be deleted.

And in places like this, it would no longer be possible to call postTask.getResponseStream before calling connect and executeParallel. Note that I currently don't see this happening anywhere in the code, but the potential is there.

[shubham1g5]

yes, that seems promising to me 👍

[OrangeAndGreen]

Implemented these changes

[shubham1g5]

the call response.headers().get("x-api-current-version") doesn't really throw an exception even if "x-api-current-version is not present, as such do we need the catch here ?

[OrangeAndGreen]

Makes sense, I removed it. I was being a little overly safe!

[shubham1g5]

Approving as don't want to block this further but left one comment to address in previous review.