Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

remove redundant intercept of httpclient with httpmethod #427

Closed

Conversation

theScrabi
Copy link
Contributor

@theScrabi theScrabi commented Aug 18, 2021

@theScrabi theScrabi linked an issue Aug 19, 2021 that may be closed by this pull request
@theScrabi theScrabi force-pushed the fix/remove_redundant_client_call branch from e521e75 to f02cb39 Compare August 19, 2021 09:50
@theScrabi theScrabi force-pushed the fix/remove_redundant_client_call branch from f02cb39 to 6738cc3 Compare August 20, 2021 14:09
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions and suggestions here @theScrabi

Comment on lines +26 to +27
var readTimeoutVal: Long? = null
var readTimeoutUnit: TimeUnit? = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion that could be out of scope from this PR.

We could create a new data class that receives the value and the TimeUnit and use it here. So we could replace 2 variables with just one, and we won't need nested ?.let to set the timeouts. Maybe a new issue to do this?

Comment on lines 38 to 39
retryOnConnectionFailure?.let { retryOnConnectionFailure(it) }
followRedirects?.let { followRedirects(it) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that they are not nullable, we can get rid of ?.let here, can't we?

Suggested change
retryOnConnectionFailure?.let { retryOnConnectionFailure(it) }
followRedirects?.let { followRedirects(it) }
retryOnConnectionFailure(retryOnConnectionFailure)
followRedirects(followRedirects)

@theScrabi theScrabi force-pushed the fix/remove_redundant_client_call branch from 3a47679 to 3e61fb0 Compare September 15, 2021 11:27
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 29, 2021

Let's QA a bit here...

@jesmrec
Copy link
Collaborator

jesmrec commented Sep 29, 2021

(1)

Redirections 301 do not work in comparison with master branch. Check out with server ocis.owncloud.works that points (301) to ocis.ocis-traefik.latest.owncloud.works.

Second request is where difference rises:

master && fix/okhttp_singleton :

12:02:49 HTTPS GET ocis.owncloud.works /status.php -> 301. (Location in response headers) ✅
12:02:55 HTTPS GET ocis.ocis-traefik.latest.owncloud.works /signin/v1/identifier/_/authorize?redirect_uri=oc%3A%2F%2Fandroid.owncloud.com&client_id=e4rAs... -> 302 ✅

fix/remove_redundant_client_call:

12:13:30 HTTPS GET ocis.owncloud.works /status.php -> 301 (Location in response headers) ✅
12:13:31 HTTPS GET ocis.ocis-traefik.latest.owncloud.works / -> 200 returning an html format in payload ❌

showing the error message: Malformed server configuration

You can check also with ocis-a11y.owncloud.works that points to ocis.a11y.owncloud.works

This chart summarizes the results:

ocis.owncloud.works ocis-a11y.owncloud.works
master Malformed server configuration
fix/okhttp_singleton Malformed server configuration
fix/remove_redundant_client_call Malformed server configuration Malformed server configuration

@theScrabi
Copy link
Contributor Author

Since the redirect got changed compleatly in favour of the ConnectionValidator, it may not make sense to fix this partially. We may need to do the checks @jesmrec did on that branch to.

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 11, 2021

Since the redirection fixes and the new behaviour is developed inside the ConnectionValidator scope, this branch and the singleton one should not be merged in order not to break the redirections, currently working in master. When ConnectionValidator goes on, then we can proceed with this.

Is this OK?

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 21, 2021

To resumen when https://github.com/owncloud/enterprise/issues/4201 is done and merged

@theScrabi theScrabi closed this Nov 12, 2021
@theScrabi theScrabi deleted the fix/remove_redundant_client_call branch November 12, 2021 11:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove redundant use of HttpClient
3 participants