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

[java] Add tests for database sources in springboot #3742

Merged
merged 7 commits into from
Jan 21, 2025

Conversation

Mariovido
Copy link
Contributor

Motivation

Add new test for java to check that SQL Injection with database sources is supported.

Changes

  • Added only system test for weblog variant springboot.

Workflow

  1. ⚠️ Create your PR as draft ⚠️
  2. Work on you PR until the CI passes (if something not related to your task is failing, you can ignore it)
  3. Mark it as ready for review
    • Test logic is modified? -> Get a review from RFC owner. We're working on refining the codeowners file quickly.
    • Framework is modified, or non obvious usage of it -> get a review from R&P team

🚀 Once your PR is reviewed, you can merge it!

🛟 #apm-shared-testing 🛟

Reviewer checklist

  • If PR title starts with [<language>], double-check that only <language> is impacted by the change
  • No system-tests internal is modified. Otherwise, I have the approval from R&P team
  • CI is green, or failing jobs are not related to this change (and you are 100% sure about this statement)
  • A docker base image is modified?
    • the relevant build-XXX-image label is present
  • A scenario is added (or removed)?

Copy link
Member

@jandro996 jandro996 left a comment

Choose a reason for hiding this comment

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

Tests are failing for spring-boot variants 😅

@Mariovido Mariovido marked this pull request as ready for review January 16, 2025 14:12
@Mariovido Mariovido requested review from a team as code owners January 16, 2025 14:12
Copy link
Collaborator

@robertomonteromiguel robertomonteromiguel left a comment

Choose a reason for hiding this comment

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

The TestSqlRow is also used in nodejs. Could impact your changes on nodejs? We are only testing this change for java

manifests/java.yml Outdated Show resolved Hide resolved
@Mariovido Mariovido changed the title [java] Add tests for database sources in springboot Add tests for database sources in springboot Jan 17, 2025
Copy link
Member

@jandro996 jandro996 left a comment

Choose a reason for hiding this comment

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

LGTM! BTW is there any reason to not implement the endpoints on the other variants?

@Mariovido Mariovido changed the title Add tests for database sources in springboot [java] Add tests for database sources in springboot Jan 21, 2025
@Mariovido Mariovido merged commit c1185eb into main Jan 21, 2025
69 checks passed
@Mariovido Mariovido deleted the mario.vidal/add_db_taint_tests_java branch January 21, 2025 10:58
@cbeauchesne
Copy link
Collaborator

Mariovido changed the title Add tests for database sources in springboot [java] Add tests for database sources in springboot 4 hours ago

@Mariovido when the test logic is modified, you should not use title prefix. I understand that it was easier to have a quicker CI for the merge commit, but it happen too often to have issues, we really prefer to not take any risk. Thanks you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants