search

pCloud / pcloud-sdk-java

The official pCloud Java SDK repository
https://pcloud.github.io/pcloud-sdk-java/
Apache License 2.0
39 stars 14 forks source link

Add the option to use custom interceptors #14

Closed SailReal closed 3 years ago

SailReal commented 3 years ago

As a user of this SDK it would be awesome if you could use e.g. your own logging interceptor.

georgi-neykov-hub commented 3 years ago

@SailReal I cannot comprehend how the addition of logging HTTP requests made internally by the SDK would help anybody but the SDK's developers themselves.

I am against exposing the addition/removal of OkHttp's interceptors just to be able to log HTTP traffic, as it opens the way for rewriting requests/responses as anyone pleases. That includes the plugging of a cache for downloaded files in a very convenient, but completely inappropriate place.

All that the SDK should be doing is to do the heavy lifting and reduce the initial effort of calling pCloud's API though HTTP on the JVM platform, nothing more, nothing less. It was never meant to cover all possible uses cases. On top of that there already is another library for doing RPC with pCloud's API, being much more flexible and easy to be tailored to one's needs.

If there's a real-life use case of logging the internal HTTP calls made by the SDK a pull request on the topic would be gladly accepted and gets played out in way close the following:

  1. Introduce an optional single-method interface / lambda which is used to delegate logging, can be named like Logger or something similar.
  2. Introduce an enumeration class that controls the logging levels (none, basic, full).
  3. Add an optional method in ApiClient.Builder that accepts a logging delegate and logging level enum that allows for logging.
  4. Implement the necessary plumbing in the ApiClient's implementation, possibly adding square's logging interceptor implementation.
SailReal commented 3 years ago

@SailReal I cannot comprehend how the addition of logging HTTP requests made internally by the SDK would help anybody but the SDK's developers themselves.

In my opinion, logging requests and responses is absolutely necessary to be able to localize and understand errors reported by users. The other way around, I don't understand what should speak against such a clear and meaningful log, you know exactly what is communicated between client and server:

D/OkHttp: --> GET https://eapi.pcloud.com/stat?path=/133324/masterkey.cryptomator http/1.1 (unknown length)
D/OkHttp: User-Agent: pCloud SDK Java unspecified
D/OkHttp: --> END GET
D/OkHttp: <-- 200 OK https://eapi.pcloud.com/stat?path=/133324/masterkey.cryptomator (32ms)
D/OkHttp: Server: CloudHTTPd-API v1.1
D/OkHttp: Date: Tue, 30 Mar 2021 13:18:41 GMT
D/OkHttp: ETag: "BKXeaJ6OOvkrqVd5xhhyT5mcCzLX"
D/OkHttp: Cache-Control: private, max-age=0
D/OkHttp: Vary: Accept-Encoding
D/OkHttp: Connection: keep-alive
D/OkHttp: Keep-Alive: timeout=1800
D/OkHttp: <-- END HTTP
D/OkHttp: --> GET https://eapi.pcloud.com/getfilelink?path=/133324/masterkey.cryptomator http/1.1 (unknown length)
D/OkHttp: User-Agent: pCloud SDK Java unspecified
D/OkHttp: --> END GET
D/OkHttp: <-- 200 OK https://eapi.pcloud.com/getfilelink?path=/133324/masterkey.cryptomator (32ms)
D/OkHttp: Server: CloudHTTPd-API v1.1
D/OkHttp: Date: Tue, 30 Mar 2021 13:18:41 GMT
D/OkHttp: ETag: "4OnpLJPO6amcHqDyzk1YpbBhU4AX"
D/OkHttp: Cache-Control: private, max-age=0
D/OkHttp: Vary: Accept-Encoding
D/OkHttp: Connection: keep-alive
D/OkHttp: Keep-Alive: timeout=1800
D/OkHttp: <-- END HTTP
D/OkHttp: --> GET https://evc85.pcloud.com/dpZU4OgmkZvgLfgXZExOkZZjXnOG7Z2ZZtzZZjwOQ5C2cSCfH5SewyVmC98W11HSy/masterkey.cryptomator http/1.1 (unknown length)
D/OkHttp: User-Agent: pCloud SDK Java unspecified
D/OkHttp: --> END GET
D/OkHttp: <-- 200 OK https://evc85.pcloud.com/dpZU4OgmkZvgLfgXZExOkZZjXnOG7Z2ZZtzZZjwOQ5C2cSCfH5SewyVmC98W11HSy/masterkey.cryptomator (641ms)
D/OkHttp: Server: CloudHTTPd v1.1
D/OkHttp: Date: Tue, 30 Mar 2021 13:18:42 +0000
D/OkHttp: Etag: "dfe86c5f4d9281532752a6e1bec2dfa1810a30e4"
D/OkHttp: Last-Modified: Tue, 16 Mar 2021 16:37:04 +0000
D/OkHttp: Expires: Tue, 30 Mar 2021 19:18:41 +0000
D/OkHttp: Content-Disposition: attachment; filename="masterkey.cryptomator"
D/OkHttp: Accept-Ranges: bytes
D/OkHttp: Content-Transfer-Encoding: binary
D/OkHttp: Connection: keep-alive
D/OkHttp: Keep-Alive: timeout=30
D/OkHttp: <-- END HTTP

I am against exposing the addition/removal of OkHttp's interceptors just to be able to log HTTP traffic, as it opens the way for rewriting requests/responses as anyone pleases. That includes the plugging of a cache for downloaded files in a very convenient, but completely inappropriate place.

Not letting a user of this SDK provide interceptors as desired does not prevent it, as this SDK can be forked and added there accordingly. But since I'm indeed only interested in logging, I'll think about implementing the Logger as you mentioned or using a fork to save me the additional work (currently have only little time and logging seems anyway not really wanted).

georgi-neykov-hub commented 3 years ago

In my opinion, logging requests and responses is absolutely necessary to be able to localize and understand errors reported by users.

I cannot agree more to that, logging is needed by developers when investigating and debugging errors, not by the end users of the SDK.

The other way around, I don't understand what should speak against such a clear and meaningful log, you know exactly what is communicated between client and server:

I love OkHttp's logs, but this is internal logic, the SDK primary purpose is make things easy for developers and free them from doing the HTTP plumbing by themselves. That being said, the API is documented and is open to be used by everyone as they find suit, minus abuse, of course.

Not letting a user of this SDK provide interceptors as desired does not prevent it, as this SDK can be forked and added there accordingly.

The SDK is open-sourced via a permissive license and as a contributor I would be happy if gets forked and modified to suit someone's needs. I am being against adding the interceptors not because I am trying to stop them from being added in someone's fork. I want to prevent them from being added to the officially supported repository as it means an additional cases, complexity and potential problems to users which need time and care by someone from the staff.

OkHttp's interceptors are an advanced topic and require care and effort to be properly implemented outside the trivial cases of logging. If someone desperately needs to have API requests/responses intercepted I would suggest to walk the way and setup their own OkHttp + Retrofit pipeline, or make additions to their existing one to have the calls they need.

SailReal commented 3 years ago

It's probably just my taste especially because our app supports several cloud providers and I really like the level of detail in these Okhttp logs to find out if it's a problem of the user using our app, we're not using a cloud API correctly or the server has a problem...but you're right this taste doesn't apply to every user of your SDK and this repo should address these people and not my special use case.

I will close the PR because of the conclusion above. Thanks for the good discussion and providing this open source SDK!