Run history #885

csordasmarton · 2017-09-07T09:02:42Z

Closes #783

whisperity · 2017-09-07T09:52:58Z

api/report_server.thrift

@@ -32,6 +32,13 @@ struct RunData {
 }
 typedef list<RunData> RunDataList

+struct RunHistoryData {
+  1: string tag,      // tag of the report


Yet again, format the comments here as a full string, and below, etc.

whisperity · 2017-09-07T09:53:32Z

api/report_server.thrift

@@ -188,6 +197,9 @@ service codeCheckerDBAccess {
                         throws (1: shared.RequestFailed requestError),

  // PERMISSION: PRODUCT_ACCESS
+  RunHistoryDataList getRunHistory(1: i64 runId)
+                                   throws (1: shared.RequestFailed requestError),
+
  ReportData getReport(


The comment line for what permission is needed to getReport() went missing as the new function was introduced above.

whisperity · 2017-09-07T09:56:56Z

libcodechecker/libhandlers/store.py

+                        dest="tag",
+                        required=False,
+                        default=argparse.SUPPRESS,
+                        help="Tag which identifies a certain run update.")


Okay... there are a lot of concepts not really explained here. "Update"? "Tag"? I think this help message should be made better...

I believe An identifier for this individual store of results in the run's history. (Run history is used on the GUI.)

whisperity · 2017-09-07T09:57:13Z

libcodechecker/libhandlers/store.py

@@ -107,6 +107,13 @@ def add_arguments_to_parser(parser):
                             "the '--name' parameter given to 'codechecker-"
                             "analyze' will be used, if exists.")

+    parser.add_argument('--tag',


A new argument is added, but the documentation for the store command was not updated.

whisperity · 2017-09-07T09:57:39Z

libcodechecker/server/run_db_model.py

+                    ForeignKey('runs.id', deferrable=True,
+                               initially="DEFERRED", ondelete='CASCADE'),
+                    index=True)
+    tag = Column(String)


Tags need not be unique then?

If it would be unique, how would I add the same tag for the same or for a different run?
E.g:

run_id tag user date

1 v1.0 Anonymous 2017-09-06 16:56:33.681858

1 v1.0 Anonymous 2017-09-06 17:56:33.681858

The question is, do we want to allow the user to do this?

Let's ask the others. @dkrupp @gyorb Depends on whether we want to consider this annotation as a tag or as a comment. Tags, such as version tags, are usually unique.

the tag should be unique at least within a run, because it should "uniquely" identify a single update within a run. most probably users will use it to identify the version of software they are analyzing (e.g. the git hash).

Maybe the name using the name version tag would be more obvious.

whisperity · 2017-09-07T09:58:14Z

libcodechecker/server/run_db_model.py

@@ -198,6 +217,9 @@ class Report(Base):
                                   'reopened',
                                   name='detection_status'))

+    detected_at = Column(DateTime)


I believe a NOT NULL constraint should exist on the detected_at column? Every report is expected to have at least a detection date.

whisperity · 2017-09-07T09:59:12Z

libcodechecker/server/client_db_access_handler.py

@@ -11,6 +11,7 @@
 import codecs
 from collections import defaultdict
 import datetime
+from datetime import timedelta


We only use timedelta at one place, I'd rather not have a separate import just for this.

whisperity · 2017-09-07T09:59:32Z

libcodechecker/server/client_db_access_handler.py

+
+    if report_filter.fixedDate:
+        date = datetime.strptime(report_filter.fixedDate, '%Y-%m-%d')
+        tomorrow = date + timedelta(days=1)


Please use datetime.timedelta() if it is possible. At other places, datetime.strfptime() is used in a similar manner.

If I'm using this way I'm getting the following error:

type object 'datetime.datetime' has no attribute 'timedelta'

I used the following import to resolve this problem:

from datetime import datetime, timedelta

whisperity · 2017-09-07T10:00:09Z

libcodechecker/server/client_db_access_handler.py

@@ -527,6 +553,36 @@ def getRunData(self, run_name_filter):
            session.close()

    @timeit
+    def getRunHistory(self, run_id):
+        try:


Only users who have access to the product should be able to query run history.

self.__require_access()

, before the try statement.

whisperity · 2017-09-07T10:00:45Z

libcodechecker/server/client_db_access_handler.py

@@ -2300,10 +2357,15 @@ def massStoreRun(self, name, version, b64zip, force):
                                                     file_content,
                                                     None)

+        user = self.__auth_session.user \
+            if self.__auth_session else "Anonymous"


'Anonymous'

Xazax-hun · 2017-09-07T10:04:23Z

api/report_server.thrift

