Repository navigation
Conversation
Add active branch preview support for dbt Cloud IDE projects: - New config fields (account_id, base_url, preview_job_id) with frontend form - DbtCloudRestClient for dbt Cloud v2 REST API - Job validation endpoint (POST .../dbt-cloud/validate-preview-job) - Branch preview trigger endpoint (POST .../dbt-cloud/branch-preview) - Background Graphile Worker job for polling and preview creation - Refactor createPreviewWithExplores to use REST client
Comment on lines
+8102
to
+8173
| async validateDbtCloudPreviewJob( | ||
| user: SessionUser, | ||
| projectUuid: string, | ||
| ): Promise<DbtCloudJobValidationResult> { | ||
| const project = | ||
| await this.projectModel.getWithSensitiveFields(projectUuid); | ||
|
|
||
| if (project.dbtConnection.type !== DbtProjectType.DBT_CLOUD_IDE) { | ||
| throw new ParameterError( | ||
| `Project ${projectUuid} is not a dbt Cloud IDE project`, | ||
| ); | ||
| } | ||
|
|
||
| const { | ||
| api_key: apiKey, | ||
| account_id: accountId, | ||
| preview_job_id: previewJobId, | ||
| } = project.dbtConnection; | ||
|
|
||
| if (!accountId || !previewJobId) { | ||
| throw new ParameterError( | ||
| 'Account ID and Preview Job ID must be configured to validate the preview job', | ||
| ); | ||
| } | ||
|
|
||
| const client = new DbtCloudRestClient({ | ||
| serviceToken: apiKey, | ||
| accountId, | ||
| baseUrl: project.dbtConnection.base_url, | ||
| }); | ||
|
|
||
| const errors: string[] = []; | ||
| let job: DbtCloudJobResponse | null = null; | ||
| try { | ||
| job = await client.getJob(previewJobId); | ||
| } catch (e) { | ||
| return { | ||
| isValid: false, | ||
| errors: [ | ||
| `Could not fetch job ${previewJobId}: ${getErrorMessage(e)}`, | ||
| ], | ||
| job: null, | ||
| }; | ||
| } | ||
|
|
||
| if (job.jobType !== 'ci') { | ||
| errors.push( | ||
| `Job "${job.name}" is not a CI job (type: ${job.jobType}). Change job_type to "ci" in dbt Cloud.`, | ||
| ); | ||
| } | ||
|
|
||
| if (!job.deferringEnvironmentId) { | ||
| errors.push( | ||
| `Job "${job.name}" does not defer to a production environment. Set a deferring environment in dbt Cloud.`, | ||
| ); | ||
| } | ||
|
|
||
| const hasDbtParse = job.executeSteps.some((step) => | ||
| step.includes('dbt parse'), | ||
| ); | ||
| if (!hasDbtParse) { | ||
| errors.push( | ||
| `Job "${job.name}" does not include a "dbt parse" step. Add "dbt parse" to the execute steps.`, | ||
| ); | ||
| } | ||
|
|
||
| return { | ||
| isValid: errors.length === 0, | ||
| errors, | ||
| job, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
The validateDbtCloudPreviewJob service method accepts a user: SessionUser parameter but never performs any authorization/permission check on it. Per the backend rules, all service methods must have permission checks (using CASL). Add a permission check (e.g. verifying the user can view/manage the project) before proceeding with the dbt Cloud API call.
Suggested change
| async validateDbtCloudPreviewJob( | |
| user: SessionUser, | |
| projectUuid: string, | |
| ): Promise<DbtCloudJobValidationResult> { | |
| const project = | |
| await this.projectModel.getWithSensitiveFields(projectUuid); | |
| if (project.dbtConnection.type !== DbtProjectType.DBT_CLOUD_IDE) { | |
| throw new ParameterError( | |
| `Project ${projectUuid} is not a dbt Cloud IDE project`, | |
| ); | |
| } | |
| const { | |
| api_key: apiKey, | |
| account_id: accountId, | |
| preview_job_id: previewJobId, | |
| } = project.dbtConnection; | |
| if (!accountId || !previewJobId) { | |
| throw new ParameterError( | |
| 'Account ID and Preview Job ID must be configured to validate the preview job', | |
| ); | |
| } | |
| const client = new DbtCloudRestClient({ | |
| serviceToken: apiKey, | |
| accountId, | |
| baseUrl: project.dbtConnection.base_url, | |
| }); | |
| const errors: string[] = []; | |
| let job: DbtCloudJobResponse | null = null; | |
| try { | |
| job = await client.getJob(previewJobId); | |
| } catch (e) { | |
| return { | |
| isValid: false, | |
| errors: [ | |
| `Could not fetch job ${previewJobId}: ${getErrorMessage(e)}`, | |
| ], | |
| job: null, | |
| }; | |
| } | |
| if (job.jobType !== 'ci') { | |
| errors.push( | |
| `Job "${job.name}" is not a CI job (type: ${job.jobType}). Change job_type to "ci" in dbt Cloud.`, | |
| ); | |
| } | |
| if (!job.deferringEnvironmentId) { | |
| errors.push( | |
| `Job "${job.name}" does not defer to a production environment. Set a deferring environment in dbt Cloud.`, | |
| ); | |
| } | |
| const hasDbtParse = job.executeSteps.some((step) => | |
| step.includes('dbt parse'), | |
| ); | |
| if (!hasDbtParse) { | |
| errors.push( | |
| `Job "${job.name}" does not include a "dbt parse" step. Add "dbt parse" to the execute steps.`, | |
| ); | |
| } | |
| return { | |
| isValid: errors.length === 0, | |
| errors, | |
| job, | |
| }; | |
| } | |
| async validateDbtCloudPreviewJob( | |
| user: SessionUser, | |
| projectUuid: string, | |
| ): Promise<DbtCloudJobValidationResult> { | |
| const project = | |
| await this.projectModel.getWithSensitiveFields(projectUuid); | |
| if ( | |
| user.ability.cannot( | |
| 'manage', | |
| subject('Project', { | |
| organizationUuid: project.organizationUuid, | |
| }), | |
| ) | |
| ) { | |
| throw new ForbiddenError(); | |
| } | |
| if (project.dbtConnection.type !== DbtProjectType.DBT_CLOUD_IDE) { | |
| throw new ParameterError( | |
| `Project ${projectUuid} is not a dbt Cloud IDE project`, | |
| ); | |
| } | |
| const { | |
| api_key: apiKey, | |
| account_id: accountId, | |
| preview_job_id: previewJobId, | |
| } = project.dbtConnection; | |
| if (!accountId || !previewJobId) { | |
| throw new ParameterError( | |
| 'Account ID and Preview Job ID must be configured to validate the preview job', | |
| ); | |
| } | |
| const client = new DbtCloudRestClient({ | |
| serviceToken: apiKey, | |
| accountId, | |
| baseUrl: project.dbtConnection.base_url, | |
| }); | |
| const errors: string[] = []; | |
| let job: DbtCloudJobResponse | null = null; | |
| try { | |
| job = await client.getJob(previewJobId); | |
| } catch (e) { | |
| return { | |
| isValid: false, | |
| errors: [ | |
| `Could not fetch job ${previewJobId}: ${getErrorMessage(e)}`, | |
| ], | |
| job: null, | |
| }; | |
| } | |
| if (job.jobType !== 'ci') { | |
| errors.push( | |
| `Job "${job.name}" is not a CI job (type: ${job.jobType}). Change job_type to "ci" in dbt Cloud.`, | |
| ); | |
| } | |
| if (!job.deferringEnvironmentId) { | |
| errors.push( | |
| `Job "${job.name}" does not defer to a production environment. Set a deferring environment in dbt Cloud.`, | |
| ); | |
| } | |
| const hasDbtParse = job.executeSteps.some((step) => | |
| step.includes('dbt parse'), | |
| ); | |
| if (!hasDbtParse) { | |
| errors.push( | |
| `Job "${job.name}" does not include a "dbt parse" step. Add "dbt parse" to the execute steps.`, | |
| ); | |
| } | |
| return { | |
| isValid: errors.length === 0, | |
| errors, | |
| job, | |
| }; | |
| } | |
Spotted by Graphite (based on custom rule: packages/backend rules)
Is this helpful? React 👍 or 👎 to let us know.
Comment on lines
+8127
to
+8131
| const client = new DbtCloudRestClient({ | ||
| serviceToken: apiKey, | ||
| accountId, | ||
| baseUrl: project.dbtConnection.base_url, | ||
| }); |
Contributor
There was a problem hiding this comment.
A new DbtCloudRestClient is instantiated directly inside validateDbtCloudPreviewJob (and similarly in triggerBranchPreview and pollAndCreateBranchPreview). The backend rules state to prefer dependency injection over direct imports/instantiation between files to enable better testing. DbtCloudRestClient should be injected or created via a factory that is injected into the service.
Suggested change
| const client = new DbtCloudRestClient({ | |
| serviceToken: apiKey, | |
| accountId, | |
| baseUrl: project.dbtConnection.base_url, | |
| }); | |
| const client = this.dbtCloudClientFactory({ | |
| serviceToken: apiKey, | |
| accountId, | |
| baseUrl: project.dbtConnection.base_url, | |
| }); | |
Spotted by Graphite (based on custom rule: packages/backend rules)
Is this helpful? React 👍 or 👎 to let us know.
Comment on lines
+1302
to
+1315
| async triggerDbtCloudBranchPreview( | ||
| @Path() projectUuid: string, | ||
| @Body() body: { branch: string }, | ||
| @Request() req: express.Request, | ||
| ): Promise<ApiSuccess<ApiDbtCloudBranchPreviewResults>> { | ||
| this.setStatus(200); | ||
| const result = await this.services | ||
| .getProjectService() | ||
| .triggerBranchPreview(req.user!, projectUuid, body.branch); | ||
| return { | ||
| status: 'ok', | ||
| results: result, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
The triggerDbtCloudBranchPreview controller method accepts body: { branch: string } but does not validate the branch field using zod before passing it to the service. Per the backend rules, controllers must implement proper request validation using zod. The branch value comes directly from user input and should be validated (e.g. non-empty, safe characters) before use.
Suggested change
| async triggerDbtCloudBranchPreview( | |
| @Path() projectUuid: string, | |
| @Body() body: { branch: string }, | |
| @Request() req: express.Request, | |
| ): Promise<ApiSuccess<ApiDbtCloudBranchPreviewResults>> { | |
| this.setStatus(200); | |
| const result = await this.services | |
| .getProjectService() | |
| .triggerBranchPreview(req.user!, projectUuid, body.branch); | |
| return { | |
| status: 'ok', | |
| results: result, | |
| }; | |
| } | |
| async triggerDbtCloudBranchPreview( | |
| @Path() projectUuid: string, | |
| @Body() body: { branch: string }, | |
| @Request() req: express.Request, | |
| ): Promise<ApiSuccess<ApiDbtCloudBranchPreviewResults>> { | |
| this.setStatus(200); | |
| const { branch } = z | |
| .object({ | |
| branch: z | |
| .string() | |
| .min(1, 'Branch name cannot be empty') | |
| .regex( | |
| /^[a-zA-Z0-9/_.\-]+$/, | |
| 'Branch name contains invalid characters', | |
| ), | |
| }) | |
| .parse(body); | |
| const result = await this.services | |
| .getProjectService() | |
| .triggerBranchPreview(req.user!, projectUuid, branch); | |
| return { | |
| status: 'ok', | |
| results: result, | |
| }; | |
| } | |
Spotted by Graphite (based on custom rule: packages/backend rules)
Is this helpful? React 👍 or 👎 to let us know.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
account_id,base_url,preview_job_id) with frontend form inputsDbtCloudRestClientfor dbt Cloud v2 REST API (trigger runs, poll status, fetch artifacts)POST .../dbt-cloud/validate-preview-job) — checks job type, deferring env, execute stepsPOST .../dbt-cloud/branch-preview) — triggersdbt parseon a feature branchcreatePreviewWithExploresto use REST client instead of hardcodedcloud.getdbt.comURLTest plan
account_id,base_url,preview_job_idpnpm -F common typecheck && pnpm -F backend typecheck && pnpm -F frontend typecheck