search

ChuckerTeam / chucker

🔎 An HTTP inspector for Android & OkHTTP (like Charles but on device)
Apache License 2.0
3.99k stars 351 forks source link

Redesign HTTP transaction model #259

Open MiSikora opened 4 years ago

MiSikora commented 4 years ago

:warning: Is your feature request related to a problem? Please describe

Right now model for an HTTP transaction is a big structure that kind of serves as bag for everything. It makes it hard to work on new features that require new fields, different encoding format etc. Some links - #204, #202, #69.

:bulb: Describe the solution you'd like

I believe that there should be a separate table for transactions and different types of requests and responses. This way it would be easier to add new types of compression and so on.

This would require to not rely in UI layer on DB entities. They should be rather mapped to some common model (most likely a sealed class or an interface) that would be renderable on the view layer.

It would also allow to tidy a bit nullability of some fields. While request or response still might be null, some of their fields shouldn't be (like i.e. URL address).

:bar_chart: Describe alternatives you've considered

An interesting alternative would be to split HTTP transaction just into three tables (transaction, request, response) and store data as a blob which would be defined in a proto file. There should be no issue with backward compatibility as this is one of main advantages of protobuf. Additional benefit is that new tables would not appear for every new feature and only the proto schema would change.

:raising_hand: Do you want to develop this feature yourself?

I could work on it but it is rather a big change proposal that requires some discussion, design and approval beforehand. Not sure what the best way would be. This ticket, draft PR, Slack channel or maybe something else?

cortinico commented 4 years ago

If I read you correctly, you're suggesting we treat the HTTP entities as immutable. If so, I'm totally in for this.

Currently we do have a transaction table that stores a HTTP entity both for request and response and we update it once we collect the data. Ideally having separate tables will allow to make sure we treat those entities as immutable (with all the benefits of immutability).

This would require to not rely in UI layer on DB entities. They should be rather mapped to some common model (most likely a sealed class or an interface) that would be renderable on the view layer.

