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

exim: Use har-reader #5250

Merged
merged 1 commit into from
Jul 10, 2024
Merged

exim: Use har-reader #5250

merged 1 commit into from
Jul 10, 2024

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Jan 26, 2024

Overview

  • CHANGELOG > Added change note.
  • build file > Updated/added dependencies.
  • HarImporter > Re-worked to use har-reader lib.
  • HarUtils > Added to to facilitate use of the new lib. Contains a number of methods that still need to be re-worked for writing.
  • MenuImportHar > Re-worked and simplified to take advantage of the new utils, importer, and lib.
  • HarImporterUnitTest > Tweaked to accommodate HarImporter changes.

Related Issues

Checklist

  • Update help
  • Update changelog
  • Run ./gradlew spotlessApply for code formatting
  • Write tests
  • Check code coverage
  • Sign-off commits
  • Squash commits
  • Use a descriptive title

@kingthorin
Copy link
Member Author

Removed the inner classes, still need to tweak the HTTP Version handling.

@kingthorin
Copy link
Member Author

I've adjusted the version handling now. I believe this now has all the previously discussed changes.

@thc202
Copy link
Member

thc202 commented May 21, 2024

I'd do a final review later in the day.

@kingthorin
Copy link
Member Author

I think I got all the comments, just need to add tests. Please lemme know if I've missed something.

@kingthorin kingthorin force-pushed the exim-har-reader branch 2 times, most recently from 092c58a to bd49ab6 Compare May 23, 2024 01:05
@kingthorin
Copy link
Member Author

I started to add tests. There were a number of challenges. I'll add some comments to cover those.

The tests will fail, they succeed individually but there seems to be some sort of concurrency or cleanup issue when run together.

@thc202
Copy link
Member

thc202 commented May 23, 2024

#5250 (comment)

The logger should be set up for each test not all of them, also better to call Configurator.reconfigure(getClass().getResource("/log4j2-test.properties").toURI()); to reset the configuration to previous state. (To be clear that does not fix the test failures, there are still some tests that need further changes, like setting up some more mocking and the messages.)

@kingthorin kingthorin force-pushed the exim-har-reader branch 2 times, most recently from 67a8efc to 98d875f Compare May 23, 2024 10:35
@thc202
Copy link
Member

thc202 commented May 23, 2024

I was not clear with #5250 (comment) I meant that the logger should be initialised in a BeforeEach rather than BeforeAll, sorry.

@kingthorin
Copy link
Member Author

Okay I've updated a bunch of stuff. Things are a bit better. The log handling was ripped off from GraphQL tests. I had to put your reconfigure suggestion in each method because AfterAll is static and didn't like me using getClass (and I couldn't figure out an alternative to getClass).

Using mockMessages didn't seem to fix the console barf, the key is there so I don't get the issue....

Can you think of another place we've done mocking to handle persisting messages? I think I recall talking about this for another PR but I can't figure out what it was.

@thc202
Copy link
Member

thc202 commented May 23, 2024

The log reset should be done after each test.

Let me look.

SpiderTaskUnitTest

@kingthorin kingthorin force-pushed the exim-har-reader branch 2 times, most recently from 1961879 to 4279b38 Compare May 23, 2024 11:08
@kingthorin
Copy link
Member Author

Getting closer... 🤷‍♂️

@kingthorin
Copy link
Member Author

Some of those comments might be ordered poorly, I didn't realize I hadn't finished my comments/review on Friday

@kingthorin kingthorin force-pushed the exim-har-reader branch 4 times, most recently from 0b0b2b7 to 320a127 Compare June 12, 2024 21:06
@kingthorin
Copy link
Member Author

I dropped the header pre-processing.

The examples I had only impacted responses. Not sure if I should handle requests similarly?

@thc202
Copy link
Member

thc202 commented Jun 13, 2024

I'm good if done only to responses.

@kingthorin
Copy link
Member Author

I think/hope this is ready for review. I moved the remainder of the core HarUtils code here and commented it out. I'll work on writing/export in another PR.

@thc202
Copy link
Member

thc202 commented Jul 6, 2024

I don't think we should be copying commented code that applies to the older library, why not just apply the changes when needed instead?

@kingthorin
Copy link
Member Author

Okay, I can just add a TODO if preferred

@thc202
Copy link
Member

thc202 commented Jul 8, 2024

Just a TODO is better IMO.

@kingthorin
Copy link
Member Author

Done & done

@kingthorin
Copy link
Member Author

Done

@thc202
Copy link
Member

thc202 commented Jul 8, 2024

Thank you!

@kingthorin kingthorin requested a review from psiinon July 8, 2024 18:17
@kingthorin kingthorin force-pushed the exim-har-reader branch 4 times, most recently from 387205e to fb40f89 Compare July 9, 2024 17:05
- CHANGELOG > Added change note.
- build file > Updated/added dependencies.
- HarImporter > Re-worked to use har-reader lib.
- HarUtils > Added to to facilitate use of the new lib.
- MenuImportHar > Re-worked and simplified to take advantage of the new
utils, importer, and lib.
- HarImporterUnitTest > Tweaked to accommodate HarImporter changes.

Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@psiinon psiinon merged commit 7927e4d into zaproxy:main Jul 10, 2024
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

HAR import fails silently
3 participants