Skip to content

[NFC] Merge report path tables in the database #4946

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.

Sign up for GitHub

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

Open
bruntib wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[NFC] Merge report path tables in the database #4946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 76 additions & 31 deletions web/server/codechecker_server/api/mass_store_run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,8 @@
from ..database.database import DBSession
from ..database.run_db_model import \
AnalysisInfo, AnalyzerStatistic, \
BugPathEvent, BugReportPoint, \
ReportPathData, \
Checker, CheckerSet, CheckerSetItem, \
ExtendedReportData, \
File, FileContent, \
Report as DBReport, ReportAnnotations, ReviewStatus as ReviewStatusRule, \
Run, RunLock as DBRunLock, RunHistory, \
Expand All @@ -61,7 +60,6 @@
from ..session_manager import SessionManager
from ..task_executors.abstract_task import AbstractTask, TaskCancelHonoured
from ..task_executors.task_manager import TaskManager
from .thrift_enum_helper import report_extended_data_type_str

from sqlalchemy.orm import Session as SA_Session

Expand Down Expand Up @@ -1298,44 +1296,91 @@ def __realise_fake_checkers(self, session):
.update({"checker_id": chk_obj.id},
synchronize_session=False)

def __add_report_context(self, session, file_path_to_id):
def __add_report_context(
self,
session: DBSession,

@barnabasdomozi barnabasdomozi Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The correct type annonation for session is SA_Session not DBSession.

@bruntib bruntib Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are you sure about it? When I follow back the type of session through the function call chain, it ends up in a DBSession.

file_path_to_id: Dict[str, int]
):
for db_report, report in self.__added_reports:
path_data = []
used_file_ids = set()

LOG.debug("Storing bug path positions.")
for idx, path_pos in enumerate(report.bug_path_positions):
session.add(BugReportPoint(
path_pos.range.start_line, path_pos.range.start_col,
path_pos.range.end_line, path_pos.range.end_col,
idx, file_path_to_id[path_pos.file.path], db_report.id))
for path_pos in report.bug_path_positions:
path_data.append({
"from": {

@barnabasdomozi barnabasdomozi Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

While this works, using a raw dictionary for structured data is prone to errors.
Just to name a few looking at this example:

  • Making a typo in from e.g. frmo, and appending that to path_data.
  • Not following the structure correctly, e.g. placing row outside from.
  • Entirely missing a field, such as appending an element that has no fid

These are all mistakes that are easy to make but hard to notice.
In contrast, consider putting all these information into a Python dataclass which are designed for these usecases.
With a dataclass, all of the fields are required params, and a type checker can also verify that a dataclass is properly constructed. Type checkers usually treat raw dictionaries as unstructured data and don't perform checks on them.

This comment applies especially to report_server.py where this data retrieved.

A raw dictionary can be an internal representation of the data, which is eventually stored (and compressed) in the database. But in my opinion, developers should access it via a type safe interface.

"row": path_pos.range.start_line,
"col": path_pos.range.start_col
},
"to": {
"row": path_pos.range.end_line,
"col": path_pos.range.end_col
},
"type": "path",
"fid": file_path_to_id[path_pos.file.path]
})
used_file_ids.add(file_path_to_id[path_pos.file.path])

LOG.debug("Storing bug path events.")
for idx, event in enumerate(report.bug_path_events):
session.add(BugPathEvent(
event.range.start_line, event.range.start_col,
event.range.end_line, event.range.end_col,
idx, event.message, file_path_to_id[event.file.path],
db_report.id))
for event in report.bug_path_events:
path_data.append({
"from": {
"row": event.range.start_line,
"col": event.range.start_col
},
"to": {
"row": event.range.end_line,
"col": event.range.end_col
},
"type": "event",
"msg": event.message,
"fid": file_path_to_id[event.file.path]
})
used_file_ids.add(file_path_to_id[event.file.path])

LOG.debug("Storing notes.")
for note in report.notes:
data_type = report_extended_data_type_str(
ttypes.ExtendedReportDataType.NOTE)

session.add(ExtendedReportData(
note.range.start_line, note.range.start_col,
note.range.end_line, note.range.end_col,
note.message, file_path_to_id[note.file.path],
db_report.id, data_type))
path_data.append({
"from": {
"row": note.range.start_line,
"col": note.range.start_col
},
"to": {
"row": note.range.end_line,
"col": note.range.end_col
},
"type": "note",
"msg": note.message,
"fid": file_path_to_id[note.file.path]
})
used_file_ids.add(file_path_to_id[note.file.path])

LOG.debug("Storing macro expansions.")
for macro in report.macro_expansions:
data_type = report_extended_data_type_str(
ttypes.ExtendedReportDataType.MACRO)

session.add(ExtendedReportData(
macro.range.start_line, macro.range.start_col,
macro.range.end_line, macro.range.end_col,
macro.message, file_path_to_id[macro.file.path],
db_report.id, data_type))
path_data.append({
"from": {
"row": macro.range.start_line,
"col": macro.range.start_col
},
"to": {
"row": macro.range.end_line,
"col": macro.range.end_col
},
"type": "macro",
"msg": macro.message,
"fid": file_path_to_id[macro.file.path]
})
used_file_ids.add(file_path_to_id[macro.file.path])

report_path_data = ReportPathData(db_report.id, path_data)
# TODO: Here we query the File objects with session.get() that runs

@gulyasgergely902 gulyasgergely902 Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should these TODOs be in the code? I'd instead create a ticket for this and add it to the sprint.

# an SQL SELECT statement, since these files are not cached by this
# session object. We should investigate whether it's possible to
# provide a session object that has the File objects already, in
# order to save extra query time.
report_path_data.files.extend(

@barnabasdomozi barnabasdomozi Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Running individual SELECT statements for all the files seems like a performance regression to me. And also considering that this code runs for every report during a store.

The questions are:

  • Why do we need to select the individual File here associated with a file_id? Why not just insert the file_id into the database?
  • Would that be an option to perform a JOIN between the two tables and not individual SELECT statements?

map(lambda fid: session.get(File, fid), used_file_ids))
session.add(report_path_data)

if report.annotations:
self.__validate_and_add_report_annotations(
Expand Down
Loading
Loading