@@ -94,7 +101,9 @@ struct ReportFilter_v2 {
  4: list<string>                 reportHash,
  5: list<shared.Severity>        severity,
  6: list<shared.ReviewStatus>    reviewStatus,
-  7: list<shared.DetectionStatus> detectionStatus
+  7: list<shared.DetectionStatus> detectionStatus,
+  8: optional string              detectedDate,


Isn't it better to store something like an ID to the run in the history where it was first detected/fixed?

Since CodeChecker 6, a run ID is a very, very long running stable variable. A run is only introduced once and then for the entire lifespan of the project, it is not expected to be deleted, only updated (stored into) later on. Storing does not change the run ID.

Though I would like it better if dates would be sent as at least integers (in epoch time), not strings, and let the client do the formatting of these values (perhaps different clients with region-specific formats?)

I did not mean the run ID, but an ID to an entry in the run history table. So we can actually check who executed the run and what was the actual tag without joining on the date which is very error prone.

csordasmarton · 2017-09-07T14:36:52Z

I also did some minor changes: in case of the enum-based filters (severity, review status), I encoded the string value (instead of the enum value) into the URL.

http://localhost:8001/Default/#run=2&detection-status=resolved&severity=high

dkrupp

travis test fails with all databases please check why.

csordasmarton · 2017-09-08T10:08:30Z

All travis test cases are successfully passed.

dkrupp

make sure that version tag is unique within a run.

gyorb · 2017-09-13T09:26:04Z

api/v6/report_server.thrift

@@ -25,6 +25,22 @@ struct RunData {
 }
 typedef list<RunData> RunDataList

+struct RunHistoryData {
+  1: i64       runId,    // Unique id of the run.


Do we need to return the runId the client calls the api with a runId getRunHistory(1: i64 runId)?

Yes, we need it, because we can call the getRunHistory function without run id. In this case we get back history data for all runs. (See Run history tab under All reports).

gyorb · 2017-09-13T09:30:19Z

api/v6/report_server.thrift

+struct RunHistoryData {
+  1: i64       runId,    // Unique id of the run.
+  2: string    runName,  // Name of the run.
+  3: string    tag,      // Tag of the report.


Shouldn't this be the tag of the run?
Will we support only one tag for each history data entry?

A version tag is uniquely identifying a store. Actually, the last tag at the run history data is the tag of the run.

We support only one tag for each history data as I know.

gyorb · 2017-09-13T09:32:52Z

api/v6/report_server.thrift

-  7: list<shared.DetectionStatus> detectionStatus
+  7: list<shared.DetectionStatus> detectionStatus,
+  8: list<string>                 runHistoryTag,
+  9: optional string              detectedDate,


I think detectionDate or firstDetectionDate would be more expressive. A comment would be nice that this value is set only once.

gyorb · 2017-09-13T09:35:21Z

api/v6/report_server.thrift

+  7: list<shared.DetectionStatus> detectionStatus,
+  8: list<string>                 runHistoryTag,
+  9: optional string              detectedDate,
+  10: optional string             fixedDate


How about fixDate? I think we should comment here that this date can change.

gyorb · 2017-09-13T09:45:46Z

docs/user_guide.md

@@ -668,6 +668,8 @@ optional arguments:
                        reports to the database. If not specified, the '--
                        name' parameter given to 'codechecker-analyze' will be
                        used, if exists.
+  --tag TAG             An identifier for this individual store of results in


A more descriptive help message would be better to note to the user that an already used tag can not be used again.

gyorb · 2017-09-13T09:50:10Z

libcodechecker/server/client_db_access_handler.py

@@ -49,6 +50,7 @@ class CountFilter:
    SEVERITY = 3
    REVIEW_STATUS = 4
    DETECTION_STATUS = 5
+    RUN_HISTORY_TAG = 5


This should be 6.

gyorb · 2017-09-13T09:55:34Z

libcodechecker/server/run_db_model.py

+                    ForeignKey('runs.id', deferrable=True,
+                               initially="DEFERRED", ondelete='CASCADE'),
+                    index=True)
+    tag = Column(String)


Maybe the name using the name version tag would be more obvious.

gyorb

API call error: massStoreRun RequestFailed(errorCode=2, _message="(sqlite3.IntegrityError) UNIQUE constraint failed: run_histories.run_id, run_histories.tag [SQL: u'INSERT ...

The error if the tag is already used should be better handled. Tag is already used in this run message would be much nicer.

gyorb · 2017-09-13T11:00:00Z

If I check the all reports tab and I select one tag in the filters I get the number of reports but the list of reports is empty.

Going from the all reports tab and run history if I select one tagged version the run id will be shown as the run name and the run counts will show wrong numbers.

gyorb · 2017-09-13T11:02:56Z

Products page is not shown correctly, I'm not sure because of the changes here.

gyorb · 2017-09-13T11:35:15Z

I can reproduce the product listing problem with chrome, firefox works fine for me.

csordasmarton · 2017-09-13T14:44:49Z

I fixed the error message of an already existing tag for the run to this:

[14:50] - Storing analysis results for run 'tinyxml-resolved'
API call error: massStoreRun
RequestFailed(errorCode=2, _message='Tag v1.0 is already used in this run')
[14:50] - Storage failed: RequestFailed(errorCode=2, _message='Tag v1.0 is already used in this run')

I fixed the empty report error.
I created an issue for product listing error (Product listing error in Firefox #912).

whisperity

time on the API remains a string, then?

whisperity

What is the concept behind showing Run History on the All reports pane?

whisperity · 2017-09-14T08:11:37Z

www/style/codecheckerviewer.css

+  font-size: 1.5em;
+  font-weight: bold;
+  color: #000;
+  font-family: Times, serif;


Is this style here intentional? We don't really use serif fonts in the GUI. Looks awkward.

gyorb · 2017-09-14T13:27:58Z

After cleaning all the filter values setting the Fix date (only the day) in the Detection date filter the request to the server crashes.

csordasmarton added enhancement 🌟 API change 📄 Content of patch changes API! database 🗄️ Issues related to the database schema. GUI 🎨 WARN ⚠️: Backward compatibility breaker! MIND THE GAP! Merging this patch will mess up compatibility with the previous releases! labels Sep 7, 2017

csordasmarton added this to the 6.0 pre4 milestone Sep 7, 2017

csordasmarton requested review from whisperity and gyorb September 7, 2017 09:02

whisperity requested changes Sep 7, 2017

View reviewed changes

Xazax-hun reviewed Sep 7, 2017

View reviewed changes

csordasmarton force-pushed the run_history branch 2 times, most recently from b65a0eb to 754086d Compare September 7, 2017 14:31

csordasmarton requested a review from whisperity September 7, 2017 14:37

csordasmarton force-pushed the run_history branch from 754086d to ac85840 Compare September 8, 2017 06:59

dkrupp requested changes Sep 8, 2017

View reviewed changes

csordasmarton force-pushed the run_history branch 2 times, most recently from bbd66ff to 3c04a73 Compare September 8, 2017 15:33

csordasmarton added the WIP 💣 Work In Progress label Sep 8, 2017

csordasmarton force-pushed the run_history branch 9 times, most recently from d45021d to b5ed4f2 Compare September 12, 2017 13:01

gyorb suggested changes Sep 13, 2017

View reviewed changes

csordasmarton force-pushed the run_history branch 2 times, most recently from 71eb8fc to 0e43cb3 Compare September 13, 2017 14:29

csordasmarton force-pushed the run_history branch 2 times, most recently from d2fb1f1 to b831eee Compare September 14, 2017 07:53

whisperity reviewed Sep 14, 2017

View reviewed changes

whisperity requested changes Sep 14, 2017

View reviewed changes

whisperity mentioned this pull request Sep 14, 2017

Show the tags from the filtered runs if multiple runs are searched #919

Closed

csordasmarton force-pushed the run_history branch 4 times, most recently from 27f91e2 to 30ff3c1 Compare September 14, 2017 10:13

whisperity approved these changes Sep 14, 2017

View reviewed changes

csordasmarton force-pushed the run_history branch from 30ff3c1 to 735538f Compare September 14, 2017 11:48

csordasmarton requested a review from gyorb September 14, 2017 11:48

csordasmarton force-pushed the run_history branch from 735538f to 8a3b6e4 Compare September 14, 2017 13:39

gyorb approved these changes Sep 14, 2017

View reviewed changes

Run history

4c56d8a

csordasmarton force-pushed the run_history branch from 8a3b6e4 to 4c56d8a Compare September 14, 2017 15:01

gyorb merged commit 9d43204 into Ericsson:master Sep 14, 2017

csordasmarton deleted the run_history branch September 21, 2017 07:56

run_id	tag	user	date
1	v1.0	Anonymous	2017-09-06 16:56:33.681858
1	v1.0	Anonymous	2017-09-06 17:56:33.681858

Run history #885

Run history #885

Conversation

csordasmarton commented Sep 7, 2017 • edited Loading

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

dkrupp Sep 8, 2017 • edited Loading

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

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

Xazax-hun Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

csordasmarton commented Sep 7, 2017

dkrupp left a comment

Choose a reason for hiding this comment

csordasmarton commented Sep 8, 2017

dkrupp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyorb left a comment

Choose a reason for hiding this comment

gyorb commented Sep 13, 2017

gyorb commented Sep 13, 2017

gyorb commented Sep 13, 2017

csordasmarton commented Sep 13, 2017

whisperity left a comment

Choose a reason for hiding this comment

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyorb commented Sep 14, 2017

csordasmarton commented Sep 7, 2017 •

edited

Loading

dkrupp Sep 8, 2017 •

edited

Loading

Xazax-hun Sep 7, 2017 •

edited

Loading