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

refactor: search groupID by artifactID and version #23

Merged

Conversation

DmitriyLewen
Copy link
Collaborator

Description

Change logic to search GroupID by ArtifactID:

  • return only GroupiD
  • Use version to select GroupID (we need to choose only GroupIDs which contain required version)

See aquasecurity/trivy/issues/5627 for more info.

@DmitriyLewen DmitriyLewen self-assigned this Dec 11, 2023
pkg/db/db.go Outdated
}
return indexes, nil
return groupID, nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure we need to return index here.
Looks like GroupID will be enough.

pkg/db/db.go Outdated
Comment on lines 182 to 189
SELECT relevant.group_id, COUNT(relevant.group_id) as count
FROM indices i
JOIN (SELECT a.id, a.group_id
FROM indices i
JOIN artifacts a on a.id = i.artifact_id
WHERE a.artifact_id = ? AND i.version = ? AND i.archive_type = ?) relevant ON relevant.id = i.artifact_id
GROUP BY "group_id"`,
artifactID, version, fileType)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I understand correctly, we need to add one more SELECT to get maximum value from count in sql query.
I'm not sure we need this.

I simply added logic to select the maximum quantity in rows.Next() loop.

pkg/db/db.go Outdated
FROM indices i
JOIN artifacts a on a.id = i.artifact_id
WHERE a.artifact_id = ? AND i.version = ? AND i.archive_type = ?) relevant ON relevant.id = i.artifact_id
GROUP BY "group_id"`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need GROUP BY here? I want to keep the logic in trivy-java-db as minimal as possible. That's why this function returns indexes.
We can match the version in SQL as it is simple enough.

Copy link
Collaborator

@knqyf263 knqyf263 Dec 17, 2023

Choose a reason for hiding this comment

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

By the way, what if the latest version is used in the project? The database probably doesn't have it yet as the database is downloaded once a week, and Trivy will miss it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need GROUP BY here? I want to keep the logic in trivy-java-db as minimal as possible. That's why this function returns indexes.

I updated query - 5c30667
take a look, please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

database is downloaded once a week

Do you mean trivy-java-db?

We update trivy-java-db every day:

- cron: "0 0 * * *" # update indexes every day in 00:00

Users download java-db every 3 days:

const updateInterval = time.Hour * 72 // 3 days

But i understand your point.
I think it is possible. We can get wrong GroupID (if version is not added).
But i am not sure that our changes will cause many issues:

  • after 1(3) days user will still see new package (perhaps we need to add info in Trivy docs about need to update trivy-java-db in this case).
  • we check pom.properties and MANIFEST.MF files before accessing java-db.

@DmitriyLewen
Copy link
Collaborator Author

@knqyf263 can we merge this PR?

@knqyf263 knqyf263 merged commit 184bd74 into aquasecurity:main Jan 9, 2024
3 checks passed
@DmitriyLewen DmitriyLewen deleted the refactor/search-by-aid-and-version branch January 10, 2024 06:39
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.

None yet

2 participants