nit: We have one emoji on each header. Maybe we should keep that uniform style?

Up to you :). I don't mind either way.

Sorry if I came late to the game here. Just wanted to drop my opinion on the table.

I believe the library-no-op variant of the interceptor should not edit the request at all. I'm afraid this is going to create a precedent.

I really like the idea of having a Skip-Chucker-Interceptor special header, but it should be developer's duty to make sure it's applied only on debug requests and doesn't leak. We can provide a simple interceptor that will remove Chucker headers if on BuildConfig.DEBUG.

Ok. But is there any point in stripping the header at all in this case? If header is not removed in the release builds it for sure does not matter if it is removed in debug and qa builds.

Maybe this would suggest that the stripping of header should be left to the consumers if they want to clean their requests before sending them to a server? An additional interceptor as a part of the library might be an option but I think it would have to be part of both library flavors.

We would just have to document to the users that this interceptor must be applied after ChuckerInterceptor for this feature to work.

Or it could be a configuration part of the ChuckerInterceptor that defaults to false.

Yeah I think we're having usability on one side vs security on the other. I do understand that the current implementation (that I really like btw) is the best from the developer point of view.

On the other hand this implementation leaked implementation details inside the library-no-op. By its nature, the -no-op library should do nothing at all. This is not the case anymore.

Maybe this would suggest that the stripping of header should be left to the consumers if they want to clean their requests before sending them to a server?

That could be an option absolutely.

An additional interceptor as a part of the library might be an option but I think it would have to be part of both library flavors.

Unfortunately yes.

I was rather thinking about the user as they do not have to apply two interceptors this way but I totally get your point and will make a follow-up PR with an additional interceptor. I just want to clarify the point about header values first.

Same as the other comment, those two constants should be "" to make sure those don't leak in the Release APK

I don't think it would be the best user experience as users would have to be very mindful of how they use Chucker.

fun main() {
  try {
    Retrofit.Builder()
      .baseUrl("https://www.exmaple.com")
      .validateEagerly(true)
      .build()
      .create<Service>()
  } catch (e: Exception) {
    e.printStackTrace()
  }

  try {
    Request.Builder()
      .url("https://www.example.com")
      .addHeader("", "true")
      .build()
  } catch (e: Exception) {
    e.printStackTrace()
  }
}

interface Service {
  @GET("/") @Headers("") fun get()
}

This would result in exceptions.

java.lang.IllegalArgumentException: @Headers value must be in the form "Name: Value". Found: ""
    for method Service.get
	at retrofit2.Utils.methodError(Utils.java:53)
	at retrofit2.Utils.methodError(Utils.java:43)
	at retrofit2.RequestFactory$Builder.parseHeaders(RequestFactory.java:282)
	at retrofit2.RequestFactory$Builder.parseMethodAnnotation(RequestFactory.java:235)
	at retrofit2.RequestFactory$Builder.build(RequestFactory.java:171)
	at retrofit2.RequestFactory.parseAnnotations(RequestFactory.java:67)
	at retrofit2.ServiceMethod.parseAnnotations(ServiceMethod.java:26)
	at retrofit2.Retrofit.loadServiceMethod(Retrofit.java:192)
	at retrofit2.Retrofit.validateServiceInterface(Retrofit.java:179)
	at retrofit2.Retrofit.create(Retrofit.java:134)
	at MainKt.main(Main.kt:33)
	at MainKt.main(Main.kt)

java.lang.IllegalArgumentException: name is empty
	at okhttp3.Headers.checkName(Headers.java:269)
	at okhttp3.Headers$Builder.add(Headers.java:323)
	at okhttp3.Request$Builder.addHeader(Request.java:196)
	at MainKt.main(Main.kt:22)
	at MainKt.main(Main.kt)

Removing headers from the no-op library (and effectively from the core library) makes it a burden for the users to use this feature as they have to declare them in the project by themselves and need to keep track of implementation details changes.

Yeah, it should be a developer's duty to make sure that you add the header if on Debug, and do not add it at all on release.

Why though? It makes it really hard to use it with Retrofit as headers are added in annotations. Also not that pleasant with OkHttp if it is not Android module. I can see removing public header constants from both library flavors but not from one because it gets really confusing for the end users otherwise.

I'm confused to be honest. I also do not see a point in designing this around Retrofit. Solution with headers is designed around OkHttp and not Retrofit. Retrofit just allows to use it due to its configurability. It would look the same for any other network library that uses OkHttp. How the header is the delivered to that library is up to that library.

