-
-
Notifications
You must be signed in to change notification settings - Fork 341
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
Add support for HTTP Archive (HAR) #696
Conversation
Sorry for the radio silence. I'm currently preoccupied with life stuff. I should have more time soonish to take a look at this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great 🚀 Thanks for taking the time to do this. I've left several comments. Sorry for taking longer to review.
I'd also like other maintainers to chime in if possible.
Also as a side note for the future: the diff size was quite big. Having something smaller (like 2/3 PRs instead) would have speeded up the whole process.
library/src/main/java/com/chuckerteam/chucker/internal/data/har/Log.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/har/log/Entry.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/har/Log.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/har/log/Entry.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/har/log/entry/Cookie.kt
Outdated
Show resolved
Hide resolved
library/src/test/java/com/chuckerteam/chucker/internal/data/har/EntryTest.kt
Outdated
Show resolved
Hide resolved
library/src/test/java/com/chuckerteam/chucker/internal/data/har/EntryTest.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionActivity.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/ui/MainActivity.kt
Outdated
Show resolved
Hide resolved
library/src/test/java/com/chuckerteam/chucker/util/TestTransactionFactory.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember there were some issues with usability, so these should be checked before merging.
library/src/main/java/com/chuckerteam/chucker/internal/data/har/log/Entry.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/har/log/entry/request/PostData.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/JsonConverter.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/TransactionDetailsHarSharable.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/ui/MainActivity.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionActivity.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would leave the rest of review to @cortinico and @MiSikora as they already jumped in here and left quite a lot of feedback.
From my side the only request is to also update the CHANGELOG file to have this new feature mentioned.
Thanks a lot @gusdantas for picking up this feature request.
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/har/log/Entry.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/ui/MainActivity.kt
Outdated
Show resolved
Hide resolved
library/src/test/java/com/chuckerteam/chucker/util/HarTestUtils.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/support/ResponseProcessor.kt
Outdated
Show resolved
Hide resolved
One more thing that just caught my eye. Should we do anything special about headers? Chucker can redact them on demand. I think it's ok to show them as redacted in |
Sorry @MiSikora, I didn't get your concerns about the headers... Could you please give an example of possible issue if it's not too much trouble? |
The correct byte count for such a header is 179. However, when we redact it using I don't see it as a big problem because that is functionality that users willingly enable. Just something I'd like to get feedback on. |
Oh, now it is clear, thanks @MiSikora ! |
Under P.S. I would like to remind you about my request in one of previous comments:
|
@vbuberen actually I was thinking to pass the real byte count received, not the redacted one (in the example, it would be a "heuristic" to know that the field is fine, like, I cannot know what is in that field but I know it may be ok because it has the same size of an expected one). Nevertheless, if it is preferable to show the byte count redacted I should do this change.
|
The current implementation will show a redacted byte count. IMO this is not preferable. I will do some integration tests this weekend, and if everything is ok, it will be good to go from my side. |
Why is it prerrable to show the redacted byte count? From my point of view, redacting should just affect the "viewing" of the headers, but all the other properties should not be affected. I think not tweaking the byte count will be preferable and will introduce less confusion, but I'm interested to hear other opinion.
Awesome 🙏 Looking forward to it. |
A typo. I meant not preferable. That's why I even asked about it in the first place. 🙃 Nothing blocking though. |
Dears, |
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Outdated
Show resolved
Hide resolved
library/src/main/java/com/chuckerteam/chucker/internal/data/entity/HttpTransaction.kt
Outdated
Show resolved
Hide resolved
@MiSikora I added a test on |
You should be able to take it from the server. Something like |
@MiSikora thank you, now the test passes on both |
library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt
Outdated
Show resolved
Hide resolved
Left 1 NIT. I'll do tests tomorrow to see how it behaves, and hopefully, we're good to go. 👍 |
library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did some tests using Chucker both as an application interceptor and a network interceptor. For reading HAR files, I used this application - https://toolbox.googleapps.com/apps/har_analyzer.
Thank you for this contribution! I didn't find any issues, and everything worked as expected. After changing one last NIT and after @cortinico gives their approval, I think we can merge it.
Great work 🚀 Thanks for tackling this |
@cortinico @MiSikora @vbuberen I do appreciate all the compliments, but the main effort was made by @JayNewstrom ! |
📷 Screenshots
📄 Context
This updates @JayNewstrom 's solution which allows Chucker to share/export files with desktop http viewers such as Charles Proxy, Chrome Network Debugger, etc.
Closes #401
📝 Changes
Most of the code is new, but some changes were made on toolbars export buttons to allow har export.
🚫 Breaking
No breaking changes.
🛠️ How to test
@JayNewstrom 's solution has some automated tests. You can also test via the UI (see screenshots). After you have the file ready to import into another application, it should be able to be imported.
⏱️ Next steps
As told by @JayNewstrom: