feat: add DAL (CM-951)#3836
feat: add DAL (CM-951)#3836ulemons wants to merge 1 commit intofeat/add-project-discovery-workerfrom
Conversation
b03cb7a to
425e4cb
Compare
bb118d5 to
f4cc9b8
Compare
|
|
3cbe8ec to
0af35ee
Compare
b5c3b03 to
7e6dc18
Compare
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
0af35ee to
82f29d9
Compare
There was a problem hiding this comment.
Pull request overview
Adds new Data Access Layer (DAL) modules for the project catalog and evaluated projects domain, and re-exports them from the DAL package entrypoint.
Changes:
- Introduces
project-catalogDAL: types plus CRUD-ish query helpers (find/insert/bulk insert/upsert/update/delete). - Introduces
evaluated-projectsDAL: types plus query helpers for evaluation lifecycle and bulk insert. - Exports both modules via
services/libs/data-access-layer/src/index.ts.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| services/libs/data-access-layer/src/project-catalog/types.ts | Defines DB-facing types for project catalog records and create/update payloads. |
| services/libs/data-access-layer/src/project-catalog/projectCatalog.ts | Adds SQL helpers for selecting/inserting/upserting/updating/deleting project catalog rows. |
| services/libs/data-access-layer/src/project-catalog/index.ts | Barrel export for the project-catalog module. |
| services/libs/data-access-layer/src/evaluated-projects/types.ts | Defines DB-facing types for evaluated projects and create/update payloads. |
| services/libs/data-access-layer/src/evaluated-projects/evaluatedProjects.ts | Adds SQL helpers for evaluated project operations (find/insert/bulk insert/update/mark evaluated/onboarded/delete). |
| services/libs/data-access-layer/src/evaluated-projects/index.ts | Barrel export for the evaluated-projects module. |
| services/libs/data-access-layer/src/index.ts | Re-exports the newly added modules from the DAL package entrypoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| projectCatalogId: item.projectCatalogId, | ||
| evaluationStatus: item.evaluationStatus ?? 'pending', | ||
| evaluationScore: item.evaluationScore ?? null, | ||
| evaluation: item.evaluation ? JSON.stringify(item.evaluation) : null, |
There was a problem hiding this comment.
In bulkInsertEvaluatedProjects, values pre-serializes evaluation with JSON.stringify(...), but the SQL recordset defines evaluation jsonb. This will cause the inserted JSONB to become a JSON string (or fail coercion), rather than storing the object. Pass the evaluation object directly in values (let JSON.stringify only happen once for the whole values payload) and keep the SQL cast to jsonb.
| evaluation: item.evaluation ? JSON.stringify(item.evaluation) : null, | |
| evaluation: item.evaluation ?? null, |
| export interface IDbProjectCatalog { | ||
| id: string | ||
| projectSlug: string | ||
| repoName: string | ||
| repoUrl: string | ||
| criticalityScore: number | null | ||
| syncedAt: string | null | ||
| createdAt: string | null | ||
| updatedAt: string | null |
There was a problem hiding this comment.
The DB migration for "projectCatalog" defines "ossfCriticalityScore" and "lfCriticalityScore" columns (backend/src/database/migrations/V1770653666__add-automatic_projects_discovery-tables.sql), but this DAL type uses a single criticalityScore field. As written, the queries in this module will select/insert/update a column that doesn't exist. Align the DAL types with the actual schema (e.g., expose both score columns, or rename to match whichever column you intend to persist).
| const PROJECT_CATALOG_COLUMNS = [ | ||
| 'id', | ||
| 'projectSlug', | ||
| 'repoName', | ||
| 'repoUrl', | ||
| 'criticalityScore', | ||
| 'syncedAt', | ||
| 'createdAt', | ||
| 'updatedAt', | ||
| ] |
There was a problem hiding this comment.
PROJECT_CATALOG_COLUMNS includes criticalityScore, but the "projectCatalog" table (per backend/src/database/migrations/V1770653666__add-automatic_projects_discovery-tables.sql) does not have a "criticalityScore" column (it has "ossfCriticalityScore" and "lfCriticalityScore"). This will cause runtime SQL errors on SELECT/RETURNING. Update the selected column list to match the real table columns.
| const PROJECT_CATALOG_COLUMNS = [ | |
| 'id', | |
| 'projectSlug', | |
| 'repoName', | |
| 'repoUrl', | |
| 'criticalityScore', | |
| 'syncedAt', | |
| 'createdAt', | |
| 'updatedAt', | |
| ] | |
| const PROJECT_CATALOG_COLUMNS = [ | |
| 'id', | |
| 'projectSlug', | |
| 'repoName', | |
| 'repoUrl', | |
| 'ossfCriticalityScore', | |
| 'lfCriticalityScore', | |
| 'syncedAt', | |
| 'createdAt', | |
| 'updatedAt', | |
| ] |
| INSERT INTO "projectCatalog" ( | ||
| "projectSlug", | ||
| "repoName", | ||
| "repoUrl", | ||
| "criticalityScore", | ||
| "createdAt", | ||
| "updatedAt" | ||
| ) |
There was a problem hiding this comment.
insertProjectCatalog is inserting into a "criticalityScore" column, which does not exist in the current "projectCatalog" schema (see backend/src/database/migrations/V1770653666__add-automatic_projects_discovery-tables.sql). This INSERT will fail at runtime. Change the INSERT/params to use the actual score column(s) defined by the table.
| INSERT INTO "projectCatalog" ( | ||
| "projectSlug", | ||
| "repoName", | ||
| "repoUrl", | ||
| "criticalityScore", | ||
| "createdAt", | ||
| "updatedAt" | ||
| ) | ||
| VALUES ( | ||
| $(projectSlug), | ||
| $(repoName), | ||
| $(repoUrl), | ||
| $(criticalityScore), | ||
| NOW(), | ||
| NOW() | ||
| ) | ||
| ON CONFLICT ("repoUrl") DO UPDATE SET | ||
| "projectSlug" = EXCLUDED."projectSlug", | ||
| "repoName" = EXCLUDED."repoName", | ||
| "criticalityScore" = EXCLUDED."criticalityScore", | ||
| "updatedAt" = NOW() |
There was a problem hiding this comment.
upsertProjectCatalog uses "criticalityScore" in both the INSERT column list and the ON CONFLICT update, but the table schema defines "ossfCriticalityScore" / "lfCriticalityScore" instead. This will error at runtime and also means conflicts won't update the intended score fields. Update the statement to target the actual column name(s).
| export type IDbEvaluatedProjectCreate = Omit<EvaluatedProjectWritable, 'projectCatalogId'> & { | ||
| projectCatalogId: string | ||
| } & { | ||
| evaluationStatus?: EvaluationStatus | ||
| evaluationScore?: number | ||
| evaluation?: Record<string, unknown> | ||
| evaluationReason?: string | ||
| evaluatedAt?: string | ||
| starsCount?: number | ||
| forksCount?: number | ||
| commitsCount?: number | ||
| pullRequestsCount?: number | ||
| issuesCount?: number | ||
| onboarded?: boolean | ||
| onboardedAt?: string | ||
| } |
There was a problem hiding this comment.
IDbEvaluatedProjectCreate allows onboarded/onboardedAt/evaluatedAt, but the insert/bulk-insert queries in evaluatedProjects.ts never write those columns (only defaults apply). This makes the API misleading and can silently ignore caller intent. Either remove these fields from the create type or include them in the INSERT statements (with appropriate default/null handling).
| evaluationScore?: number | ||
| evaluation?: Record<string, unknown> | ||
| evaluationReason?: string | ||
| evaluatedAt?: string | ||
| starsCount?: number | ||
| forksCount?: number | ||
| commitsCount?: number | ||
| pullRequestsCount?: number | ||
| issuesCount?: number | ||
| onboarded?: boolean | ||
| onboardedAt?: string | ||
| } | ||
|
|
||
| export type IDbEvaluatedProjectUpdate = Partial<{ | ||
| evaluationStatus: EvaluationStatus | ||
| evaluationScore: number | ||
| evaluation: Record<string, unknown> | ||
| evaluationReason: string | ||
| evaluatedAt: string | ||
| starsCount: number | ||
| forksCount: number | ||
| commitsCount: number | ||
| pullRequestsCount: number | ||
| issuesCount: number | ||
| onboarded: boolean | ||
| onboardedAt: string |
There was a problem hiding this comment.
IDbEvaluatedProjectUpdate types nullable DB columns (e.g. evaluationScore, evaluationReason, evaluatedAt, counts) as non-nullable (number/string). That prevents clearing fields (setting to NULL) even though the table schema allows it. Consider typing these as number | null / string | null etc. so updates can intentionally null out values when needed.
| evaluationScore?: number | |
| evaluation?: Record<string, unknown> | |
| evaluationReason?: string | |
| evaluatedAt?: string | |
| starsCount?: number | |
| forksCount?: number | |
| commitsCount?: number | |
| pullRequestsCount?: number | |
| issuesCount?: number | |
| onboarded?: boolean | |
| onboardedAt?: string | |
| } | |
| export type IDbEvaluatedProjectUpdate = Partial<{ | |
| evaluationStatus: EvaluationStatus | |
| evaluationScore: number | |
| evaluation: Record<string, unknown> | |
| evaluationReason: string | |
| evaluatedAt: string | |
| starsCount: number | |
| forksCount: number | |
| commitsCount: number | |
| pullRequestsCount: number | |
| issuesCount: number | |
| onboarded: boolean | |
| onboardedAt: string | |
| evaluationScore?: number | null | |
| evaluation?: Record<string, unknown> | null | |
| evaluationReason?: string | null | |
| evaluatedAt?: string | null | |
| starsCount?: number | null | |
| forksCount?: number | null | |
| commitsCount?: number | null | |
| pullRequestsCount?: number | null | |
| issuesCount?: number | null | |
| onboarded?: boolean | |
| onboardedAt?: string | null | |
| } | |
| export type IDbEvaluatedProjectUpdate = Partial<{ | |
| evaluationStatus: EvaluationStatus | |
| evaluationScore: number | null | |
| evaluation: Record<string, unknown> | null | |
| evaluationReason: string | null | |
| evaluatedAt: string | null | |
| starsCount: number | null | |
| forksCount: number | null | |
| commitsCount: number | null | |
| pullRequestsCount: number | null | |
| issuesCount: number | null | |
| onboarded: boolean | |
| onboardedAt: string | null |
| INSERT INTO "evaluatedProjects" ( | ||
| "projectCatalogId", | ||
| "evaluationStatus", | ||
| "evaluationScore", | ||
| evaluation, | ||
| "evaluationReason", | ||
| "starsCount", | ||
| "forksCount", | ||
| "commitsCount", | ||
| "pullRequestsCount", | ||
| "issuesCount", | ||
| "createdAt", | ||
| "updatedAt" | ||
| ) |
There was a problem hiding this comment.
insertEvaluatedProject ignores onboarded, onboardedAt, and evaluatedAt even though they are part of the writable model/type. This can lead to unexpected behavior if callers pass these fields (they'll be silently dropped). Include these columns in the INSERT (or remove them from the create type if they should only be set via dedicated update helpers).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
services/libs/data-access-layer/src/evaluated-projects/evaluatedProjects.ts
Show resolved
Hide resolved
|
@ulemons please check cursor bugbot comments - I think some are pretty valid. |

Note
Medium Risk
Adds new SQL DAL surfaces for reading/writing
projectCatalogandevaluatedProjects, including bulk insert/upsert and status transitions; risk is mainly around correctness of new queries and JSON/typing conversions against the existing schema.Overview
Adds new DAL modules for
projectCatalogandevaluatedProjects, exposing typed CRUD/query functions (single lookups, listing with pagination, counts, and deletes) and wiring them into the library exports.Includes bulk insert/upsert helpers (via
jsonb_to_recordset) and specialized update helpers such asmarkEvaluatedProjectAsEvaluated,markEvaluatedProjectAsOnboarded, and a join query to fetch pending evaluated projects with catalog metadata.Written by Cursor Bugbot for commit 82f29d9. This will update automatically on new commits. Configure here.