Can you elaborate further? Or provide an example (e.g. adding SSL/TLS information as done in #252). How that would look like in your suggestion.

Moreover, let's put Testing in the roadmap as we have much to improve on that side 👍

vbuberen commented 4 years ago

Moreover, let's put Testing in the roadmap as we have much to improve on that side 👍

I thought that we already agreed about it in Slack. On 21st of January I wrote such message for plans after 3.1.0:

MiSikora commented 4 years ago

If I read you correctly, you're suggesting we treat the HTTP entities as immutable. If so, I'm totally in for this.

Yes, this would be one thing to do but not the main one. I think that model should be separated into at least 3 tables. Transaction - with reference to a request and a response. Request with all the data for it. And Response with all the data for it.

One step further would be to divide it to stuff like GrpahQlResponse, ProtoResponse, ImageResponse, Multi-Part Response, PlainTextResponse and so on. Here is where I'm not sure what the best approach would be. Maybe it adds too much complexity. On the other hand it is easier to add new requests and responses as the model that represents it does not have to bloat with multiple nullable fields to conform to every transaction type.

Another option that I was thinking of is to not create multiple tables but one with just a blob as a field that would be proto encoded data.

This would require to not rely in UI layer on DB entities. They should be rather mapped to some common model (most likely a sealed class or an interface) that would be renderable on the view layer.

Can you elaborate further? Or provide an example (e.g. adding SSL/TLS information as done in #252). How that would look like in your suggestion.

Yes. Basically repositories would not return database entities but some concrete domain type that can be displayed in the UI. So there would be type (class or interface) like ChuckerTranscation that would have methods and fields that can be displayed on the UI level. It would require mapping from database classes to ChukcerTransaction type which would be responsibility of the repository.

ChuckerTransaction might be as simple as an ordered map or something more complex. Not sure really right now. Also, I can't envision right now how to make it highly configurable and not Android dependent. Easiest way might be to make it depend on the resources.

  • Add some tool to analyze coverage. Initially I thought about Codecov (https://codecov.io/), but now I think that Codacy (https://www.codacy.com/) might work better for us, since we could incorporate more quality checks and reports. Still would like to hear more feedback on this point.

Do you have any code coverage target in mind?

vbuberen commented 4 years ago

Do you have any code coverage target in mind?

I think we should start with some realistic and easy to achieve numbers like 60% and gradually bump it by 5-10% depending on availability of free time and amount of changes we might have.

vbuberen commented 4 years ago

Would like to return to this topic. I am all in for splitting our current model into Request and Response to avoid such god objects, which are hard to compare, etc. I am also not sure on what would be the best approach in case of splitting Response further, so can't give my opinion there at the moment.

MiSikora commented 4 years ago

To be honest, to make it simple, I would just divide and conquer and for now just split into three separate entities - Transaction, Request, Response with the same data that is currently present (maybe with more thought behind nullability of fields). After this step it will be easier to think of a next step and plan it when necessary.

cortinico commented 4 years ago

After this step it will be easier to think of a next step and plan it when necessary.

Agree on having this as an intermediate step. It will make things easier for us to handle and we can make this change incremental.

MiSikora commented 2 years ago

@vbuberen Can I assign myself to it? If you don't work on it, I think I could start working on it this or next month. I'd provide here my general plan on approaching it because there are some things I want to get feedback on.

vbuberen commented 2 years ago

Yes, feel free.

MiSikora commented 2 years ago

My plan for approaching it is as follows.

Introduce new persistence models. These models would be pure data classes, and I want to remove all logic like URL formatting, things specific to HAR, etc. They would use Room but with an in-memory database for now. I would use it as an initial step that would allow me to test everything gradually. I want to use OkHttp with custom converters for this layer. Some initial concepts are below (with omitted annotations).

internal data class HttpCall(
    val id: Long,
    val request: HttpRequest?,
    val response: HttpResponse?,
)

internal sealed interface HttpBody {
    data class Decoded(val value: String) : HttpBody
    data class Encoded(val value: ByteString) : HttpBody
}

internal data class HttpRequest(
    val id: Long,
    val url: HttpUrl,
    val method: String,
    val redactedHeaders: Headers,
    val unredactedHeadersByteSize: Long,
    val body: HttpBody?,
)

internal data class HttpResponse(
    val id: Long,
    val protocol: String,
    val body: HttpBody?,
)

After this is covered, I'd like to see what is needed to deliver the results to the UI. New persistence models should be enough for our purposes, and I don't anticipate a need for any separate presentation model (at least not for this task, maybe for #204). Firstly I'd map old models to new ones and integrate them with the UI. If that works, I'd swap old data providers with new ones, remove mappings, and hopefully remove legacy code.

I would most likely drop the LiveData in favor of Flow to handle issues with the database access itself - #456.

Doing this gradually would allow us to tackle the main issue, but also split ChuckerInterceptor tests into logical units, and most likely open the possibility to handle a couple of the other tasks like:

570

579

744

vbuberen commented 2 years ago

Sounds good to me.

cortinico commented 2 years ago

Looks good to me. The only thing that comes to my mind is:

val redactedHeaders: Headers, val body: HttpBody?,

I suppose we're going to write wrappers for those as well, right? Ideally let's get rid of okhttp3.Headers inside the data model: https://github.com/ChuckerTeam/chucker/blob/1f7d9074af2cc19cc159b04ef1fb7ecc91b334de/library/src/main/kotlin/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt#L15 and related as this is making hard to adapt the library to other HTTP libraries.

vbuberen commented 2 years ago

Random thoughts not directly connected with models redesign.

What about adding some pagination to our DB, so we would be pretty efficient in transactions list?

MiSikora commented 2 years ago

Looks good to me. The only thing that comes to my mind is:

val redactedHeaders: Headers, val body: HttpBody?,

I suppose we're going to write wrappers for those as well, right? Ideally let's get rid of okhttp3.Headers inside the data model:

I was actually thinking about reusing OkHttp types (though HttpBody is a custom type here). I can add our own types to at least start decoupling if you think it will add value. Personally, I don't see much benefit in it because I don't think that #224 is anywhere near on anyone's radar.

cortinico commented 2 years ago

Personally, I don't see much benefit in it because I don't think that #224 is anywhere near on anyone's radar.

Agree that there is no immediate value at this stage, but as we're redesigning the data model, having more decoupling will help tackle future scenarios. As the mobile space is quickly evolving (KMP and co.), I would do the extra effort to try to have a vanilla data model.

ArjanSM commented 2 years ago

Hi All, 👋 Really excited about this change. Willing to extend an extra hand to get this feature implemented. Let me know how can I help

cortinico commented 2 years ago

@vbuberen Can I assign this to me? I'd like to work on this a bit 👍

vbuberen commented 2 years ago

Yes, feel free to grab it.

cortinico commented 2 years ago

Providing a small update here as I've been working on this for long time now.

This is going to be a massive rework of the internals. I'm not sure I will be able to split into smallers PRs at first. I hope to be able to send a draft PR with the whole rework before the end of the year + if there is an agreement from @vbuberen I'll work on splitting it in some form afterwards.