This feature couldn't be implemented for Retrofit with a CallAdapter. CallAdapters are called before application interceptors and they do not modify content of requests. The best they can do is to inspect or modify responses.

I don't feel discredited or anything. I just don't understand what should be done here and why :). From what I understood the expected implementation should be like this.

• ChuckerInterceptor just reacts to the header but does not strip it.
• Some other interceptor (i.e. ChuckerStrippingInterceptor) should be available in both library versions and deal with removing the header.
• The header should not be public and just documented.

I just don't understand what should be done here and why

Sorry, I was definitely not clear. Also I just wanted to drop my view here. I'm not asking for any change (given that this was already merged).

Given that the solution was implemented with custom headers, ChuckerInterceptor should behave in this way from my point of view:

• On Debug: if the custom header is there, strip it, and skip the rest of the ChuckerInterceptor processing.
• On Release: nothing, just pass over the request.

It's a developer's responsibility to make sure that the header is applied only on Debug build (i.e. use it at your own risk).

If the developer is using OkHTTP Request directly, this is trivial as:

if (BuildConfig.DEBUG) {
    build.addHeader(Chucker. SKIP_INTERCEPTOR_HEADER_NAME, "true")
}

If the developer is using Retrofit, this becomes not trivial as you can't selectively remove a Header when on Debug.

To solve this scenario we could either:

• Provide a warning on the README about this situation.
• Provide a sample interceptor in a snippet in the README that will strip out the extra header.
• Provide a ChuckerStrippingInterceptor in a new module that will have to be applied as implementation dependency (both for debug and release), and should be applied after ChuckerInterceptor.

The header should not be public and just documented.

I'm not sure I follow you here.
The header should be a public field in the Debug version, and empty string in Release imho.

This feature couldn't be implemented for Retrofit with a CallAdapter.

Yup you're right. I was thinking about an alternative solution using custom annotations on the Retrofit side, but turns out is not that easy.

Anyway before we start coding, I'd love to get your point of view on this and find an agreement 👍 I don't want to impose my vision here.

Ok, so this is what I'm thinking. Personally, I don't see a problem with a no-op library doing cleaning but I totally understand your concerns about it and this is perfectly valid point.

As far as dealing with it I would just put a snippet and release a separate library for stripping headers. I have a library that would benefit from it as well so this would help there. I can make a release next week of this small stripping interceptor unless you want to have it as a part of ChuckerTeam. I don't mind either way, just don't want to duplicate libraries. :)

The point about headers is really not clear to me. I don't know why Chucker.SKIP_INTERCEPTOR_HEADER and Chucker.SKIP_INTERCEPTOR_HEADER_NAME should have empty values in the no-op flavor. While I understand that devs should be aware of what they apply and where, and I'm 100% with you on this, headers being empty strings is a bad thing in my opinion.

One thing is mentioned interaction with Retrofit (and most likely overwhelming amount of Chucker users are Retrofit users). Another thing is that is also not that easy to use with OkHttp. When I write my apps I put network layers in JVM modules which do not have access to BuildConfig. I could make same Gradle magic and generate this. I could inject information whether I'm in a debug mode or not. I could also inject an OkHttp client that does not have ChuckerInterceptor attached to it.

The best alternative is to define a top level static field that mimics what is in the library. But I would have to do this in every new project, be mindful of changes to this value and so on. It seems rather silly to me given that I just want not to use a QA tool for a specific network request. If I don't want this header to reach the server that's what stripping interceptor is for.

While I understand that devs should be aware of what they apply and where, and I'm 100% with you on this, headers being empty strings is a bad thing in my opinion.

I see your points about passing empty headers + not being able to easily apply the header only on debug builds, and they're totally valid. My point was related to not having implementation details (Strings) bleeding in the -no-op library, but it makes sense to drop it in this case 👍 thanks for clarifying.

I can make a release next week of this small stripping interceptor unless you want to have it as a part of ChuckerTeam.

As for this, I'd love to get @vbuberen point of view as well.
I'm also happy to collect other feedbacks from the community here.

Ok, joining the discussion here. I didn't think about concerns you have @cortinico and it seems that made a a decision too fast about merging this PR. My suggestion is to revert this change for 3.2.0 and return it when we have a better implementation.

As to additional interceptor suggested by @MiSikora - in case we are with this solution I would prefer to have it in ChuckerTeam so it will have more credibility for users.