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

fix(multipart): accept multipart/form-data with boundary #1499

Merged
merged 19 commits into from
Jun 2, 2023

Conversation

aali309
Copy link
Contributor

@aali309 aali309 commented May 25, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Fixes: #1490

Description of the change:

Accept multipart forms with boundaries when creating automated rules

Motivation for the change:

This change is helpful because user may send requests with multiple Content-Type header entries including delimiters like semicolons ':' . The automated rule will not fail i.e the content type will discard anything that comes after multipart/form and the semicolon.

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. $ curl -vk -F name="firstRule" -F matchExpression="target.labels[\"app\"]==\"jfr-sb-app\"" -F eventSpecifier="template=Profiling,type=TARGET" https://localhost:8181/api/v2/rules -H 'Accept: application/json' -H 'Content-Type: multipart/form-data'

@aali309 aali309 changed the title added a test to accept MultiPartWithBoundary,edited settings, and add… fix(multipart): fixed the multipart/form part May 25, 2023
@aali309 aali309 requested a review from andrewazores May 26, 2023 13:27
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-e6f001003327999204db6e4fb02b497cdd5eddff sh smoketest.sh

@aali309 aali309 marked this pull request as ready for review May 26, 2023 13:52
.vscode/settings.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.pom.xml.swp Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-7be9d788ddf72f239b4a809a93084e8fce832b15 sh smoketest.sh

@andrewazores andrewazores changed the title fix(multipart): fixed the multipart/form part fix(multipart): accept multipart/form-data with boundary May 26, 2023
@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-3a32f649e1e87e9f5d389b251aef188af6249037 sh smoketest.sh

andrewazores
andrewazores previously approved these changes May 26, 2023
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

If you're interested, I can also walk you through how to add an integration test so we can be really sure that this is working as intended when the whole application is deployed and interacting over a real network stack with a real HTTP client.

@aali309
Copy link
Contributor Author

aali309 commented May 26, 2023

Looks great!

If you're interested, I can also walk you through how to add an integration test so we can be really sure that this is working as intended when the whole application is deployed and interacting over a real network stack with a real HTTP client.

Looks great!

If you're interested, I can also walk you through how to add an integration test so we can be really sure that this is working as intended when the whole application is deployed and interacting over a real network stack with a real HTTP client.

yes that will be great.

@andrewazores
Copy link
Member

In this case there is already an existing integration test exercising most of the same code, it just missed this particular case.

https://github.com/cryostatio/cryostat/blob/main/src/test/java/itest/RulesPostFormIT.java

Generally, just creating a new file in that directory with the name pattern SomethingIT.java is sufficient. The class should probably extend StandardSelfTest for convenience, but there are a couple of other options in the bases subdirectory.

When the unit tests run it is only testing the direct code under test, just the methods being called in the compiled bytecode. In the integration test harness an actual Cryostat server instance is brought up within a container, and there are tools for adding additional target applications for testing functionality. Rather than calling methods directly like in a unit test, you interact with the server using an HTTP client in a separate process, so it's a lot like the manual testing you have been doing with curl where the full stack is actually live and running.

@andrewazores
Copy link
Member

To build the server and run the integration tests you can do mvn clean verify. Once that has been built however, you don't need to rebuild the whole application every time you want to run the tests. bash repeated-integration-tests.bash will be useful. This will recompile just the integration tests, then spin up the already-built server container and run the tests against that server again.

Any requests you make to the server in the integration tests should be carefully cleaned up afterward, so that those changes do not affect tests that run later. So for example since your test will be attempting to create a new Automated Rule definition, you should also have an @AfterAll cleanup method that deletes that rule, so that when the next integration test class executes it does not find an additional unexpected rule or recordings that may have been created by it.

Once the integration test run finishes (whether tests pass or fail), the database, storage directories, etc. will be dropped, and the next integration test run will use fresh copies of everything. So at least in between test runs things are guaranteed to start from a clean slate, it is only in between individual tests or test classes that cleanup has to be done manually.

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-1b08585588a1f9cb43cf04b8cf9ae8dfd6c73253 sh smoketest.sh

@github-actions
Copy link
Contributor

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-1b08585588a1f9cb43cf04b8cf9ae8dfd6c73253 sh smoketest.sh

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-217d02dcde8ee07f8a87f676a04b276f7f2e47b5 sh smoketest.sh

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1499-37fdf477d1fb50f639f87245fe796fd21bbfca89 sh smoketest.sh

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Congrats on your first PR approval :-)

@andrewazores andrewazores merged commit 3f191a7 into cryostatio:main Jun 2, 2023
mergify bot pushed a commit that referenced this pull request Jun 2, 2023
Signed-off-by: Atif Ali <atali@rehat.com>
Co-authored-by: Atif Ali <atali@rehat.com>
(cherry picked from commit 3f191a7)
@aali309
Copy link
Contributor Author

aali309 commented Jun 2, 2023

Thank you so much @andrewazores for the guidance and help!

@aali309 aali309 deleted the multipart-form branch June 2, 2023 15:13
andrewazores pushed a commit that referenced this pull request Jun 2, 2023
Signed-off-by: Atif Ali <atali@rehat.com>
Co-authored-by: Atif Ali <atali@rehat.com>
(cherry picked from commit 3f191a7)

Co-authored-by: Atif Ali <56743004+aali309@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] HTTP 415 when creating Automated Rules using multipart/form-data
2 participants