feat: Update queries #3172

Rexbeast2 · 2023-07-22T05:16:51Z

This PR is to be only considered after closing #3147.

anthonyharrison

Need to also add some tests to validate that the metrics table has been populated correctly.

anthonyharrison · 2023-07-23T16:23:42Z

cve_bin_tool/data_sources/epss_source.py

@@ -118,5 +118,5 @@ def parse_epss_data(self, file_path=None):
        # Parse the data from the remaining rows
        for row in reader:
            cve_id, epss_score, epss_percentile = row[:3]
-            parsed_data.append((cve_id, "EPSS", epss_score, epss_percentile))
+            parsed_data.append((cve_id, 1, epss_score, epss_percentile))


@Rexbeast2 You should find the value of the EPSS metric by searching for the EPSS string in the metrics table rather than assume it is '1'

anthonyharrison · 2023-07-23T16:35:43Z

cve_bin_tool/cvedb.py

+            cursor.execute(query, [cve.get("CVSS_version")])
+            # Fetch all the results of the query and use 'map' to extract only the 'metrics_name' from the result
+            metric = list(map(lambda x: x[0], cursor.fetchall()))
+            # Since the query is expected to return a single result, extract the first item from the list and store it in 'metric'


We should probably add some debug to check this assumption

anthonyharrison · 2023-07-23T16:37:27Z

cve_bin_tool/cvedb.py

+
+        for cve in severity_data:
+            # Check no None values
+            if not bool(cve.get("score")):


Won't cve["score"] = cve.get("score","unknown") work?

Well, for this. I tried to keep it as similar as possible to populate_severity.

terriko

Reminder: because I enabled branch protection I won't be able to merge this until all tests are passing. It looks like you'll need to update this branch to get the last one I had to disable to get main passing again. (Plus it looks like Anthony's got other feedback that needs incorporation.)

Rexbeast2 · 2023-07-25T06:03:38Z

@terriko As far as I can see, the test cases are failing due to test/test_csv2cve. Not because of any code change that I made. Can you please verify that?
@anthonyharrison if you see any more improvements, please let me know.

terriko · 2023-07-26T00:21:51Z

@terriko As far as I can see, the test cases are failing due to test/test_csv2cve. Not because of any code change that I made.

test_csv2cve isn't failing on main, and the way it's failing makes it look like it's not finding some data (it looks like the assert is expecting to find more than 60 of something, and instead it's finding 1). If I had to guess, you're accidentally clobbering some data that it needs to return correct results. Probably something in the get_cves() code?

terriko

Looks like you've got a merge conflict to resolve now that I've merged #3147 . I'll leave that to you, I also put in some inline comments about docstrings that you might as well do at the same time. We haven't been super consistent about docstrings across the project but you've got a couple of places where you've already written the string as a comment so we might as well just format it as a docstring! If you're bored while you're waiting for tests on this to run you could go back and look at your previously added functions for some easy refactoring commits.

terriko · 2023-07-26T18:30:09Z

cve_bin_tool/cvedb.py

@@ -552,6 +554,56 @@ def populate_affected(self, affected_data, cursor, data_source):
        except Exception as e:
            LOGGER.info(f"Unable to insert data for {data_source} - {e}")

+    def metric_finder(self, cursor, cve):
+        # SQL query to retrieve the metrics_name based on the metrics_id


This comment is helpful; maybe we could turn it into a docstring so it can be parsed and used as such?

Suggested change

# SQL query to retrieve the metrics_name based on the metrics_id

''' SQL query to retrieve the metrics_name based on the metrics_id '''

Short explainer on docstrings in case you're not familiar with them: https://www.programiz.com/python-programming/docstrings

More generally: cvedb is lacking in a lot of docstrings but it would be nice to have them in new functions if you remember. I'll flag a few more new functions in this PR for you. Sorry I didn't think to do it with earlier PRs!

terriko · 2023-07-26T18:30:33Z

cve_bin_tool/cvedb.py

+            )
+        return metric
+
+    def populate_cve_metrics(self, severity_data, cursor):


Good place for a docstring!

