Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose additional BoxAPIRequest/BoxAPIConnection members for request intercept use case #488

Open
timvossen opened this issue Nov 29, 2017 · 3 comments
Labels
enhancement Added to issues that describes enhancements

Comments

@timvossen
Copy link

In a Box SDK integration, I implemented a com.box.sdk.RequestInterceptor that uses its own HTTP client to send the requests over the network. Doing that, I noticed that in order to support this scenario, certain members should, in my opinion, be exposed to clients outside the SDK:

  • com.box.sdk.BoxAPIRequest.getHeaders() should be public so that the corresponding content is accessible
  • com.box.sdk.BoxAPIRequest.shouldAuthenticate should be exposed via a getter to distinguish the two cases that it indicates

In order to hold a read lock on the access token during the request (as com.box.sdk.BoxAPIRequest.trySend(ProgressListener) does), it would be necessary to also expose

  • com.box.sdk.BoxAPIConnection.lockAccessToken() and
  • com.box.sdk.BoxAPIConnection.unlockAccessToken().

The access token can be obtained with the (already public) method com.box.sdk.BoxAPIConnection.getAccessToken(), but in this case there is no lock and the access token could be invalidated before the HTTP request is sent (or while it is being sent).

@carycheng carycheng added the enhancement Added to issues that describes enhancements label Jan 5, 2018
@mattwiller
Copy link

Hey @timvossen — could you tell us a bit more about your use case here? It would help us to know exactly what you're trying to do with the request interceptor so we can prioritize and evaluate the necessary access changes.

@timvossen
Copy link
Author

@mattwiller
The use case was to send the actual HTTP requests via an Apache HTTP client instead of the HttpURLConnection that is built into the SDK. This leads to increased robustness regarding connection resets and read timeouts, as that HTTP client can do retries in those scenarios.

It was my understanding that this use case should be supported for implementations of RequestInterceptor.

@mattwiller
Copy link

@timvossen I'm not totally sure if that's a use case we'll be able to support at the moment — it seems a bit strange to make essentially the entire BoxAPIConnection object public so that an end user could basically reimplement the core request logic of the SDK. Both lockAccessToken() and unlockAccessToken() are really implementation details of the connection, which should probably not be exposed.

As far as I can tell, the RequestInterceptor was never intended to solve this sort of use case — which is why it runs entirely before the lock on the access token is grabbed by the BoxAPIRequest. The whole idea, as best I can tell from the documentation, was to either modify or inspect the request, and optionally return a different response instead of contacting the Box API. Your use case, as stated, seems out of line with what the feature is designed to provide. I'll add better support for this use case to our backlog for the next major version of the SDK so we don't lose track of it, but I don't think it's something we'll be able to support well today.

In any case, the current version of the SDK should be retrying on connection errors and read timeouts — can you share a bit more about what problems you're seeing around those so we can see if there's a better way for us to address it in the SDK?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Added to issues that describes enhancements
Projects
None yet
Development

No branches or pull requests

3 participants