Merge pull request Ericsson#3580 from bruntib/review_status_per_report

[feat] Review status belongs to report

docs/web/user_guide.md

@@ -1280,6 +1280,10 @@ filter arguments:
                         :new,unresolved:false_positive,intentional"
 
 comparison modes:
+  List reports that can be found only in baseline or new runs or in both. False
+  positive and intentional reports are considered as resolved, i.e. these are
+  excluded from the report set as if they were not reported.
+
   --new                 Show results that didn't exist in the 'base' but
                         appear in the 'new' run.
   --resolved            Show results that existed in the 'base' but

web/api/js/codechecker-api-node/package.json

@@ -1,6 +1,6 @@
 {
   "name": "codechecker-api",
-  "version": "6.48.0",
+  "version": "6.50.0",
   "description": "Generated node.js compatible API stubs for CodeChecker server.",
   "main": "lib",
   "homepage": "https://github.com/Ericsson/codechecker",

web/api/py/codechecker_api/setup.py

@@ -8,7 +8,7 @@
 with open('README.md', encoding='utf-8', errors="ignore") as f:
     long_description = f.read()
 
-api_version = '6.48.0'
+api_version = '6.50.0'
 
 setup(
     name='codechecker_api',

web/api/py/codechecker_api_shared/setup.py

@@ -8,7 +8,7 @@
 with open('README.md', encoding='utf-8', errors="ignore") as f:
     long_description = f.read()
 
-api_version = '6.48.0'
+api_version = '6.50.0'
 
 setup(
     name='codechecker_api_shared',

web/api/report_server.thrift

@@ -226,7 +226,8 @@ struct ReviewData {
   1: ReviewStatus  status,
   2: string        comment,
   3: string        author,
-  4: string        date
+  4: string        date,
+  5: bool          isInSource, // Indicates whether the review status comes from source code comment.
 }
 
 /*
@@ -285,7 +286,7 @@ struct ReviewStatusRuleFilter {
  *     be only one rule.
  * - reviewData: Review status information.
  * - associatedReportCount: Number of associated reports. If there is no
-       associated in the product for reportHash the value will be 0.
+ *     associated in the product for reportHash the value will be 0.
  */
 struct ReviewStatusRule {
   1: string reportHash,

web/client/codechecker_client/cmd/cmd.py

@@ -539,7 +539,12 @@ def __register_diff(parser):
 
     __add_filtering_arguments(parser, DEFAULT_FILTER_VALUES, True)
 
-    group = parser.add_argument_group("comparison modes")
+    group = parser.add_argument_group(
+        "comparison modes",
+        "List reports that can be found only in baseline or new runs or in "
+        "both. False positive and intentional reports are considered as "
+        "resolved, i.e. these are excluded from the report set as if they "
+        "were not reported.")
     group = group.add_mutually_exclusive_group(required=True)
 
     group.add_argument('--new',

web/client/codechecker_client/cmd_line_client.py

@@ -882,7 +882,25 @@ def get_diff_local_dir_remote_run(
         run_ids, run_names, tag_ids = \
             process_run_args(client, remote_run_names)
         local_report_hashes = set([r.report_hash for r in report_dir_results])
-        local_report_hashes.update(baseline.get_report_hashes(baseline_files))
+
+        review_status_filter = ttypes.ReviewStatusRuleFilter()
+        review_status_filter.reportHashes = local_report_hashes
+        review_status_filter.reviewStatuses = [
+            ttypes.ReviewStatus.FALSE_POSITIVE,
+            ttypes.ReviewStatus.INTENTIONAL]
+        closed_hashes = set(
+            rule.reportHash for rule in
+            client.getReviewStatusRules(
+                review_status_filter, None, None, None))
+
+        report_dir_results = [
+            r for r in report_dir_results if
+            r.review_status not in ['false positive', 'intentional'] and
+            (r.report_hash not in closed_hashes or
+             r.review_status == 'confirmed')]
+        local_report_hashes = set(r.report_hash for r in report_dir_results)
+        local_report_hashes.update(
+            baseline.get_report_hashes(baseline_files) - closed_hashes)
 
         if diff_type == ttypes.DiffType.NEW:
             # Get report hashes which can be found only in the remote runs.
@@ -955,8 +973,24 @@ def get_diff_remote_run_local_dir(
             process_run_args(client, remote_run_names)
         local_report_hashes = set([r.report_hash for r in report_dir_results])
 
-        local_report_hashes = local_report_hashes.union(
-            baseline.get_report_hashes(baseline_files))
+        review_status_filter = ttypes.ReviewStatusRuleFilter()
+        review_status_filter.reportHashes = local_report_hashes
+        review_status_filter.reviewStatuses = [
+            ttypes.ReviewStatus.FALSE_POSITIVE,
+            ttypes.ReviewStatus.INTENTIONAL]
+        closed_hashes = set(
+            rule.reportHash for rule in
+            client.getReviewStatusRules(
+                review_status_filter, None, None, None))
+
+        report_dir_results = [
+            r for r in report_dir_results if
+            r.review_status not in ['false positive', 'intentional'] and
+            (r.report_hash not in closed_hashes or
+             r.review_status == 'confirmed')]
+        local_report_hashes = set(r.report_hash for r in report_dir_results)
+        local_report_hashes.update(
+            baseline.get_report_hashes(baseline_files) - closed_hashes)
 
         remote_hashes = client.getDiffResultsHash(
             run_ids, local_report_hashes, diff_type, None, tag_ids)

web/client/codechecker_client/helpers/results.py

@@ -99,6 +99,10 @@ def changeReviewStatus(self, report_id, status, message):
     def changeReviewStatusByHash(self, bug_hash, status, message):
         pass
 
+    @ThriftClientCall
+    def getReviewStatusRules(self, filter, sortMode, limit, offset):
+        pass
+
     @ThriftClientCall
     def getRunResults(self, runIds, limit, offset, sortType, reportFilter,
                       cmpData, getDetails):

web/codechecker_web/shared/version.py

@@ -18,7 +18,7 @@
 # The newest supported minor version (value) for each supported major version
 # (key) in this particular build.
 SUPPORTED_VERSIONS = {
-    6: 48
+    6: 50
 }
 
 # Used by the client to automatically identify the latest major and minor

web/server/codechecker_server/api/mass_store_run.py

@@ -38,7 +38,7 @@
 from ..database.database import DBSession
 from ..database.run_db_model import AnalysisInfo, AnalyzerStatistic, \
     BugPathEvent, BugReportPoint, ExtendedReportData, File, FileContent, \
-    Report as DBReport, Run, RunHistory, RunLock
+    Report as DBReport, ReviewStatus, Run, RunHistory, RunLock
 from ..metadata import checker_is_unavailable, MetadataInfoParser
 
 from .report_server import ThriftRequestHandler
@@ -712,10 +712,13 @@ def __add_report(
         run_id: int,
         report: Report,
         file_path_to_id: Dict[str, int],
+        review_status: ReviewStatus,
+        review_status_is_in_source: bool,
         detection_status: str,
         detection_time: datetime,
         analysis_info: AnalysisInfo,
-        analyzer_name: Optional[str] = None
+        analyzer_name: Optional[str] = None,
+        fixed_at: Optional[datetime] = None
     ) -> int:
         """ Add report to the database. """
         try:
@@ -728,9 +731,14 @@ def __add_report(
                 run_id, report.report_hash, file_path_to_id[report.file.path],
                 report.message, checker_name or 'NOT FOUND',
                 report.category, report.type, report.line, report.column,
-                severity, detection_status, detection_time,
+                severity, review_status.status, review_status.author,
+                review_status.message, review_status.date,
+                review_status_is_in_source,
+                detection_status, detection_time,
                 len(report.bug_path_events), analyzer_name)
 
+            db_report.fixed_at = fixed_at
+
             session.add(db_report)
             session.flush()
 
@@ -799,50 +807,70 @@ def __process_report_file(
         if not reports:
             return True
 
-        def set_review_status(report: Report):
+        def get_review_status(report: Report) -> Tuple[ReviewStatus, bool]:
             """
-            Set review status for the given report if there is any source code
-            comment.
+            Return the review status belonging to the given report and a
+            boolean value which indicates whether the review status comes from
+            a source code comment.
+
+            - Return the review status and True based on the source code
+              comment.
+            - If the review status is ambiguous (i.e. multiple source code
+              comments belong to it) then (unreviewed, False) returns.
+            - If there is no source code comment then the default review status
+              and True returns based on review_statuses database table.
+            - If the report doesn't have a default review status then an
+              "unreviewed" review status and False returns.
             """
-            checker_name = report.checker_name
+            unreviewed = ReviewStatus()
+            unreviewed.status = 'unreviewed'
+            unreviewed.message = b''
+            unreviewed.bug_hash = report.report_hash
+            unreviewed.author = self.user_name
+            unreviewed.date = run_history_time
 
             # The original file path is needed here, not the trimmed, because
             # the source files are extracted as the original file path.
-            file_name = report.file.original_path
+            source_file_name = os.path.realpath(os.path.join(
+                source_root, report.file.original_path.strip("/")))
+
+            if os.path.isfile(source_file_name):
+                src_comment_data = parse_codechecker_review_comment(
+                    source_file_name, report.line, report.checker_name)
+
+                if len(src_comment_data) == 1:
+                    data = src_comment_data[0]
+                    rs = data.status, bytes(data.message, 'utf-8')
+
+                    review_status = ReviewStatus()
+                    review_status.status = rs[0]
+                    review_status.message = rs[1]
+                    review_status.bug_hash = report.report_hash
+                    review_status.author = self.user_name
+                    review_status.date = run_history_time
+
+                    return review_status, True
+                elif len(src_comment_data) > 1:
+                    LOG.warning(
+                        "Multiple source code comment can be found "
+                        "for '%s' checker in '%s' at line %s. "
+                        "This suppression will not be taken into account!",
+                        report.checker_name, source_file_name, report.line)
+
+                    self.__wrong_src_code_comments.append(
+                        f"{source_file_name}|{report.line}|"
+                        f"{report.checker_name}")
+
+                    return unreviewed, False
+
+            review_status = session.query(ReviewStatus) \
+                .filter(ReviewStatus.bug_hash == report.report_hash) \
+                .one_or_none()
 
-            source_file_name = os.path.realpath(
-                os.path.join(source_root, file_name.strip("/")))
+            if review_status is None:
+                review_status = unreviewed
 
-            # Check and store source code comments.
-            if not os.path.isfile(source_file_name):
-                return
-
-            report_line = report.line
-            source_file = os.path.basename(file_name)
-
-            src_comment_data = parse_codechecker_review_comment(
-                source_file_name, report_line, checker_name)
-
-            if len(src_comment_data) == 1:
-                status = src_comment_data[0].status
-                rw_status = ttypes.ReviewStatus.FALSE_POSITIVE
-                if status == 'confirmed':
-                    rw_status = ttypes.ReviewStatus.CONFIRMED
-                elif status == 'intentional':
-                    rw_status = ttypes.ReviewStatus.INTENTIONAL
-
-                self.__report_server._setReviewStatus(
-                    session, report.report_hash, rw_status,
-                    src_comment_data[0].message, run_history_time)
-            elif len(src_comment_data) > 1:
-                LOG.warning(
-                    "Multiple source code comment can be found "
-                    "for '%s' checker in '%s' at line %s. "
-                    "This bug will not be suppressed!",
-                    checker_name, source_file, report_line)
-
-                self.__wrong_src_code_comments.append(
-                    f"{source_file}|{report_line}|{checker_name}")
+            return review_status, False
 
         def get_missing_file_ids(report: Report) -> List[str]:
             """ Returns file paths which database file id is missing. """
@@ -884,6 +912,7 @@ def get_missing_file_ids(report: Report) -> List[str]:
             detection_status = 'new'
             detected_at = run_history_time
 
+            old_report = None
             if report.report_hash in hash_map_reports:
                 old_report = hash_map_reports[report.report_hash][0]
                 old_status = old_report.detection_status
@@ -893,16 +922,27 @@ def get_missing_file_ids(report: Report) -> List[str]:
 
             analyzer_name = mip.checker_to_analyzer.get(
                 report.checker_name, report.analyzer_name)
+            review_status, scc = get_review_status(report)
+            review_status.date = run_history_time
+
+            # False positive and intentional reports are considered as closed
+            # reports which is indicated with non-null "fixed_at" date.
+            fixed_at = None
+            if review_status.status in ['false_positive', 'intentional']:
+                if old_report and old_report.review_status in \
+                        ['false_positive', 'intentional']:
+                    fixed_at = old_report.review_status_date
+                else:
+                    fixed_at = review_status.date
 
             report_id = self.__add_report(
                 session, run_id, report, file_path_to_id,
-                detection_status, detected_at, analysis_info, analyzer_name)
+                review_status, scc, detection_status, detected_at,
+                analysis_info, analyzer_name, fixed_at)
 
             self.__new_report_hashes.add(report.report_hash)
             self.__already_added_report_hashes.add(report_path_hash)
 
-            set_review_status(report)
-
             LOG.debug("Storing report done. ID=%d", report_id)
 
         return True
@@ -995,11 +1035,6 @@ def get_skip_handler(
                 reports_to_delete.update([x.id for x in reports])
             else:
                 for report in reports:
-                    # We set the fix date of a report only if the report
-                    # has not been fixed before.
-                    if report.fixed_at:
-                        continue
-
                     checker = report.checker_id
                     if checker in disabled_checkers:
                         report.detection_status = 'off'
@@ -1008,7 +1043,8 @@ def get_skip_handler(
                     else:
                         report.detection_status = 'resolved'
 
-                    report.fixed_at = run_history_time
+                    if report.fixed_at is None:
+                        report.fixed_at = run_history_time
 
         if reports_to_delete:
             self.__report_server._removeReports(
@@ -1095,14 +1131,17 @@ def store(self) -> int:
                 # thrown: (psycopg2.extensions.TransactionRollbackError)
                 # deadlock detected.
                 # The problem is that the report hash is the key for the
-                # review data table and both of the store actions try to
-                # update the same review data row.
+                # review_statuses table and both of the store actions try to
+                # update the same review_statuses data row.
                 # Neither of the two processes can continue, and they will wait
                 # for each other indefinitely. PostgreSQL in this case will
                 # terminate one transaction with the above exception.
                 # For this reason in case of failure we will wait some seconds
                 # and try to run the storage again.
                 # For more information see #2655 and #2653 issues on github.
+                # TODO: Since review status is stored in "reports" table and
+                # "review_statuses" table is not written during storage, this
+                # multiple trials should be unnecessary.
                 max_num_of_tries = 3
                 num_of_tries = 0
                 sec_to_wait_after_failure = 60