terriko · 2023-07-26T18:36:31Z

cve_bin_tool/cvedb.py

@@ -567,6 +619,12 @@ def populate_metrics(self):
        self.connection.commit()
        self.db_close()

+    def populate_epss(self):


Another good place for a docstring. I feel like this short function is mostly self-evident to people who know the acronyms but could be difficult for someone who doesn't. So this is a nice opportunity to expand the acronym and have a really short explainer about what it's for. Something like "Add Exploit Prediction Scoring System (EPSS) data to help users evaluate risks"

terriko · 2023-07-26T18:37:24Z

cve_bin_tool/data_sources/epss_source.py

@@ -99,6 +101,14 @@ async def download_epss_data(self):
            except aiohttp.ClientError as e:
                self.LOGGER.error(f"An error occurred during downloading epss {e}")

+    def EPSS_id_finder(self, cursor):


Another good place for a docstring.

test/test_source_epss.py

codecov-commenter · 2023-08-04T11:14:33Z

Codecov Report

Merging #3172 (7853193) into main (ae66713) will decrease coverage by 0.79%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3172      +/-   ##
==========================================
- Coverage   81.28%   80.50%   -0.79%     
==========================================
  Files         716      716              
  Lines       11126    11163      +37     
  Branches     1495     1497       +2     
==========================================
- Hits         9044     8987      -57     
- Misses       1693     1776      +83     
- Partials      389      400      +11

Flag	Coverage Δ
longtests	`80.50% <100.00%> (+5.11%)`	⬆️
win-longtests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
cve_bin_tool/cve_scanner.py	`85.26% <100.00%> (+1.54%)`	⬆️
cve_bin_tool/cvedb.py	`62.24% <100.00%> (+0.15%)`	⬆️
cve_bin_tool/data_sources/epss_source.py	`72.15% <100.00%> (+2.28%)`	⬆️
cve_bin_tool/util.py	`79.66% <100.00%> (+0.17%)`	⬆️
test/test_source_epss.py	`100.00% <100.00%> (ø)`

... and 20 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

terriko

Just some formatting fixes for docstrings. I'm so glad you managed to resolve the test issue!

terriko · 2023-08-07T17:34:02Z

cve_bin_tool/cvedb.py

+        """
+        EPSS uses metrics table to get the EPSS metric id.
+        It can't be ran before creation of metrics table.
+        """


This one actually doesn't need to be a string, it can be a comment. Docstrings are a special case of a string appearing immediately after a def function definition (at least for functions) but anything else inside the function can probably be a comment.

Suggested change

"""

EPSS uses metrics table to get the EPSS metric id.

It can't be ran before creation of metrics table.

"""

# EPSS uses metrics table to get the EPSS metric id.

# It can't be ran before creation of metrics table.

terriko · 2023-08-07T17:35:30Z

cve_bin_tool/cvedb.py

+    """Adds data into CVE metrics table"""
+
    def populate_cve_metrics(self, severity_data, cursor):


Suggested change

"""Adds data into CVE metrics table"""

def populate_cve_metrics(self, severity_data, cursor):

def populate_cve_metrics(self, severity_data, cursor):

""" Adds data into CVE metrics table """

Docstring goes after def here

terriko · 2023-08-07T17:36:44Z

cve_bin_tool/cvedb.py

+    """Adding data to metric table."""
+
    def populate_metrics(self):


Suggested change

"""Adding data to metric table."""

def populate_metrics(self):

def populate_metrics(self):

""" Adding data to metric table. """

Same deal. Beware, I'm doing suggestions from the web interface so my indentation may be incorrect if you try to just merge these in.

terriko · 2023-08-07T17:37:19Z

cve_bin_tool/cvedb.py

+    """Add EPSS data into the database"""
+
+    def populate_epss(self):


Suggested change

"""Add EPSS data into the database"""

def populate_epss(self):

def populate_epss(self):

""" Add EPSS data into the database """

Same thing

terriko · 2023-08-07T17:38:50Z

cve_bin_tool/data_sources/epss_source.py

+    """Search for metric id in EPSS table"""
+
+    def EPSS_id_finder(self, cursor):


