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(db): change argument order in Exists query for JavaDB #4595

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

DaspawnW
Copy link
Contributor

@DaspawnW DaspawnW commented Jun 8, 2023

Description

Related issues

Checklist

  • [ x] I've read the guidelines for contributing to this repository.
  • [x ] I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@DaspawnW DaspawnW requested a review from knqyf263 as a code owner June 8, 2023 16:23
@CLAassistant
Copy link

CLAassistant commented Jun 8, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@DaspawnW DaspawnW changed the title fix(others): change argument order in Exists query for JavaDB fix(db): change argument order in Exists query for JavaDB Jun 8, 2023
@knqyf263 knqyf263 requested a review from afdesk June 11, 2023 05:46
@afdesk
Copy link
Contributor

afdesk commented Jun 12, 2023

@DaspawnW thanks for your PR!

it looks like a curious mistake.
how did it work before? )

@afdesk
Copy link
Contributor

afdesk commented Jun 13, 2023

but I can't find a case where this mistake matters.

@DaspawnW
Copy link
Contributor Author

Hi @afdesk,

I guess it's more a matter of performance. The Exists query gets called within the aquasecurity/go-dep-parser
https://github.com/aquasecurity/go-dep-parser/blob/main/pkg/java/jar/parse.go#L144 . In this particular case it will always fail and continue then with the SHA1 based query https://github.com/aquasecurity/go-dep-parser/blob/main/pkg/java/jar/parse.go#L151

So it will work but has to perform for each dependency a second query even though it's available.

@afdesk
Copy link
Contributor

afdesk commented Jun 13, 2023

it seems i understood what happens:
this block never runs
https://github.com/aquasecurity/go-dep-parser/blob/main/pkg/java/jar/parse.go#L146

@afdesk
Copy link
Contributor

afdesk commented Jun 13, 2023

Hi @DaspawnW
thanks! you're right!

@afdesk
Copy link
Contributor

afdesk commented Jun 13, 2023

@DaspawnW I've created an issue from your discussion - it's our workflow.
and updated your comment, i hope it's ok.

Copy link
Contributor

@afdesk afdesk left a comment

Choose a reason for hiding this comment

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

thanks for the good catch!

@knqyf263 knqyf263 added this pull request to the merge queue Jun 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 13, 2023
@knqyf263 knqyf263 added this pull request to the merge queue Jun 14, 2023
Merged via the queue into aquasecurity:main with commit bc9513f Jun 14, 2023
9 of 10 checks passed
AnaisUrlichs pushed a commit to AnaisUrlichs/trivy that referenced this pull request Aug 10, 2023
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.

Failure in exists query for JavaDB
4 participants