Skip to content
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

Add branch id to code quality reports #286

Merged
merged 12 commits into from
May 29, 2023
33 changes: 33 additions & 0 deletions migrations/Version20230529072916.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

/**
* Auto-generated Migration: Please modify to your needs!
*/
final class Version20230529072916 extends AbstractMigration
{
public function getDescription(): string
{
return '';
}

public function up(Schema $schema): void
{
// this up() migration is auto-generated, please modify it to your needs
$this->addSql('ALTER TABLE code_coverage_report ADD branch_id VARCHAR(255) DEFAULT NULL AFTER commit_hash');
$this->addSql('ALTER TABLE code_inspection_report ADD branch_id VARCHAR(255) DEFAULT NULL AFTER inspection_id');
}

public function down(Schema $schema): void
{
// this down() migration is auto-generated, please modify it to your needs
$this->addSql('ALTER TABLE code_coverage_report DROP branch_id');
$this->addSql('ALTER TABLE code_inspection_report DROP branch_id');
}
}
8 changes: 8 additions & 0 deletions resources/openapi/code-coverage.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@
true,
schema: ['type' => 'string', 'pattern' => '^[a-zA-Z0-9]{6,255}$']
),
new Parameter(
'branchId',
'query',
'The identifier of the branch the report is for. Can either be branch name, MR/PR id, or something else unique to the branch. This ' .
'will be used to find the latest `identifier` within the branch group for a set of commit hashes.',
true,
schema: ['type' => 'string', 'minLength' => 1, 'maxLength' => 255]
),
new Parameter(
'basePath',
'query',
Expand Down
8 changes: 8 additions & 0 deletions resources/openapi/code-inspection.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@
true,
schema: ['type' => 'string', 'minLength' => 1, 'maxLength' => 50]
),
new Parameter(
'branchId',
'query',
'The identifier of the branch the report is for. Can either be branch name, MR/PR id, or something else unique to the branch. This ' .
'will be used to find the latest `identifier` within the branch group for a set of commit hashes.',
true,
schema: ['type' => 'string', 'minLength' => 1, 'maxLength' => 255]
),
new Parameter(
'basePath',
'query',
Expand Down
18 changes: 13 additions & 5 deletions src/Controller/Api/Report/UploadCodeCoverageController.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,24 @@ public function __invoke(UploadCodeCoverageRequest $request, string $repositoryN
throw new NotFoundHttpException();
}

$format = $request->getFormat();
$format = $request->getFormat();
$basePath = $request->getBasePath();
$data = $request->getData();
$branchId = $request->getBranchId();
$data = $request->getData();

$this->logger?->info(
'CodeCoverageReport: {name}, {basePath}, {hash}, {id}, {format}, body size: {size}',
['name' => $repositoryName, 'hash' => $commitHash, 'basePath' => $basePath, 'format' => $format, 'size' => strlen($data)]
'CodeCoverageReport: {name}, {basePath}, {hash}, {branchId}, {format}, body size: {size}',
[
'name' => $repositoryName,
'hash' => $commitHash,
'branchId' => $branchId,
'basePath' => $basePath,
'format' => $format,
'size' => strlen($data)
]
);

$report = $this->reportFactory->parse($repository, $commitHash, $format, $basePath, $data);
$report = $this->reportFactory->parse($repository, $commitHash, $branchId, $format, $basePath, $data);

$this->logger?->info(
'CodeCoverageReport: {name}, creating report with {count} files.',
Expand Down
14 changes: 8 additions & 6 deletions src/Controller/Api/Report/UploadCodeInspectionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,21 +38,23 @@ public function __invoke(UploadCodeInspectionRequest $request, string $repositor
}

$this->logger?->info(
'CodeInspectionReport: {name}, {basePath}, {hash}, {id}, {format}, body size: {size}',
'CodeInspectionReport: {name}, {basePath}, {hash}, {id}, {branchId}, {format}, body size: {size}',
[
'name' => $repositoryName,
'hash' => $commitHash,
'name' => $repositoryName,
'hash' => $commitHash,
'basePath' => $request->getBasePath(),
'id' => $request->getIdentifier(),
'format' => $request->getFormat(),
'size' => strlen($request->getData())
'id' => $request->getIdentifier(),
'branchId' => $request->getBranchId(),
'format' => $request->getFormat(),
'size' => strlen($request->getData())
]
);

$report = $this->reportFactory->parse(
$repository,
$commitHash,
$request->getIdentifier(),
$request->getBranchId(),
$request->getFormat(),
$request->getBasePath(),
$request->getData()
Expand Down
15 changes: 15 additions & 0 deletions src/Entity/Report/CodeCoverageReport.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ class CodeCoverageReport
#[ORM\Column(length: 255)]
private string $commitHash;

#[ORM\Column(length: 255, nullable: true)]
private ?string $branchId = null;

#[ORM\Column]
private int $createTimestamp;

Expand Down Expand Up @@ -64,6 +67,18 @@ public function setCommitHash(string $commitHash): self
return $this;
}

public function getBranchId(): ?string
{
return $this->branchId;
}

public function setBranchId(?string $branchId): self
{
$this->branchId = $branchId;

return $this;
}

public function getCreateTimestamp(): int
{
return $this->createTimestamp;
Expand Down
15 changes: 15 additions & 0 deletions src/Entity/Report/CodeInspectionReport.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ class CodeInspectionReport
#[ORM\Column(length: 50)]
private ?string $inspectionId = null;

#[ORM\Column(length: 255, nullable: true)]
private ?string $branchId = null;

#[ORM\ManyToOne(targetEntity: Repository::class)]
#[ORM\JoinColumn(nullable: false)]
private ?Repository $repository = null;
Expand Down Expand Up @@ -78,6 +81,18 @@ public function setInspectionId(?string $inspectionId): self
return $this;
}

public function getBranchId(): ?string
{
return $this->branchId;
}

public function setBranchId(?string $branchId): self
{
$this->branchId = $branchId;

return $this;
}

public function getRepository(): ?Repository
{
return $this->repository;
Expand Down
89 changes: 79 additions & 10 deletions src/Repository/Report/CodeInspectionReportRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

namespace DR\Review\Repository\Report;

use Doctrine\ORM\Query\ResultSetMappingBuilder;
use Doctrine\Persistence\ManagerRegistry;
use DR\Review\Doctrine\EntityRepository\ServiceEntityRepository;
use DR\Review\Entity\Report\CodeInspectionReport;
Expand All @@ -24,24 +25,92 @@ public function __construct(ManagerRegistry $registry)
}

/**
* For the given set of revisions find all inspection reports and the branchId if available
*
* @param Revision[] $revisions
*
* @return CodeInspectionReport[]
* @return array<array{0: string, 1: string}>
*/
public function findByRevisions(Repository $repository, array $revisions): array
public function findBranchIds(Repository $repository, array $revisions): array
{
// TODO optimize this to select with group by and then order by
$hashes = array_values(array_map(static fn(Revision $rev): string => (string)$rev->getCommitHash(), $revisions));
$reports = $this->findBy(['repository' => $repository, 'commitHash' => $hashes], ['createTimestamp' => 'DESC'], 50);
$hashes = array_values(array_map(static fn(Revision $rev): string => (string)$rev->getCommitHash(), $revisions));
/** @var array<array{inspectionId: string, branchId: string}> $rows */
$rows = $this->getEntityManager()
->createQueryBuilder()
->select(['r.inspectionId', 'r.branchId'])
->from($this->getEntityName(), 'r')
->where('r.commitHash IN (:hashes)')
->andWhere('r.repository = :repositoryId')
->andWhere('r.branchId IS NOT NULL')
->setParameter('hashes', $hashes)
->setParameter('repositoryId', $repository->getId())
->groupBy('r.inspectionId')
->getQuery()
->getArrayResult();

$result = [];
foreach ($reports as $report) {
if (isset($result[$report->getInspectionId()]) === false) {
$result[$report->getInspectionId()] = $report;
}
foreach ($rows as $row) {
$result[] = [$row['inspectionId'], $row['branchId']];
}

return $result;
}

/**
* @param Revision[] $revisions
* @param array<array{0: string, 1: string}> $branchIds
*
* @return CodeInspectionReport[]
*/
public function findByRevisions(Repository $repository, array $revisions, array $branchIds): array
{
$conn = $this->getEntityManager()->getConnection();
$qb = $conn->createQueryBuilder();
$params = [
'hashes' => array_values(array_map(static fn(Revision $rev): string => (string)$rev->getCommitHash(), $revisions)),
'repositoryId' => $repository->getId()
];

$filterExpr = [$qb->expr()->in('commit_hash', ':hashes')];
foreach ($branchIds as $index => [$inspectionId, $branchId]) {
$filterExpr[] = (string)$qb->expr()->and(
$qb->expr()->eq('inspection_id', ':inspectionId' . $index),
$qb->expr()->eq('branch_id', ':branchId' . $index),
);
$params['inspectionId' . $index] = $inspectionId;
$params['branchId' . $index] = $branchId;
}

return array_values($result);
// find most recent report for each inspectionId given commit hashes and branchIds
$subQuery = $conn->createQueryBuilder()
->select('inspection_id', 'MAX(create_timestamp) AS create_timestamp')
->from('code_inspection_report')
->where($qb->expr()->or(...$filterExpr))
->andWhere('repository_id = :repositoryId')
->groupBy('inspection_id');

// inner join to get full entity data
$query = $conn->createQueryBuilder()
->select('report.*')
->from('code_inspection_report', 'report')
->innerJoin(
'report',
sprintf('(%s)', $subQuery->getSQL()),
'filter',
(string)$qb->expr()->and(
$qb->expr()->eq('report.inspection_id', 'filter.inspection_id'),
$qb->expr()->eq('report.create_timestamp', 'filter.create_timestamp')
)
);

// create ResultSetMapping to transform to Entity
$rsm = new ResultSetMappingBuilder($this->getEntityManager());
$rsm->addRootEntityFromClassMetadata(CodeInspectionReport::class, 'report');

/** @var CodeInspectionReport[] $result */
$result = $this->getEntityManager()->createNativeQuery($query->getSQL(), $rsm)->setParameters($params)->getResult();

return $result;
}

public function cleanUp(int $beforeTimestamp): int
Expand Down
6 changes: 6 additions & 0 deletions src/Request/Report/UploadCodeCoverageRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@

class UploadCodeCoverageRequest extends AbstractValidatedRequest
{
public function getBranchId(): ?string
{
return $this->request->query->get('branchId');
}

public function getBasePath(): string
{
return $this->request->query->get('basePath', '');
Expand All @@ -30,6 +35,7 @@ protected function getValidationRules(): ?ValidationRules
[
'query' => [
'basePath' => 'string',
'branchId' => 'string|min:1|max:255',
'format' => 'string|in:' . CoberturaParser::FORMAT
]
]
Expand Down
6 changes: 6 additions & 0 deletions src/Request/Report/UploadCodeInspectionRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ public function getIdentifier(): string
return $this->request->query->get('identifier', '');
}

public function getBranchId(): ?string
{
return $this->request->query->get('branchId');
}

public function getBasePath(): string
{
return $this->request->query->get('basePath', '');
Expand All @@ -37,6 +42,7 @@ protected function getValidationRules(): ?ValidationRules
[
'query' => [
'identifier' => 'required|string|min:1|max:50',
'branchId' => 'string|min:1|max:255',
'basePath' => 'string',
'format' => 'string|in:' . CheckStyleIssueParser::FORMAT . ',' . GitlabIssueParser::FORMAT . ',' . JunitIssueParser::FORMAT
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public function parse(
Repository $repository,
string $commitHash,
string $inspectionId,
?string $branchId,
string $format,
string $basePath,
string $data
Expand All @@ -25,6 +26,7 @@ public function parse(
$report = new CodeInspectionReport();
$report->setRepository($repository);
$report->setInspectionId($inspectionId);
$report->setBranchId($branchId);
$report->setCommitHash($commitHash);
foreach ($issues as $issue) {
$issue->setReport($report);
Expand Down
11 changes: 9 additions & 2 deletions src/Service/Report/Coverage/CodeCoverageReportFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,20 @@ public function __construct(private readonly CodeCoverageParserProvider $parserP
{
}

public function parse(Repository $repository, string $commitHash, string $format, string $basePath, string $data): CodeCoverageReport
{
public function parse(
Repository $repository,
string $commitHash,
?string $branchId,
string $format,
string $basePath,
string $data
): CodeCoverageReport {
$files = $this->parserProvider->getParser($format)->parse($basePath, $data);

$report = new CodeCoverageReport();
$report->setRepository($repository);
$report->setCommitHash($commitHash);
$report->setBranchId($branchId);
foreach ($files as $file) {
$file->setReport($report);
$report->getFiles()->add($file);
Expand Down
3 changes: 2 additions & 1 deletion src/ViewModelProvider/CodeQualityViewModelProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ public function getCodeQualityViewModel(CodeReview $review, string $filePath): C
$revisions = $this->revisionService->getRevisions($review);

// find code inspections
$inspectionReports = $this->reportRepository->findByRevisions($repository, $revisions);
$branchIds = $this->reportRepository->findBranchIds($repository, $revisions);
$inspectionReports = $this->reportRepository->findByRevisions($repository, $revisions, $branchIds);
$issues = [];
if (count($inspectionReports) > 0) {
$issues = $this->issueRepository->findBy(['report' => $inspectionReports, 'file' => $filePath]);
Expand Down
6 changes: 4 additions & 2 deletions src/ViewModelProvider/ReviewSummaryViewModelProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ public function __construct(
*/
public function getSummaryViewModel(CodeReview $review, array $revisions, DirectoryTreeNode $fileTree): ReviewSummaryViewModel
{
$reports = $this->reportRepository->findByRevisions(Assert::notNull($review->getRepository()), $revisions);
$issues = [];
$repository = Assert::notNull($review->getRepository());
$branchIds = $this->reportRepository->findBranchIds($repository, $revisions);
$reports = $this->reportRepository->findByRevisions($repository, $revisions, $branchIds);
$issues = [];
if (count($reports) > 0) {
$filePaths = array_map(static fn(DiffFile $file) => $file->getPathname(), $fileTree->getFilesRecursive());
$issues = $this->issueRepository->findBy(['report' => $reports, 'file' => $filePaths], ['file' => 'ASC']);
Expand Down
Loading