Suggested change

"""Search for metric id in EPSS table"""

def EPSS_id_finder(self, cursor):

def EPSS_id_finder(self, cursor):

"""Search for metric id in EPSS table"""

terriko · 2023-08-07T17:39:27Z

cve_bin_tool/data_sources/epss_source.py

+    """Parse epss data from the file path given and return the parse data"""
+
    def parse_epss_data(self, file_path=None):


Suggested change

"""Parse epss data from the file path given and return the parse data"""

def parse_epss_data(self, file_path=None):

def parse_epss_data(self, file_path=None):

""" Parse epss data from the file path given and return the parse data """

terriko

Okay, looks like those docstrings are handled. Let's get this merged so you're set for the next piece. Thank you!

Rexbeast2 added 2 commits July 22, 2023 04:18

feat: updating queries

f8fe671

Merge branch 'main' into update_queries

8a30099

anthonyharrison requested changes Jul 23, 2023

View reviewed changes

terriko requested changes Jul 24, 2023

View reviewed changes

Rexbeast2 added 4 commits July 24, 2023 23:51

fix: updating EPSS insert

0f50346

fix: updating test case

47a7243

fix: updating test

d34a5f3

fix: updating test

b8a4f93

Merge branch 'main' into update_queries

4d5a594

terriko requested changes Jul 26, 2023

View reviewed changes

Rexbeast2 added 4 commits July 27, 2023 22:10

Merge branch 'main' into update_queries

00b662f

fix: removing copy

17fd6d2

fix: removing copy

2418c6f

Merge branch 'main' into update_queries

be01fc5

Rexbeast2 and others added 3 commits August 4, 2023 20:21

fix: fixing pre commit

30b1346

fix: adding comments

2b833f2

fix: fixing precommit

3ba09ef

terriko requested changes Aug 7, 2023

View reviewed changes

fix: fixing docstring

7853193

terriko approved these changes Aug 7, 2023

View reviewed changes

terriko merged commit 06b55f7 into intel:main Aug 7, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update queries #3172

feat: Update queries #3172

Rexbeast2 commented Jul 22, 2023

anthonyharrison left a comment

anthonyharrison Jul 23, 2023

anthonyharrison Jul 23, 2023

anthonyharrison Jul 23, 2023

Rexbeast2 Jul 24, 2023

terriko left a comment

Rexbeast2 commented Jul 25, 2023 •

edited

Loading

terriko commented Jul 26, 2023

terriko left a comment

terriko Jul 26, 2023

terriko Jul 26, 2023

terriko Jul 26, 2023

terriko Jul 26, 2023

codecov-commenter commented Aug 4, 2023 •

edited

Loading

terriko left a comment

terriko Aug 7, 2023

terriko Aug 7, 2023

terriko Aug 7, 2023

terriko Aug 7, 2023

terriko Aug 7, 2023

terriko Aug 7, 2023

terriko left a comment

	# SQL query to retrieve the metrics_name based on the metrics_id
	''' SQL query to retrieve the metrics_name based on the metrics_id '''

		"""Adds data into CVE metrics table"""

		def populate_cve_metrics(self, severity_data, cursor):

		"""Adding data to metric table."""

		def populate_metrics(self):

		"""Add EPSS data into the database"""

		def populate_epss(self):

		"""Search for metric id in EPSS table"""

		def EPSS_id_finder(self, cursor):

		"""Parse epss data from the file path given and return the parse data"""

		def parse_epss_data(self, file_path=None):

feat: Update queries #3172

feat: Update queries #3172

Conversation

Rexbeast2 commented Jul 22, 2023

anthonyharrison left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terriko left a comment

Choose a reason for hiding this comment

Rexbeast2 commented Jul 25, 2023 • edited Loading

terriko commented Jul 26, 2023

terriko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 4, 2023 • edited Loading

Codecov Report

terriko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terriko left a comment

Choose a reason for hiding this comment

Rexbeast2 commented Jul 25, 2023 •

edited

Loading

codecov-commenter commented Aug 4, 2023 •

edited

Loading