-
Notifications
You must be signed in to change notification settings - Fork 531
fix: cvedb metric refactoring #4955
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
base: main
Are you sure you want to change the base?
Conversation
@22f1001635 What do you think about creating a constant for the metrics data, used in populate_metrics: cve-bin-tool/cve_bin_tool/cvedb.py Lines 645 to 650 in 8f9043a
like: METRICS = [
(UNKNOWN_METRIC_ID, "UNKNOWN"),
(EPSS_METRIC_ID, "EPSS"),
(CVSS_2_METRIC_ID, "CVSS-2"),
(CVSS_3_METRIC_ID, "CVSS-3"),
] and use the new constant in # Check for metrics table data
if table_name == "metrics":
for metrics_id, metrics_name in METRICS:
result = cursor.execute(
"SELECT * FROM metrics WHERE metrics_id=? AND metrics_name", (metrics_id, metrics_name)
)
if not result.fetchone():
schema_latest = False This has the benefit that it's simpler to add additional METRICS in the future like SSVC in example. |
Hi @jloehel , I have made the required changes. Can you take a look and let me know |
Hi :-) Thanks. I have read the code again and I am not sure if the condition is at the right place because self.refresh_cache_and_update_db() gets still only executed if the db does not exist, the db is older than 24 days and the latest_schema is not matching. I think the condition needs to go here: cve-bin-tool/cve_bin_tool/cvedb.py Lines 328 to 336 in 04c47b8
... and you don't need to call populate_metrics again. It gets called in populate_db already. Only the condition when the database gets updated needs to get modified. Sorry, I should have checked this earlier. |
55554fa
to
6b08bef
Compare
@jloehel made changes; take a look and let me know if anything else is needed |
@22f1001635 What do you think about a test case for this scenario?
|
6b08bef
to
0062994
Compare
hi @jloehel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the work. Looks good to me. I added one comment. Making it a non skip test would be good.
test/test_cvedb.py
Outdated
@@ -91,3 +92,25 @@ def test_new_database_schema(self): | |||
assert all(column in column_names for column in required_columns[table]) | |||
|
|||
self.cvedb.db_close() | |||
|
|||
@pytest.mark.skipif(not LONG_TESTS(), reason="Skipping long tests") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be not a long running test. I think it's not necessary to sync the nvd source for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have made the required changes check whether it aligns with what you have in mind if any other change is needed do tell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a long test because it needs to sync all the NVD source during the update, right? We do not really need a source sync for the test, right? I am not sure, is it possible to update without sources?
0e6b7e6
to
ecca24f
Compare
This PR addresses the issue of handling unknown values in metrics by ensuring the UNKNOWN metric is properly initialized and validated in the CVE database. Key changes include:
✔️ Added UNKNOWN_METRIC_ID to populate_metrics to ensure the "UNKNOWN" metric is inserted into the metrics table.
✔️ Enhanced schema validation in latest_schema to check for the existence of the UNKNOWN metric, triggering a refresh if missing.
✔️ Updated metrics during refresh to ensure metrics are re-populated if the schema is outdated.
These changes ensure that the database consistently handles unknown values and maintains up-to-date metrics, improving reliability and correctness.
Fixes
#4812