fix: Init refactor target -> resource #213
Conversation
WalkthroughThe changes in this pull request focus on renaming various references from "target" to "resource" across multiple files and components in the codebase. This includes updates to function parameters, response structures, authorization checks, and database schema elements. The modifications aim to standardize terminology and improve consistency without altering the underlying logic or error handling mechanisms. The changes span across event workers, web services, database schemas, and authorization utilities, reflecting a comprehensive shift toward a resource-centric naming convention. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (20)
packages/job-dispatch/src/new-target.ts (1)
Complete the target -> resource rename and update all dependencies
The function
dispatchJobsForNewTargetsneeds to be renamed as suggested, and its imports need to be updated in multiple locations:
- Rename
packages/job-dispatch/src/new-target.tstonew-resource.tsand update its contents as suggested- Update imports in dependent files:
packages/job-dispatch/src/index.ts: Update export statementpackages/job-dispatch/src/target.ts: Update import statementpackages/api/src/router/environment.ts: Update import statement from@ctrlplane/job-dispatch🔗 Analysis chain
Line range hint
11-24: Complete the target -> resource renameTo maintain consistency with the broader codebase changes, consider updating all "target" references in this file:
- Rename the file to
new-resource.ts- Update the function signature and implementation:
- export async function dispatchJobsForNewTargets( + export async function dispatchJobsForNewResources( db: Tx, - newTargetIds: string[], + newResourceIds: string[], envId: string, ): Promise<void> { const releaseJobTriggers = await createReleaseJobTriggers(db, "new_resource") - .targets(newTargetIds) + .targets(newResourceIds) .environments([envId]) .insert();
- Update the JSDoc comment:
/** - * Dispatches jobs for new targets added to an environment. + * Dispatches jobs for new resources added to an environment. */Let's verify if there are any imports of this file that would need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all files importing new-target.ts rg -l "new-target" --type tsLength of output: 99
Script:
#!/bin/bash # Check how new-target.ts is imported in index.ts and target.ts rg "new-target" --type ts -A 2 -B 2Length of output: 741
Script:
#!/bin/bash # Check usage of dispatchJobsForNewTargets in target.ts ast-grep --pattern 'dispatchJobsForNewTargets($_)'Length of output: 52
Script:
#!/bin/bash # Check usage of dispatchJobsForNewTargets with more context rg "dispatchJobsForNewTargets" --type ts -A 5 -B 5Length of output: 3853
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)
Line range hint
8-13: Update schema imports to match new terminologyIf the database schema has been renamed from
targettoresource, the imports should be updated accordingly.Apply this change if the schema has been renamed:
import { deployment, environment, job, release, releaseJobTrigger, - target, + resource, } from "@ctrlplane/db/schema";packages/db/src/schema/target-provider.ts (1)
Line range hint
1-1: File name should be updated to match new terminologyThe file name
target-provider.tsshould be renamed toresource-provider.tsto maintain consistency with the new terminology.apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts (1)
Line range hint
1-89: Incomplete refactoring of target -> resourceThe file contains many remaining references to "target" that should be updated for consistency:
- File path:
target-providers- Imports:
createTarget,targetProvider- Schema references:
targetProvider,createTarget- Variable names:
targetsToInsert,targets- Function:
upsertTargetsConsider:
- Creating a tracking issue for the complete rename
- Following a systematic approach:
- Update schema first
- Update imports and type references
- Update API routes and endpoints
- Update variable names
- Update file/folder structure
Would you like me to help create a GitHub issue to track these remaining changes?
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (2)
Line range hint
143-149: Update response structure for API consistencyThe response structure still uses
targetin the final response object. This should be updated to maintain API consistency.return NextResponse.json({ ...je.job, ...je, variables, - target: targetWithMetadata, + resource: targetWithMetadata, approval, });
Schema changes have been identified - table renamed from
targettoresourceThe review comment's concern about database schema compatibility is valid. The recent migration (0035_moaning_supernaut.sql) shows a major schema change where the
targettable and all related tables have been renamed to useresourceterminology:
target→resourcetarget_metadata→resource_metadata- All related foreign key constraints and indexes have been updated accordingly
This change affects the code in question as it's still using the old terminology. The code needs to be updated to use the new schema names.
🔗 Analysis chain
Line range hint
127-134: Verify database schema compatibilityThe code queries the
targetMetadatatable using the old terminology. Before proceeding with the refactoring, we should verify the database schema changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify database schema changes # Check for schema definitions echo "Checking for schema definitions..." rg -A 5 "createTable.*target" --type ts rg -A 5 "createTable.*resource" --type ts # Check for migration files echo "Checking for related migrations..." fd -e sql -e ts --full-path "migration"Length of output: 378
Script:
#!/bin/bash # Let's try a different approach to find schema and migration related files # Search for schema definitions using broader patterns echo "Searching for schema definitions..." rg -l "schema|table.*target|table.*resource" --type ts # Look for Prisma schema files echo "Looking for Prisma schema files..." fd "schema.prisma" # Search for specific database-related terms in the codebase echo "Searching for database-related terms..." rg -l "targetMetadata|target.*table" --type ts # Check for any SQL or migration files echo "Checking for migration files..." fd -e sqlLength of output: 19531
Script:
#!/bin/bash # Let's examine the schema files and migrations for target metadata related changes # Check the target schema definition echo "Checking target schema definition..." cat packages/db/src/schema/target.ts # Check recent migrations for any target metadata related changes echo -e "\nChecking recent migrations..." cat packages/db/drizzle/0035_moaning_supernaut.sql packages/db/drizzle/0036_furry_blue_marvel.sqlLength of output: 18074
packages/api/src/router/release-deploy.ts (1)
Line range hint
1-180: Complete the target → resource refactoringThe refactoring from "target" to "resource" seems incomplete. Consider updating:
- Import statement:
import { ..., target } from "@ctrlplane/db/schema"- Error messages: "Target not found" → "Resource not found"
- Variable name:
t→resource- Procedure name:
toTarget→toResourceThis would maintain consistency with the new terminology.
Here's a suggested refactor:
- import { ..., target } from "@ctrlplane/db/schema"; + import { ..., resource } from "@ctrlplane/db/schema"; - toTarget: protectedProcedure + toResource: protectedProcedure - const t = await ctx.db + const resource = await ctx.db .select() - .from(target) - .where(eq(target.id, input.targetId)) + .from(resource) + .where(eq(resource.id, input.targetId)) .then(takeFirstOrNull); - if (!t) throw new Error("Target not found"); + if (!resource) throw new Error("Resource not found"); - if (t.lockedAt != null) throw new Error("Target is locked"); + if (resource.lockedAt != null) throw new Error("Resource is locked");packages/api/src/router/target-metadata-group.ts (1)
Line range hint
1-242: Consider database schema implicationsThe file contains multiple references to database tables and columns with the "target" prefix (e.g.,
targetMetadata,targetMetadataGroup). While this PR focuses on authorization types, the database schema will likely need to be updated in a future PR to maintain consistency with the new "resource" terminology.Consider:
- Planning the database schema migration strategy
- Ensuring backward compatibility during the transition
- Coordinating the changes with any ORM or query builder configurations
packages/db/src/schema/target.ts (6)
Line range hint
38-63: Ensure consistent renaming throughout the schema definitionWhile the table name has been changed to "resource", there are several references that still use the old "target" naming:
- Foreign key reference to
targetProvider- Variable name
targetin the code- Types and schemas with 'Target' prefix
Consider updating these references for consistency:
-export const target = pgTable( +export const resource = pgTable( "resource", { // ... - providerId: uuid("provider_id").references(() => targetProvider.id, { + providerId: uuid("provider_id").references(() => resourceProvider.id, { // ... } ); -export type Target = InferSelectModel<typeof target>; +export type Resource = InferSelectModel<typeof resource>; -export const createTarget = createInsertSchema(target, { +export const createResource = createInsertSchema(resource, {
Line range hint
87-98: Update export constant name for consistencyThe table name has been updated to "resource_schema", but the export constant still uses the old naming.
-export const targetSchema = pgTable( +export const resourceSchema = pgTable(
Line range hint
100-116: Update view-related types and functions for consistencyWhile the table name has been updated to "resource_view", related types and functions still use the old naming:
TargetViewtypeTargetConditiontype for the filter fieldcreateTargetViewandupdateTargetViewfunctions-export type TargetView = InferSelectModel<typeof targetView>; +export type ResourceView = InferSelectModel<typeof resourceView>; -export const createTargetView = createInsertSchema(targetView, { - filter: targetCondition, +export const createResourceView = createInsertSchema(resourceView, { + filter: resourceCondition, }).omit({ id: true }); -export const updateTargetView = createTargetView.partial(); +export const updateResourceView = createResourceView.partial();
Line range hint
119-128: Update metadata-related column names and functionsThe table has been renamed to "resource_metadata", but several related elements still use the old naming:
- Column name "target_id"
- Relations and helper functions
export const targetMetadata = pgTable( "resource_metadata", { id: uuid("id").primaryKey().defaultRandom().notNull(), - targetId: uuid("target_id") + resourceId: uuid("resource_id") .references(() => resource.id, { onDelete: "cascade" }) .notNull(), // ... } ); -export const targetMetadataRelations = relations(targetMetadata, ({ one }) => ({ - target: one(target, { - fields: [targetMetadata.targetId], - references: [target.id], +export const resourceMetadataRelations = relations(resourceMetadata, ({ one }) => ({ + resource: one(resource, { + fields: [resourceMetadata.resourceId], + references: [resource.id], }), })); -const buildMetadataCondition = (tx: Tx, cond: MetadataCondition): SQL => { +const buildMetadataCondition = (tx: Tx, cond: MetadataCondition): SQL => { // Update references in the function body // ... };Also applies to: 134-196
Line range hint
232-257: Update relationship-related column names and typesWhile the table and enum names have been updated, several elements still use the old naming:
- Column names "source_id" and "target_id"
- Types and functions with "Target" prefix
-export const targetRelationshipType = pgEnum("resource_relationship_type", [ +export const resourceRelationshipType = pgEnum("resource_relationship_type", [ "associated_with", "depends_on", ]); -export const targetRelationship = pgTable( +export const resourceRelationship = pgTable( "resource_relationship", { id: uuid("id").primaryKey().defaultRandom(), - sourceId: uuid("source_id") - .references(() => target.id, { onDelete: "cascade" }) + sourceId: uuid("source_id") + .references(() => resource.id, { onDelete: "cascade" }) .notNull(), - targetId: uuid("target_id") - .references(() => target.id, { onDelete: "cascade" }) + destinationId: uuid("destination_id") + .references(() => resource.id, { onDelete: "cascade" }) .notNull(), - type: targetRelationshipType("type").notNull(), + type: resourceRelationshipType("type").notNull(), }, ); -export const createTargetRelationship = createInsertSchema(targetRelationship).omit({ id: true }); +export const createResourceRelationship = createInsertSchema(resourceRelationship).omit({ id: true }); -export const updateTargetRelationship = createTargetRelationship.partial(); +export const updateResourceRelationship = createResourceRelationship.partial(); -export type TargetRelationship = InferSelectModel<typeof targetRelationship>; +export type ResourceRelationship = InferSelectModel<typeof resourceRelationship>;
Line range hint
260-285: Update variable-related column names and typesThe table has been renamed to "resource_variable", but several elements still use the old naming:
- Column name "target_id"
- Types and functions with "Target" prefix
export const targetVariable = pgTable( "resource_variable", { id: uuid("id").primaryKey().defaultRandom(), - targetId: uuid("target_id") - .references(() => target.id, { onDelete: "cascade" }) + resourceId: uuid("resource_id") + .references(() => resource.id, { onDelete: "cascade" }) .notNull(), // ... } ); -export const targetVariableRelations = relations(targetVariable, ({ one }) => ({ - target: one(target, { - fields: [targetVariable.targetId], - references: [target.id], +export const resourceVariableRelations = relations(resourceVariable, ({ one }) => ({ + resource: one(resource, { + fields: [resourceVariable.resourceId], + references: [resource.id], }), })); -export const createTargetVariable = createInsertSchema(targetVariable, { +export const createResourceVariable = createInsertSchema(resourceVariable, { value: z.union([z.string(), z.number(), z.boolean()]), }).omit({ id: true }); -export const updateTargetVariable = createTargetVariable.partial(); +export const updateResourceVariable = createResourceVariable.partial(); -export type TargetVariable = InferSelectModel<typeof targetVariable>; +export type ResourceVariable = InferSelectModel<typeof resourceVariable>;packages/db/drizzle/0036_furry_blue_marvel.sql (1)
7-19: Consider reordering operations for safer migration.While the renaming is consistent, consider reordering the operations to:
- Drop foreign key constraints first
- Perform table/column renames
- Recreate constraints and indexes
This order minimizes the risk of dependency-related issues during migration.
packages/db/src/schema/environment.ts (1)
41-43: Consider a comprehensive rename strategyThe current change only renames the database column while retaining the old terminology in types (
TargetCondition), imports (@ctrlplane/validators/targets), and validation functions (isValidTargetCondition). Consider:
- Updating type definitions to use "resource" terminology
- Creating new validation functions with updated names
- Deprecating old types/functions with a migration period
- Updating API contracts if this field is exposed
This will ensure consistency across the entire codebase.
packages/auth/src/utils/rbac.ts (2)
211-211: Systematic naming inconsistency across scope handlersThe inconsistency between "target" and "resource" terminology appears systematic across multiple handlers. A comprehensive refactoring is needed to ensure consistency across:
- Function names
- Database schema references
- Return types
Consider using a migration strategy:
- Update database schema first
- Create new functions with "resource" naming
- Deprecate old functions
- Update all references
- Remove deprecated functions
Also applies to: 225-225
Line range hint
1-396: Incomplete refactoring needs comprehensive approachThe current changes only update return types to use "resource" terminology while leaving function names and database schema references using "target". This partial refactoring could lead to:
- Runtime errors due to mismatched property access
- Maintenance confusion due to mixed terminology
- Potential issues in type checking and IDE support
Recommend:
- Create a tracking issue for the complete refactoring
- Document all places requiring updates (functions, schemas, types)
- Plan the migration in stages to maintain backward compatibility
- Add tests to verify the refactoring doesn't break existing functionality
packages/api/src/router/deployment-variable.ts (1)
295-295: Complete the target -> resource refactoringWhile the authorization check update from "target" to "resource" aligns with the PR objectives, there's an inconsistency as the procedure name
byTargetIdand its implementation still use the "target" terminology. Consider completing the refactoring for consistency.Apply these changes to maintain consistent terminology:
- byTargetId: protectedProcedure + byResourceId: protectedProcedureAlso, consider updating the related database queries and variable names from
targettoresourcein a follow-up PR to maintain consistency throughout the codebase.packages/api/src/router/deployment.ts (1)
Line range hint
1-500: Consider using a systematic refactoring approachThe current changes show an incomplete transition from "target" to "resource" terminology. To ensure consistency and prevent bugs, consider:
- Using a systematic approach to identify all occurrences
- Creating a migration plan that includes:
- Database schema changes
- API endpoint updates
- Documentation updates
- Test updates
- Using TypeScript's type system to catch inconsistencies
Would you like help creating a comprehensive checklist for this refactoring effort?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
apps/event-worker/src/target-scan/index.ts(2 hunks)apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts(1 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts(2 hunks)apps/webservice/src/app/api/v1/targets/[targetId]/route.ts(3 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts(2 hunks)packages/api/src/router/deployment-variable.ts(1 hunks)packages/api/src/router/deployment.ts(2 hunks)packages/api/src/router/job.ts(6 hunks)packages/api/src/router/release-deploy.ts(1 hunks)packages/api/src/router/release.ts(1 hunks)packages/api/src/router/target-metadata-group.ts(3 hunks)packages/api/src/router/target-provider.ts(5 hunks)packages/api/src/router/target.ts(12 hunks)packages/auth/src/utils/rbac.ts(7 hunks)packages/db/drizzle/0036_furry_blue_marvel.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment-variables.ts(1 hunks)packages/db/src/schema/environment.ts(1 hunks)packages/db/src/schema/rbac.ts(2 hunks)packages/db/src/schema/release.ts(2 hunks)packages/db/src/schema/target-group.ts(1 hunks)packages/db/src/schema/target-provider.ts(2 hunks)packages/db/src/schema/target.ts(6 hunks)packages/job-dispatch/src/new-target.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/api/src/router/deployment.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-11-10T20:33:41.576Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
packages/db/src/schema/rbac.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-11-10T20:33:41.576Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
🔇 Additional comments (46)
packages/db/src/schema/target-group.ts (2)
Line range hint 8-28: Consider updating TypeScript identifiers for consistency.
While the database table has been renamed to use "resource", the TypeScript identifiers still use "target". For better maintainability and consistency, consider renaming:
targetMetadataGroup→resourceMetadataGroupcreateTargetMetadataGroup→createResourceMetadataGroupupdateTargetMetadataGroup→updateResourceMetadataGroupTargetMetadataGroup→ResourceMetadataGroup
Let's check for any dependencies on these types:
#!/bin/bash
# Description: Find usages of the current type names to assess the scope of the change
echo "Checking TypeScript/JavaScript files for usages:"
rg -l "targetMetadataGroup|TargetMetadataGroup" --type ts --type js8-8: Verify database migration for table rename.
The table name change from "target_metadata_group" to "resource_metadata_group" looks good, aligning with the PR objective.
Let's verify the migration script exists:
✅ Verification successful
Migration script for table rename is present and complete
The migration script 0036_furry_blue_marvel.sql contains the necessary table rename from "target_metadata_group" to "resource_metadata_group" along with proper constraint and foreign key updates. This is part of a comprehensive migration that renames all "target" related tables to "resource".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for migration file containing the table rename
# Expected: Find a migration file with ALTER TABLE statement
rg -l 'ALTER TABLE.*target_metadata_group.*TO.*resource_metadata_group' --type sql
Length of output: 849
Script:
#!/bin/bash
# Let's check for migration files in common locations
fd -e sql migration
# Also check for any SQL files containing table rename patterns
rg -l 'RENAME TABLE|ALTER TABLE.*RENAME' --type sql
Length of output: 300
Script:
#!/bin/bash
# Let's check the content of these migration files, focusing on the most recent ones first
cat packages/db/drizzle/0036_furry_blue_marvel.sql
cat packages/db/drizzle/0025_low_talisman.sql
Length of output: 9224
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)
50-50:
Potential inconsistency in target/resource naming
While the response mapping uses row.resource, the code still imports and joins with the target table. This inconsistency suggests that the refactoring might be incomplete.
Let's verify the database schema changes:
packages/db/src/schema/target-provider.ts (3)
Line range hint 1-60: Consider renaming TypeScript identifiers for consistency
While the database tables have been renamed to use "resource", the TypeScript identifiers still use "target". This mixed terminology could be confusing. Consider updating:
targetProvider→resourceProvidertargetProviderRelations→resourceProviderRelationscreateTargetProvider→createResourceProviderTargetProvider→ResourceProvidertargetProviderGoogle→resourceProviderGooglecreateTargetProviderGoogle→createResourceProviderGoogleupdateTargetProviderGoogle→updateResourceProviderGoogleTargetProviderGoogle→ResourceProviderGoogle
#!/bin/bash
# Check for any files importing these types/variables that would need updates
rg "import.*\{.*(targetProvider|TargetProvider).*\}" --type ts40-42: Verify foreign key relationships after table rename
The foreign key column rename from "target_provider_id" to "resource_provider_id" needs to be properly migrated along with existing relationships.
#!/bin/bash
# Check for any remaining references to the old column name
rg "target_provider_id" --type ts18-18: Verify database migration for table rename
The table name change from "target_provider" to "resource_provider" needs a proper migration strategy to handle existing data.
Consider using a database migration tool like prisma or flyway if not already in use, to safely handle such schema changes.
✅ Verification successful
Let's also check if there are any Prisma schema files or other migration tools being used:
Let's also examine the current file more closely to understand the context:
Migration for table rename is properly handled
The table rename from "target_provider" to "resource_provider" is properly handled in migration file 0036_furry_blue_marvel.sql. The migration includes:
- Renaming the table:
ALTER TABLE "target_provider" RENAME TO "resource_provider" - Updating all related foreign key constraints
- Recreating indexes with the new name
- Handling related tables like
resource_provider_google
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for migration files that handle this table rename
fd -e sql -e ts migration | rg -l "target_provider.*resource_provider"
Length of output: 127
Script:
#!/bin/bash
# Let's try a more comprehensive search for migration files
fd migration
# Then search for any files containing target_provider or resource_provider
rg -l "target_provider|resource_provider"
Length of output: 2060
Script:
#!/bin/bash
# Look for Prisma schema files
fd schema.prisma
# Check for other common migration tool configurations
fd -e flyway.conf -e liquibase.properties -e dbmate.yml
Length of output: 76
Script:
#!/bin/bash
# Get the content of the target-provider schema file
cat packages/db/src/schema/target-provider.ts
Length of output: 1912
Script:
#!/bin/bash
# Let's check the latest migration file that might contain the rename
cat packages/db/drizzle/0036_furry_blue_marvel.sql
# Also check the content of the target.ts schema to understand the relationship
cat packages/db/src/schema/target.ts
Length of output: 16733
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts (1)
75-75: Consider updating route path structure
While the authorization type has been updated to "resource", the API route path still uses "targets" (/workspaces/[workspaceId]/targets/). This might need to be updated for complete consistency.
#!/bin/bash
# Description: Check for other API routes that might need path updates
# and verify the impact of potential route path changes
# Find all route.ts files in the API directory
fd route.ts apps/webservice/src/app/api
# Check for "targets" in route paths
rg -l "targets" apps/webservice/src/app/apiapps/event-worker/src/target-scan/index.ts (3)
50-50: LGTM: Logging message updates are consistent
The renaming from target_provider to resource_provider in the logging messages is consistent with the refactoring objective.
Also applies to: 72-72, 78-78, 83-83
55-60: Verify GKE scanning functionality after renaming
The renaming from target_provider_google to resource_provider_google affects the GKE scanning logic. While the change is consistent, ensure that the getGkeTargets function and its consumers are updated accordingly.
✅ Verification successful
Based on the output, I can now generate the final response as I have sufficient information to verify the concern. The search results show that the codebase has been consistently updated with the new terminology:
GKE scanning functionality is properly updated after renaming
The renaming from target_provider_google to resource_provider_google has been consistently implemented across the codebase:
- The database schema in
packages/db/src/schema/target-provider.tsuses the new terminology - The GKE scanning function in
apps/event-worker/src/target-scan/gke.tscorrectly acceptsTargetProviderGoogletype - The caller in
apps/event-worker/src/target-scan/index.tsusestp.resource_provider_googleconsistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check GKE-related files for consistent terminology
echo "Checking GKE-related files..."
rg -A 5 "getGkeTargets|GkeTarget" --type ts
# Check for any remaining Google provider references
echo "Checking Google provider references..."
rg -A 5 "provider_google|ProviderGoogle" --type ts
Length of output: 14374
Line range hint 1-94: Consider completing the resource terminology refactoring
While the changes to resource_provider references are consistent, there are several remaining instances of "target" terminology that should be updated for consistency:
-
File and directory structure:
target-scan/index.ts→resource-scan/index.ts
-
Type and import references:
InsertTarget→InsertResourceTargetScanEvent→ResourceScanEventtargetProvider→resourceProvidertargetProviderGoogle→resourceProviderGoogle
-
Function and variable names:
getGkeTargets→getGkeResourcesupsertTargets→upsertResourcestargetsarray →resourcesarray
-
Queue and channel names:
Channel.TargetScan→Channel.ResourceScantargetScanQueue→resourceScanQueue
Let's verify the extent of this refactoring across the codebase:
apps/webservice/src/app/api/v1/targets/[targetId]/route.ts (4)
68-68: Verify permission enum alignment with resource type
Similar to the GET handler, there's an inconsistency between using "resource" type with Permission.TargetUpdate.
94-94: Verify permission enum alignment with resource type
Similar to the GET and PATCH handlers, there's an inconsistency between using "resource" type with Permission.TargetDelete.
Line range hint 1-107: Consider completing the target → resource refactoring
The current changes only modify the authorization type from "target" to "resource", but several related elements still use "target":
- File path and route structure (
targets/[targetId]) - Permission enums (
Permission.TargetGet, etc.) - Database schema and queries (
schema.target,db.query.target) - Variable names and parameters (
targetId)
This partial refactoring could lead to confusion and maintenance issues. Consider:
- Creating a migration plan to update all related terminology consistently
- Updating database schema and migrations
- Refactoring route structures and parameters
- Updating permission enums
#!/bin/bash
# Check for remaining "target" references in the codebase
echo "Files still using 'target' in paths:"
fd -t f target
echo "\nCode still using 'target' in schema and queries:"
rg 'schema\.target|query\.target'
echo "\nPermission enums still using 'Target':"
rg 'Permission\.Target'20-20: Verify permission enum alignment with resource type
The authorization type has been changed to "resource" but the permission enum Permission.TargetGet still uses the "Target" prefix. This inconsistency in terminology could lead to confusion.
packages/db/src/schema/deployment-variables.ts (2)
63-65:
Fix inconsistency between column name and schema validation
While the column has been renamed from targetFilter to resource_filter, the schema validation still uses targetFilter. This inconsistency should be addressed.
Apply this diff to align the schema validation with the new column name:
export const createDeploymentVariableValue = createInsertSchema(
deploymentVariableValue,
- { targetFilter: targetCondition },
+ { resource_filter: targetCondition },
)Likely invalid or redundant comment.
63-65: Verify the impact of column rename
The column rename from targetFilter to resource_filter might require database migrations and updates to related queries.
Let's verify the broader impact:
✅ Verification successful
Let me gather more specific information about the database migrations and schema changes.
Column rename is already handled with proper migration
The column rename from targetFilter to resource_filter has been properly implemented:
- Migration file
0036_furry_blue_marvel.sqlincludes the column rename for bothdeployment_variable_valueandenvironmenttables - Schema definitions in both
deployment-variables.tsandenvironment.tsare correctly usingjsonb("resource_filter")while maintaining the field name astargetFilterin the TypeScript interface - The migration appears to be part of a broader rename from "target" to "resource" across the schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to targetFilter and related database queries
# Check for any remaining references to targetFilter
rg -l "targetFilter"
# Check for SQL migrations related to this change
fd -e sql -e ts -p "migration|migrate" -x rg -l "targetFilter|resource_filter" {}
# Check for related query implementations
ast-grep --pattern 'select($$$).from($$$).where({ targetFilter: $$$ })'
Length of output: 1781
Script:
#!/bin/bash
# Check for migration files in the db package
fd -e sql -e ts migration packages/db/src -x cat {}
# Check for specific schema definitions and queries using resource_filter
rg -A 5 "resource_filter"
# Look for Drizzle schema definitions related to this column
ast-grep --pattern 'jsonb("resource_filter")'
Length of output: 3172
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
92-92:
Inconsistent renaming of target to resource
While you've changed row.target to row.resource here, this appears to be an incomplete refactoring. The rest of the file still uses the "target" terminology.
Let's verify the extent of this refactoring across the codebase:
Consider completing the refactoring by updating:
- Import statements (target, targetMetadata)
- Database query joins (target table)
- Variable names (targetWithMetadata, jobTargetMetadataRows)
- Response structure (target field)
Would you like me to provide a complete diff for these changes?
packages/api/src/router/release-deploy.ts (2)
Line range hint 112-117: LGTM: Improved input validation with UUID checks
The addition of UUID validation for all ID fields is a good improvement that ensures data consistency and catches invalid inputs early.
111-111:
Maintain consistency in resource/target terminology
While the authorization check has been updated to use "resource", the procedure name and schema references still use "target". This partial update could lead to confusion.
Let's verify the consistency of this renaming across the codebase:
packages/db/drizzle/meta/_journal.json (1)
256-262: LGTM! Verify the associated migration file.
The new migration entry follows the established pattern and correctly tracks the schema changes for renaming "target" to "resource".
Let's verify the associated migration file exists and contains the expected changes:
✅ Verification successful
Migration file correctly implements the target->resource renaming
The migration file 0036_furry_blue_marvel.sql exists and contains all the expected changes:
- Adds new enum values with "resource" prefix
- Renames all "target" tables to "resource" tables
- Updates foreign key constraints and indexes
- Renames columns containing "target" to use "resource"
- Maintains referential integrity by properly handling cascading deletes and updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration file exists and contains the target->resource renaming
# Check if the migration file exists
fd --type f "0036_furry_blue_marvel.sql" packages/db/drizzle
# Check the content of the migration file for target->resource renaming
rg -i "target|resource" $(fd --type f "0036_furry_blue_marvel.sql" packages/db/drizzle)
Length of output: 7010
packages/api/src/router/target-metadata-group.ts (3)
212-212: LGTM: Authorization type change is consistent
The change maintains consistency with the authorization type renaming pattern established in the byId procedure.
234-234: LGTM: Authorization type change is consistent
The change maintains consistency with the authorization type renaming pattern established in other procedures.
116-116: Verify the scope of target -> resource renaming
While the authorization type has been updated to use "resourceMetadataGroup", the procedure names and schema still use the "target" prefix. This appears to be an intentional partial rename focusing only on authorization types.
Consider creating a follow-up issue to track the remaining renames (schema, procedure names, etc.) if they're intentionally left for a separate PR to manage the scope of changes.
✅ Verification successful
The search results show a consistent pattern where authorization types have been renamed from "target" to "resource" across multiple router files, while keeping the file names, procedure names, and schemas unchanged. This appears to be an intentional approach to handle the migration in phases, focusing first on authorization types.
Let me verify if there are any remaining "target" authorization types to ensure the rename is complete:
Authorization type changes are consistent with the broader refactoring
The authorization type change from "targetMetadataGroup" to "resourceMetadataGroup" is part of a consistent pattern across the codebase. All authorization types have been renamed from "target" to "resource" variants, while intentionally preserving file names, procedure names, and schemas for a phased migration approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this pattern of partial renaming is consistent across other files
rg -A 2 "type: \"resource" packages/api/src/router/
Length of output: 4593
Script:
#!/bin/bash
# Check for any remaining "targetMetadataGroup" or similar authorization types
rg -i 'type: "target' packages/api/src/router/
Length of output: 48
packages/api/src/router/target-provider.ts (4)
105-105: Authorization type change - see previous comment
212-212: Authorization type change - see previous comment
88-88: Verify authorization type change consistency
The authorization type has been changed to "resourceProvider", but we need to ensure this is consistent with the Permission system.
✅ Verification successful
Authorization type change is consistent with database schema
The change from "targetProvider" to "resourceProvider" in the authorization check is correct and consistent with the codebase. The evidence shows:
- The database table is named "resource_provider" (packages/db/src/schema/target-provider.ts)
- All RBAC checks consistently use "resourceProvider" type (packages/auth/src/utils/rbac.ts)
- The Permission enum contains "TargetList" which is being used correctly with the "resourceProvider" type
The change aligns the authorization check with the actual database schema and existing RBAC implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other authorization type references to ensure consistency
rg --type typescript "type: \"(target|resource)Provider\"" -B 2 -A 2
# Check Permission enum definition
ast-grep --pattern 'enum Permission {
$$$
TargetList
$$$
}'
Length of output: 667
Script:
#!/bin/bash
# Search for authorization type references with proper typescript extension
rg --type ts "type: \"(target|resource)Provider\"" -B 2 -A 2
# Check Permission enum definition in typescript files
rg --type ts "enum Permission" -A 10
Length of output: 2750
Script:
#!/bin/bash
# Let's see more of the Permission enum to find target-related permissions
rg --type ts "enum Permission" -A 30
# Also check for any remaining "targetProvider" references that might need updating
rg --type ts "targetProvider" -B 2 -A 2
Length of output: 32405
Line range hint 1-240: Consider completing the target → resource renaming
The file still contains many references to "target" in variable names, imports, and queue names (e.g., targetScanQueue, createTargetProvider, etc.). Consider whether these should also be renamed for consistency.
packages/db/src/schema/release.ts (2)
188-190:
Inconsistency: Column renamed but referenced table remains unchanged.
While the column has been renamed from targetId to resource_id, it still references the target table. This suggests an incomplete refactoring:
- Inconsistent naming between the column and the referenced table
- Need to ensure this change is part of a proper database migration
163-164: Verify the enum value changes across the codebase.
The enum value changes from "new_target" to "new_resource" and "target_changed" to "resource_changed" look good, but let's ensure all usages are updated.
✅ Verification successful
The enum value changes are part of an ongoing migration from "target" to "resource"
The verification shows that:
- The new enum values
new_resourceandresource_changedare properly defined in the schema - The new values are already being used in
packages/job-dispatch/src/new-target.ts - However, there's an extensive usage of "target" throughout the codebase (file names, components, APIs) indicating this is part of a larger, ongoing migration
- The inconsistency between the enum values and the rest of the codebase appears intentional as part of a gradual transition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old enum values
rg -i "new_target|target_changed" --type ts
Length of output: 45
Script:
#!/bin/bash
# Let's verify the usage of the new enum values to ensure they are properly used
rg -i "new_resource|resource_changed" --type ts -A 2 -B 2
Length of output: 880
Script:
#!/bin/bash
# Let's check if there are any inconsistencies in file names or paths still using "target"
fd target
Length of output: 4948
packages/db/src/schema/target.ts (1)
Line range hint 1-24: Verify updates in dependent packages
The file imports types from "@ctrlplane/validators/targets" that still use the old "target" naming:
TargetConditionTargetFilterTypetargetCondition
Run the following script to check for related files that might need updates:
packages/db/drizzle/0036_furry_blue_marvel.sql (1)
1-127: Verify complete coverage of target->resource renaming.
Let's verify that all references to "target" have been updated consistently across the schema.
packages/db/src/schema/environment.ts (2)
41-43: Verify database migration path
This schema change requires careful handling of:
- Existing data migration
- Query updates
- API compatibility
Please ensure that:
- A database migration script exists
- The migration handles existing data correctly
- API consumers are notified of the change
Let's verify the migration setup:
#!/bin/bash
# Check for related migration files
fd -e sql . | xargs rg "resource_filter|target_filter"
# Look for potentially affected queries
rg "targetFilter|resource_filter" --type typescript -B 2 -A 241-43:
Inconsistent naming between database column and TypeScript types
While the database column has been renamed from targetFilter to resource_filter, the type annotation still uses TargetCondition. This creates an inconsistency in terminology between the database layer and the type system.
Let's verify if similar type definitions exist and need updates:
packages/api/src/router/release.ts (2)
Line range hint 1-24: Verify status of schema renaming
The file still imports and uses the target schema from "@ctrlplane/db/schema". This seems inconsistent with the target -> resource refactoring effort.
122-122: Verify the impact of response structure change
Since this modifies the response structure of the list procedure, we should verify all consumers of this endpoint.
✅ Verification successful
Based on the search results, we can see that the target: j.resource field is part of the release job trigger mapping in the list procedure, and the release router is properly integrated into the root router. While we didn't find direct consumers in the frontend, we have enough context about the structure change to make a determination.
Response structure change is safe
The change from target: j.resource appears to be a straightforward field mapping in the release router's list procedure, where we're mapping the resource field to the target property. This is an internal transformation that maintains consistency with the expected schema.
- The field is properly mapped from the database resource
- The router is correctly integrated into the application's root router
- No breaking changes to type definitions were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of the releaseRouter.list procedure and any references to its response structure
# Search for direct usage of the list procedure
echo "Searching for direct usage of releaseRouter.list..."
rg -l "releaseRouter\W+list|useQuery.*list" --type ts
# Search for any type definitions or interfaces that might include this structure
echo "Searching for type definitions that might need updates..."
ast-grep --pattern 'interface $name {
$$$
target: $type
$$$
}'
# Search for test assertions that might be affected
echo "Searching for test files that might need updates..."
rg -l "\.target\W" --type test
Length of output: 539
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Search for any files that might be consuming the release router
echo "Searching for files importing or using release router..."
rg -l "from.*router/release|releaseRouter" --type typescript
# 2. Search for interface definitions related to releases
echo "Searching for release-related interfaces..."
rg -l "interface.*Release|type.*Release" --type typescript
# 3. Search for test files specifically
echo "Searching for release-related test files..."
fd -e test.ts -e spec.ts | xargs rg -l "target"
# 4. Search for any usage of target property in typescript files
echo "Searching for usage of target property..."
rg -l "\.target\b" --type typescript
Length of output: 860
Script:
#!/bin/bash
# Adjust search with correct file type 'ts' instead of 'typescript'
# 1. Search for any files that might be consuming the release router
echo "Searching for files importing or using release router..."
rg -l "from.*router/release|releaseRouter" --type ts
# 2. Search for interface definitions related to releases
echo "Searching for release-related interfaces..."
rg -A 5 "interface.*Release|type.*Release" --type ts
# 3. Look at the context of target usage in the found test files
echo "Examining target usage in test files..."
rg -C 3 "target" packages/job-dispatch/src/__test__/job-variables-deployment.test.ts integrations/terraform-cloud-scanner/src/__tests__/scanner.test.ts
# 4. Search for any usage of target property in ts files
echo "Searching for usage of target property..."
rg -l "\.target\b" --type ts
Length of output: 86873
Script:
#!/bin/bash
# Let's examine the specific usage of 'target' in the release router and related files
# 1. Look at the release router implementation
echo "Examining release router implementation..."
rg -C 5 "target: j\.resource" packages/api/src/router/release.ts
# 2. Look at the root router where release router is used
echo "Examining root router..."
rg -C 5 "releaseRouter" packages/api/src/root.ts
# 3. Look for any direct consumers of the list procedure
echo "Looking for consumers of list procedure..."
rg -C 5 "useQuery.*list.*release" --type ts apps/
# 4. Check for any type definitions that might be affected
echo "Checking type definitions..."
rg -C 5 "type.*Release.*\{" packages/api/src/router/release.ts
Length of output: 1719
packages/auth/src/utils/rbac.ts (2)
357-357: Complex query still references 'target' table
The job scopes implementation changes the return type to "resource" but still joins with the target table. This inconsistency in a complex query with multiple joins needs careful consideration.
Let's check for similar complex queries:
#!/bin/bash
# Search for complex queries involving target/resource
rg -B 5 -A 5 "\.innerJoin\(target," --type ts181-182:
Inconsistent naming between function and return type
The function name getTargetMetadataGroupScopes and database schema still use "target" terminology while the return type uses "resource". This inconsistency could lead to runtime errors.
Let's verify the database schema changes:
packages/api/src/router/deployment.ts (2)
222-222: Verify database schema alignment with terminology changes
The change from row.target to row.resource suggests a schema change, but the query still references the target table. This inconsistency could lead to runtime errors.
Let's verify the database schema changes:
✅ Verification successful
Let's check the content of recent migrations and schema definitions to verify if there's a schema change related to this renaming.
Let's check the specific query implementation to ensure it's aligned with the schema changes.
Based on the evidence gathered, I can now provide a final response:
Previous concern about schema inconsistency is resolved
The migration files (specifically in the recent migrations) show a comprehensive rename from target to resource, including:
- Table rename:
ALTER TABLE "target" RENAME TO "resource" - Related tables and constraints updates
- Index updates
- Foreign key updates
The comment about potential runtime errors can be dismissed as the database schema has been properly updated to reflect the terminology change from "target" to "resource".
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the database schema has been updated consistently
# Look for remaining references to 'target' in schema definitions
# Check schema definitions
rg -g "schema.ts" "target.*=.*defineTable"
# Check if there are any pending migrations
fd -g "*.sql" -t f
Length of output: 1779
Script:
#!/bin/bash
# Check recent migrations for any table/column renames
rg -l "target|resource" packages/db/drizzle/00{34,35,36}*.sql | xargs cat
# Check schema definitions for both target and resource
rg -g "schema.ts" -A 5 "resource.*=.*defineTable|target.*=.*defineTable"
# Check the specific query implementation
rg -B 5 -A 5 "distributionById.*=.*" packages/api/src/router/deployment.ts
Length of output: 8457
Script:
#!/bin/bash
# Check the distributionById query implementation
rg -C 10 "distributionById" packages/api/src/router/deployment.ts
# Check for any remaining references to 'target' table in the codebase
rg -g "*.ts" "from.*target.*join|join.*target"
Length of output: 769
407-407:
Complete the target -> resource renaming refactoring
While the authorization check has been updated to use "resource", the procedure name byTargetId and its internal references still use "target". This inconsistency should be addressed.
Consider:
- Renaming the procedure to
byResourceId - Updating variable names (e.g.,
tgtoresource) - Updating comments and documentation
Let's check for other inconsistencies in the codebase:
packages/api/src/router/target.ts (4)
125-125: LGTM: Authorization type updates in targetViews router
The changes consistently update the authorization type from "targetView" to "resourceView" across all operations, aligning with the standardization effort.
Also applies to: 142-142, 154-154
205-205: LGTM: Authorization type updates in targetVariables router
The changes consistently update the authorization type from "target" to "resource" across all operations while maintaining the existing authorization logic and safety checks.
Also applies to: 231-231, 261-261
310-312: LGTM: Authorization type updates in targetRouter
The changes consistently update the authorization type from "target" to "resource" across all operations while preserving the existing authorization logic. The array-based authorization check in the delete operation is also properly updated.
Also applies to: 418-418, 477-477, 523-523, 540-540, 558-558
Line range hint 125-558: Verify completeness of authorization type updates
All authorization type updates from "target"/"targetView" to "resource"/"resourceView" appear to be consistently implemented. Let's verify there are no missed occurrences:
✅ Verification successful
No authorization-related changes needed for remaining occurrences
The remaining instances of type: "target" are used in React component props for diagram visualization, not for authorization purposes. They are correctly left unchanged as they serve a different purpose:
- In
RelationshipsDiagram.tsxandRelationshipsDiagramDependencies.tsx, the type is used to identify node types in a visual graph representation - These occurrences are not related to the authorization system changes being verified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining instances of old authorization types
echo "Checking for any missed 'type: \"target\"' occurrences:"
rg 'type:\s*"target"' --type ts
echo "Checking for any missed 'type: \"targetView\"' occurrences:"
rg 'type:\s*"targetView"' --type ts
Length of output: 574
Script:
#!/bin/bash
# Let's examine the context of these remaining occurrences to determine if they are related to authorization
echo "Checking context of remaining 'type: \"target\"' occurrences:"
rg 'type:\s*"target"' --type ts -B 5 -A 5
# Also check for any other authorization-related code near these files
echo "Checking for authorization patterns in the same files:"
rg 'canUser|Permission|authorizationCheck' apps/webservice/src/app/\[workspaceSlug\]/_components/target-drawer/relationships/RelationshipsDiagram.tsx apps/webservice/src/app/\[workspaceSlug\]/_components/job-drawer/RelationshipsDiagramDependencies.tsx
Length of output: 3389
packages/api/src/router/job.ts (3)
176-176:
Breaking Change: API Response Structure Modified
The renaming of the target property to resource in API responses is a breaking change that could affect API consumers. Ensure that:
- This change is documented in the API changelog
- API version is bumped if following semantic versioning
- Clients are notified about the change
#!/bin/bash
# Description: Check for API documentation and version changes
# Check for changelog updates
fd CHANGELOG -e md | xargs rg -l 'target.*resource'
# Check for version bump in package.json
fd package.json | xargs rg '"version":'Also applies to: 294-294
79-79: Verify database schema changes for target to resource renaming
The parameter and property renaming looks good, but we should verify that corresponding database schema changes have been made.
Also applies to: 113-113
✅ Verification successful
Database schema changes have been properly implemented
The migration files show comprehensive schema changes from "target" to "resource", including:
- Table renames (e.g.,
ALTER TABLE "target" RENAME TO "resource") - Foreign key constraint updates
- Index updates
- Column renames in related tables
- Enum type updates for trigger types and scope types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if database schema has been updated to reflect the target to resource renaming
# Check for any remaining references to 'target' in schema files
fd -e ts -e sql . | xargs rg -l 'createTable\("target"|TABLE\s+target\b'
# Check for new 'resource' table definitions
fd -e ts -e sql . | xargs rg -l 'createTable\("resource"|TABLE\s+resource\b'
Length of output: 2432
Script:
#!/bin/bash
# Let's check for database schema changes more specifically
# Check schema files for target/resource table definitions and migrations
echo "=== Checking schema files ==="
rg -A 5 "createTable\(.*target" packages/db/src/schema/
rg -A 5 "createTable\(.*resource" packages/db/src/schema/
echo "=== Checking recent migration files ==="
# Look at recent migration files for table renames or structure changes
fd -e sql . packages/db/drizzle/ | sort -r | head -n 5 | xargs cat
echo "=== Checking type definitions ==="
# Check type definitions to understand the data structure
rg -A 3 "type.*Target" packages/db/src/schema/
rg -A 3 "type.*Resource" packages/db/src/schema/
Length of output: 14804
620-620: Verify consistent authorization type changes
The authorization type change from "target" to "resource" needs to be consistent across all authorization checks in the codebase.
Also applies to: 633-633
✅ Verification successful
Let me gather more specific information about the authorization type usage.
Authorization type changes are consistent across the codebase
The verification shows that all authorization checks in the backend code consistently use type: "resource" for target-related permissions. The only occurrences of type: "target" are in frontend visualization components (RelationshipsDiagram.tsx and RelationshipsDiagramDependencies.tsx) where it's used for UI rendering purposes, not for authorization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent authorization type changes
# Check for any remaining "target" type in authorization checks
rg -l 'type:\s*"target"' -g '*.ts'
# Check for new "resource" type usage in authorization checks
rg -l 'type:\s*"resource"' -g '*.ts'
# Check authorization utilities for type definitions
ast-grep --pattern 'type\s+ResourceType\s*=.*"resource".*'
Length of output: 516
Script:
#!/bin/bash
# Check the actual usage context of "type" in authorization checks
rg 'type:\s*"(?:target|resource)"' -A 2 -B 2 --color never
# Check for any type definitions or enums related to these types
ast-grep --pattern 'type $_AuthType = {
$$$
}'
Length of output: 8836
| @@ -14,7 +14,7 @@ export async function dispatchJobsForNewTargets( | |||
| newTargetIds: string[], | |||
| envId: string, | |||
| ): Promise<void> { | |||
| const releaseJobTriggers = await createReleaseJobTriggers(db, "new_target") | |||
| const releaseJobTriggers = await createReleaseJobTriggers(db, "new_resource") | |||
There was a problem hiding this comment.
Inconsistent terminology detected
While you've updated the string literal to "new_resource", the surrounding context still uses "target" terminology. This creates an inconsistency between the function's interface and its implementation.
| @@ -48,7 +48,7 @@ export const scopeType = pgEnum("scope_type", [ | |||
| "job", | |||
| "jobAgent", | |||
| "runbook", | |||
| "targetView", | |||
| "resourceView", | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent naming detected: targetView and resourceView coexist
The codebase shows mixed usage of both targetView and resourceView:
- The old
targetViewnaming is still present in multiple files:packages/validators/src/auth/index.tspackages/db/src/schema/target.tspackages/api/src/router/target.ts- Multiple files under
apps/webservice/src/app/[workspaceSlug]/_components/
- The new
resourceViewnaming appears in:packages/db/src/schema/rbac.tspackages/api/src/router/target.tspackages/auth/src/utils/rbac.ts
This inconsistency needs to be addressed to maintain a coherent naming convention throughout the codebase.
🔗 Analysis chain
Verify consistency of resourceView scope type
The addition of resourceView aligns with the target → resource renaming pattern.
Let's verify the consistency of this change:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of resourceView
# Check for any remaining references to targetView
echo "Checking for remaining targetView references:"
rg "targetView" -t ts
# Check for new resourceView usage
echo "Checking for new resourceView usage:"
rg "resourceView" -t ts
Length of output: 4092
| "resource", | ||
| "resourceProvider", | ||
| "resourceMetadataGroup", |
There was a problem hiding this comment.
💡 Codebase verification
Migration required for scope type changes from 'target' to 'resource'
The codebase search reveals extensive usage of the old 'target' terminology across multiple packages. This includes database schemas, API routes, validators, and frontend components. No migration files were found, and the changes appear to be breaking changes that require careful migration:
packages/validators/src/auth/index.ts: Contains permission enums using old target terminologypackages/db/src/schema/: Multiple files still use target-prefixed table names and typespackages/api/src/router/: API routes and handlers using old target terminologyapps/webservice/src/app/: Frontend components using target-based naming
🔗 Analysis chain
Verify migration strategy for scope type changes
The renaming from target-related to resource-related scope types looks good and aligns with the PR objectives. However, as this is a database schema change, we need to ensure existing records are handled properly.
Let's verify the impact and migration strategy:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing migrations and usage of old scope types
# Check for migration files
echo "Checking for related migration files:"
fd -e sql -e ts "target.*resource" packages/db/migrations
# Check for references to old scope types in code
echo "Checking for remaining references to old scope types:"
rg "targetProvider|targetMetadataGroup|targetView" -t ts
Length of output: 18902
| @@ -60,7 +60,7 @@ export const PATCH = request() | |||
| .where(eq(targetProvider.id, params.providerId)) | |||
| .then(takeFirstOrNull); | |||
|
|
|||
| const provider = query?.target_provider; | |||
| const provider = query?.resource_provider; | |||
There was a problem hiding this comment.
Inconsistent terminology between schema and variable names
The variable has been renamed to resource_provider but it's being destructured from query?.resource_provider while the schema and table name is still targetProvider. This creates confusion and makes the code harder to maintain.
Either:
- Revert the variable name change until the schema is updated:
-const provider = query?.resource_provider;
+const provider = query?.target_provider;- Or complete the schema rename first and update all related references.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -44,7 +44,7 @@ | |||
| authz(({ can, extra }) => | |||
| can | |||
| .perform(Permission.TargetUpdate) | |||
| .on({ type: "targetProvider", id: extra.params.providerId }), | |||
| .on({ type: "resourceProvider", id: extra.params.providerId }), | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent authorization type with Permission enum
The authorization type has been changed to "resourceProvider" but this is inconsistent with the Permission enum which still uses "target" prefixes. The code is using Permission.TargetUpdate but checking against type: "resourceProvider", creating a mismatch in the authorization system.
packages/validators/src/auth/index.ts: Permission enum still usesTargetUpdate,TargetProviderUpdate, etc.apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts: UsesPermission.TargetUpdatewithtype: "resourceProvider"
This inconsistency could lead to authorization failures and needs to be addressed by either:
- Reverting the authorization type back to "targetProvider" to match the Permission enum, or
- Updating the entire Permission enum to use "resource" terminology consistently
🔗 Analysis chain
Verify permission enum consistency
The authorization type has been changed to "resourceProvider", but we need to verify if this matches the Permission enum which still uses TargetUpdate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Permission enum has been updated consistently
# Look for Permission enum definition and its usage
ast-grep --pattern 'enum Permission {
$$$
TargetUpdate
$$$
}'
# Check for any remaining "target" references in auth-related files
rg -g "**/{auth,permission}*" -i "target.*update"
Length of output: 121
Script:
#!/bin/bash
# Let's try a broader search for Permission enum and authorization types
# Search for Permission enum
ast-grep --pattern 'enum Permission {
$$$
}'
# Search for authorization type definitions and usage
rg -g "!{*.lock,*.md}" "type: ['\"]resourceProvider['\"]|type: ['\"]targetProvider['\"]" -A 2 -B 2
# Search for permission-related imports and usage
rg -g "!{*.lock,*.md}" "Permission.*Target|Target.*Permission" -A 2 -B 2
Length of output: 22718
| ALTER TYPE "release_job_trigger_type" ADD VALUE 'new_resource';--> statement-breakpoint | ||
| ALTER TYPE "release_job_trigger_type" ADD VALUE 'resource_changed';--> statement-breakpoint | ||
| ALTER TYPE "scope_type" ADD VALUE 'resource';--> statement-breakpoint | ||
| ALTER TYPE "scope_type" ADD VALUE 'resourceProvider';--> statement-breakpoint | ||
| ALTER TYPE "scope_type" ADD VALUE 'resourceMetadataGroup';--> statement-breakpoint | ||
| ALTER TYPE "scope_type" ADD VALUE 'resourceView';--> statement-breakpoint |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent enum value naming convention.
The enum values use inconsistent naming conventions:
release_job_trigger_typeuses snake_case ('new_resource', 'resource_changed')scope_typeuses camelCase ('resourceProvider', 'resourceMetadataGroup')
Consider standardizing to one convention across all enum values.
ALTER TYPE "release_job_trigger_type" ADD VALUE 'new_resource';
ALTER TYPE "release_job_trigger_type" ADD VALUE 'resource_changed';
ALTER TYPE "scope_type" ADD VALUE 'resource';
-ALTER TYPE "scope_type" ADD VALUE 'resourceProvider';
-ALTER TYPE "scope_type" ADD VALUE 'resourceMetadataGroup';
-ALTER TYPE "scope_type" ADD VALUE 'resourceView';
+ALTER TYPE "scope_type" ADD VALUE 'resource_provider';
+ALTER TYPE "scope_type" ADD VALUE 'resource_metadata_group';
+ALTER TYPE "scope_type" ADD VALUE 'resource_view';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ALTER TYPE "release_job_trigger_type" ADD VALUE 'new_resource';--> statement-breakpoint | |
| ALTER TYPE "release_job_trigger_type" ADD VALUE 'resource_changed';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resource';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resourceProvider';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resourceMetadataGroup';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resourceView';--> statement-breakpoint | |
| ALTER TYPE "release_job_trigger_type" ADD VALUE 'new_resource';--> statement-breakpoint | |
| ALTER TYPE "release_job_trigger_type" ADD VALUE 'resource_changed';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resource';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resource_provider';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resource_metadata_group';--> statement-breakpoint | |
| ALTER TYPE "scope_type" ADD VALUE 'resource_view';--> statement-breakpoint |
| END $$; | ||
| --> statement-breakpoint | ||
| DO $$ BEGIN | ||
| ALTER TABLE "resource_metadata" ADD CONSTRAINT "resource_metadata_target_id_resource_id_fk" FOREIGN KEY ("target_id") REFERENCES "public"."resource"("id") ON DELETE cascade ON UPDATE no action; |
There was a problem hiding this comment.
Fix inconsistent column name in foreign key constraint.
The foreign key constraint for resource_metadata still references "target_id" instead of "resource_id":
-ALTER TABLE "resource_metadata" ADD CONSTRAINT "resource_metadata_target_id_resource_id_fk" FOREIGN KEY ("target_id") REFERENCES "public"."resource"("id") ON DELETE cascade ON UPDATE no action;
+ALTER TABLE "resource_metadata" ADD CONSTRAINT "resource_metadata_resource_id_resource_id_fk" FOREIGN KEY ("resource_id") REFERENCES "public"."resource"("id") ON DELETE cascade ON UPDATE no action;Committable suggestion skipped: line range outside the PR's diff.
| @@ -119,7 +119,7 @@ export const releaseRouter = createTRPCRouter({ | |||
| .map((j) => ({ | |||
| ...j.release_job_trigger, | |||
| job: j.job, | |||
| target: j.target, | |||
| target: j.resource, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency in the target -> resource refactoring
While the value has been updated to use j.resource, the property name is still using the old "target" terminology. To maintain consistency with the broader refactoring effort, the property name should also be updated.
Apply this change:
- target: j.resource,
+ resource: j.resource,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target: j.resource, | |
| resource: j.resource, |
| resource: getTargetScopes, | ||
| resourceView: getTargetViewScopes, | ||
| resourceProvider: getTargetProviderScopes, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent handler mappings in scopeHandlers
The scopeHandlers record maps new resource-based types to old target-based functions:
resource: getTargetScopesresourceView: getTargetViewScopesresourceProvider: getTargetProviderScopesresourceMetadataGroup: getTargetMetadataGroupScopes
This creates a confusing mapping between old and new terminology.
Consider updating the function names to match the new resource terminology:
- resource: getTargetScopes,
- resourceView: getTargetViewScopes,
- resourceProvider: getTargetProviderScopes,
+ resource: getResourceScopes,
+ resourceView: getResourceViewScopes,
+ resourceProvider: getResourceProviderScopes,
deployment: getDeploymentScopes,
deploymentVariable: getDeploymentVariableScopes,
runbook: getRunbookScopes,
system: getSystemScopes,
workspace: getWorkspaceScopes,
environment: getEnvironmentScopes,
environmentPolicy: getEnvironmentPolicyScopes,
release: getReleaseScopes,
releaseChannel: getReleaseChannelScopes,
- resourceMetadataGroup: getTargetMetadataGroupScopes,
+ resourceMetadataGroup: getResourceMetadataGroupScopes,Also applies to: 384-384
| @@ -194,7 +194,7 @@ | |||
| .then(takeFirst); | |||
|
|
|||
| return [ | |||
| { type: "target" as const, id: result.target.id }, | |||
| { type: "resource" as const, id: result.resource.id }, | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency confirmed between database schema and code
The database schema in packages/db/src/schema/target.ts defines the table name as "resource" while using the variable name target. The getTargetScopes function is mapped to handle "resource" type scopes in the scopeHandlers record. This indicates that "resource" is the correct terminology, and "target" is the legacy term that needs to be updated for consistency.
packages/db/src/schema/target.ts: File name and variable should be renamed to match "resource"packages/auth/src/utils/rbac.ts: FunctiongetTargetScopesshould be renamed togetResourceScopes
🔗 Analysis chain
Inconsistent naming in getTargetScopes
Similar to the previous issue, there's a mismatch between the function name (getTargetScopes), database schema (target), and the return type (resource).
Let's verify if this pattern exists in other files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for target/resource naming inconsistencies
ast-grep --pattern 'function getTarget$_($$$) {
$$$
return { type: "resource"$_ }
}'
Length of output: 6023
Script:
#!/bin/bash
# Let's check the full context of getTargetScopes function and its usage
rg -B 5 -A 15 "getTargetScopes" --type ts
# Also check for any resource/target type definitions
ast-grep --pattern 'type $_Resource = {
$$$
}'
# And check database schema for target table
rg -B 2 -A 5 "target = pgTable" --type ts
Length of output: 3170
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/job-dispatch/src/policies/release-dependency.ts (1)
Line range hint
42-69: Consider optimizing the recursive CTEWhile the renaming changes look good, the recursive CTE could benefit from some optimizations:
- The CASE expressions are repeated multiple times
- The visited array grows with each recursion
Consider this optimization:
WITH RECURSIVE reachable_relationships(id, visited, tr_id, source_id, target_id, type) AS ( -- Base case: start with the given ID and no relationship SELECT ${trigger.targetId}::uuid AS id, ARRAY[${trigger.targetId}::uuid] AS visited, NULL::uuid AS tr_id, NULL::uuid AS source_id, NULL::uuid AS target_id, NULL::resource_relationship_type AS type UNION ALL -- Recursive case: find all relationships connected to the current set of IDs SELECT - CASE - WHEN tr.source_id = rr.id THEN tr.target_id - ELSE tr.source_id - END AS id, - rr.visited || CASE - WHEN tr.source_id = rr.id THEN tr.target_id - ELSE tr.source_id - END, + COALESCE(NULLIF(tr.target_id, rr.id), tr.source_id) AS id, + rr.visited || COALESCE(NULLIF(tr.target_id, rr.id), tr.source_id), tr.id AS tr_id, tr.source_id, tr.target_id, tr.type FROM reachable_relationships rr JOIN resource_relationship tr ON tr.source_id = rr.id OR tr.target_id = rr.id WHERE - NOT CASE - WHEN tr.source_id = rr.id THEN tr.target_id - ELSE tr.source_id - END = ANY(rr.visited) + NOT COALESCE(NULLIF(tr.target_id, rr.id), tr.source_id) = ANY(rr.visited) AND tr.target_id != ${trigger.targetId} )This optimization:
- Reduces repeated CASE expressions using COALESCE and NULLIF
- Maintains the same logic but with better readability
- May improve performance by reducing expression evaluations
packages/api/src/router/target.ts (3)
125-125: Consider updating router and schema names for consistencyWhile the authorization check types have been updated from "targetView" to "resourceView", the router name (
targetViews) and schema references still use the old terminology. Consider updating these for consistency:
- Rename
targetViewsrouter toresourceViews- Update schema references from
targetViewtoresourceViewAlso applies to: 142-142, 154-154
205-205: Update variable naming for consistencyWhile the authorization check types have been updated to use "resource", the variable references still use
targetId. Consider updating these references for consistency:
- Rename
targetIdtoresourceIdin variable references- Update schema references from
targetVariabletoresourceVariableAlso applies to: 231-231, 261-261
Line range hint
1-574: Consider a more systematic refactoring approachThe current changes focus mainly on authorization check types, but a more systematic approach to the target->resource renaming would be beneficial:
Create a comprehensive migration plan that includes:
- Schema updates (tables, foreign keys)
- Router names
- Variable names
- Event handlers
- Job-related functionality
Consider using a codemod or automated refactoring tool to ensure consistent renaming across the codebase.
Add tests to verify that the refactoring doesn't break existing functionality.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/api/src/router/job.ts(7 hunks)packages/api/src/router/target.ts(14 hunks)packages/job-dispatch/src/policies/release-dependency.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/router/job.ts
🔇 Additional comments (4)
packages/job-dispatch/src/policies/release-dependency.ts (1)
60-60: Verify database table renaming consistency
The table reference has been updated from target_relationship to resource_relationship in the JOIN clause.
Let's verify the table renaming is complete and consistent:
#!/bin/bash
# Description: Verify the database table renaming consistency
# Expected: All references should use resource_relationship, not target_relationship
# Check for any remaining references to the old table name
rg "target_relationship([^_]|$)"
# Check for migration files that handle this rename
fd -e sql -e ts . | rg -l "RENAME TABLE.*target_relationship.*TO.*resource_relationship"packages/api/src/router/target.ts (3)
279-281: Verify schema changes and consider type improvements
- Verify that the metadata table has been renamed from
target_metadatatoresource_metadatain the schema. - Consider using a more descriptive type name than
_StringStringRecordfor the metadata aggregation result.
#!/bin/bash
# Description: Check for schema changes and type usage
# Test: Look for metadata table references in schema
rg -A 5 "resource_metadata|target_metadata" --type ts
# Test: Check for similar type definitions that might be reused
rg "type.*Record<string,\s*string>" --type ts310-312: Consider comprehensive renaming and verify impact
While authorization checks have been updated to use "resource", consider a more comprehensive update:
- Rename the router from
targetRoutertoresourceRouter - Update all schema references from
targettoresource - Update related constants and types
Additionally, verify that these changes don't impact:
- Job dispatch functionality
- Event handling (
getEventsForTargetDeleted) - Release job triggers
#!/bin/bash
# Description: Check for related functionality that might be affected
# Test: Look for event handlers and job-related functions
rg "getEventsForTarget|ReleaseJobTrigger" --type ts
# Test: Check for other routers that might need similar updates
rg "createTRPCRouter.*target" --type tsAlso applies to: 418-418, 477-477, 523-523, 540-540, 558-558
67-67: Verify schema migration for resource_relationship table
The table name change from target_relationship to resource_relationship suggests a schema change. Ensure that the corresponding migration has been applied and that all related foreign key constraints have been updated.
✅ Verification successful
Based on the search results, I can see that there is a proper migration path for the table rename from target_relationship to resource_relationship. Specifically, in packages/db/drizzle/0036_furry_blue_marvel.sql, there is a clear migration:
ALTER TABLE "target_relationship" RENAME TO "resource_relationship";Additionally, the schema definitions in packages/db/src/schema/target.ts show the updated table name:
export const targetRelationship = pgTable(
"resource_relationship",
{
// ...
}
);Schema migration is properly implemented for resource_relationship table
The table rename from target_relationship to resource_relationship is properly handled through migration 0036. All related foreign key constraints and indexes are also updated in the same migration. The change is safe to proceed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for schema migration files and verify table existence
# Test: Look for migration files containing table rename
fd --type f . | rg -i "migration.*resource_relationship"
# Test: Check if both old and new tables exist in schema definitions
rg -A 5 "resource_relationship|target_relationship" --type ts
Length of output: 14751
Script:
#!/bin/bash
# Let's search for schema definitions and migrations related to resource/target relationships
# Look for schema definitions
rg "relationship.*schema" --type ts -A 5
# Look for table definitions in schema files
fd schema -t f | xargs rg "createTable.*relationship" -A 10
# Look for migrations
fd migrations -t d | xargs rg "relationship" -A 5
Length of output: 140653
| @@ -40,7 +40,7 @@ export const isPassingReleaseDependencyPolicy = async ( | |||
| NULL::uuid AS tr_id, | |||
| NULL::uuid AS source_id, | |||
| NULL::uuid AS target_id, | |||
| NULL::target_relationship_type AS type | |||
| NULL::resource_relationship_type AS type | |||
There was a problem hiding this comment.
💡 Codebase verification
Schema type renaming needs attention
The verification reveals inconsistencies in the schema type renaming:
- The table has been renamed from
target_relationshiptoresource_relationshipin migration0036_furry_blue_marvel.sql - However, there's no corresponding migration to rename the enum type from
target_relationship_typetoresource_relationship_type - The old type
target_relationship_typeis still being referenced in multiple files including:packages/api/src/router/target.tspackages/api/src/router/job.ts- Various migration files and snapshots
🔗 Analysis chain
Verify schema type renaming consistency
The type cast has been updated from target_relationship_type to resource_relationship_type as part of the broader refactoring effort.
Let's verify the type exists and is used consistently:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the schema type renaming consistency
# Expected: All references should use resource_relationship_type, not target_relationship_type
# Check for any remaining references to the old type
rg "target_relationship_type"
# Check for the new type definition and usage
rg "resource_relationship_type"
Length of output: 10641
Script:
#!/bin/bash
# Let's verify the schema changes more comprehensively
# Check for migration that renames the type
rg -A 2 -B 2 "CREATE TYPE.*resource_relationship_type"
rg -A 2 -B 2 "ALTER TYPE.*target_relationship_type.*RENAME TO.*resource_relationship_type"
# Check for the actual type definition
rg "resource_relationship_type" --type sql
# Check for the migration that renames the table
rg -A 2 -B 2 "ALTER TABLE.*target_relationship.*RENAME TO.*resource_relationship"
# Check for any remaining references to target_relationship_type
rg "target_relationship_type"
Length of output: 8593
…-target-to-resource
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/deployment.ts (1)
Line range hint
1-524: Consider documenting the target->resource refactoring strategyThe current changes appear to be part of a larger refactoring effort to rename "target" to "resource". However, the changes are inconsistent across the file, with some places updated while others retain the old terminology. This could lead to confusion and maintenance challenges.
Consider:
- Documenting the refactoring strategy and timeline
- Creating a tracking issue for all required changes
- Using a consistent approach (either update everything at once or clearly mark what's pending)
- Adding comments to explain any intentionally mixed terminology during the transition
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/api/src/router/deployment.ts(2 hunks)
🔇 Additional comments (2)
packages/api/src/router/deployment.ts (2)
413-413: 🛠️ Refactor suggestion
Consider complete refactoring of target terminology
While the authorization type has been updated to "resource", the procedure name byTargetId and related variables still use "target" terminology. This mixed usage could lead to maintenance challenges.
Consider updating the procedure name and related variables for consistency:
- byTargetId: protectedProcedure
+ byResourceId: protectedProcedureLet's analyze the impact of this change:
#!/bin/bash
# Check for procedure usage across the codebase
rg "byTargetId" -g "*.ts" -A 3
# Check for other potential target->resource renames needed
ast-grep --pattern 'byTargetId($$$)' -l ts222-222: Verify consistency between property name and table schema
While the property is renamed from target to resource, the underlying table and join conditions still use target. This might indicate an incomplete refactoring or cause confusion for developers.
Let's check if this is part of a broader schema refactoring:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/api/src/router/target.ts (1)
310-312: Consider renaming the router for consistencyWhile the authorization check updates from "target" to "resource" are correct, consider renaming
targetRoutertoresourceRouterfor consistency with the new terminology.-export const targetRouter = createTRPCRouter({ +export const resourceRouter = createTRPCRouter({Also applies to: 418-418, 477-477, 523-523, 540-540, 558-558
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/api/src/router/job.ts(8 hunks)packages/api/src/router/target.ts(15 hunks)packages/db/drizzle/0036_furry_blue_marvel.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/router/job.ts
🔇 Additional comments (10)
packages/db/drizzle/0036_furry_blue_marvel.sql (8)
1-11: Previous comment about inconsistent enum naming still applies.
13-31: Table renames look good!
The renaming of tables from "target" to "resource" is consistent across all related tables.
33-43: Column renames look good!
The renaming of columns from "target_" to "resource_" is consistent with the table renaming pattern.
45-80: Constraint drops are complete and necessary!
All old foreign key constraints are properly dropped before adding new ones with updated names.
81-91: Index drops are complete and necessary!
All old indexes are properly dropped before creating new ones with updated names.
93-163: Previous comment about inconsistent foreign key constraint still applies.
165-179: Previous comment about inconsistent index names still applies.
181-182: Type rename looks good!
The renaming of the enum type from "target_relationship_type" to "resource_relationship_type" is consistent with other changes.
packages/api/src/router/target.ts (2)
205-205: Authorization check updates look good
The changes from "target" to "resource" in authorization checks are consistent with the renaming pattern, and the underlying logic remains intact.
Also applies to: 231-231, 261-261
125-125: Verify Permission enum updates
The authorization type changes from "targetView" to "resourceView" should have corresponding updates in the Permission enum.
Also applies to: 142-142, 154-154
| jsonb_object_agg(resource_metadata.key, resource_metadata.value) | ||
| FILTER (WHERE resource_metadata.key IS NOT NULL) | ||
| `.as("resource_metadata"), |
There was a problem hiding this comment.
💡 Codebase verification
Incomplete schema changes for metadata tables
There are still references to target_metadata which suggests not all schema updates were applied.
🔗 Analysis chain
Verify metadata table schema changes
The changes from target_metadata to resource_metadata in the aggregation query need corresponding schema updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify metadata table schema changes
# Test: Check for the existence of both old and new tables
# Check for resource_metadata table
rg -g '*.sql' -A 5 'CREATE TABLE.*resource_metadata'
# Check for any remaining references to target_metadata
rg 'target_metadata'
Length of output: 6368
Script:
#!/bin/bash
# Description: Verify metadata table schema changes
# Test: Check for the existence of both old and new tables
# Check for resource_metadata table
rg -g '*.sql' -A 5 'CREATE TABLE.*resource_metadata'
# Check for any remaining references to target_metadata
rg 'target_metadata'
Length of output: 65520
| @@ -47,7 +47,7 @@ | |||
| NULL::uuid AS tr_id, | |||
| NULL::uuid AS source_id, | |||
| NULL::uuid AS target_id, | |||
| NULL::target_relationship_type AS type | |||
| NULL::resource_relationship_type AS type | |||
There was a problem hiding this comment.
💡 Codebase verification
Incomplete schema migrations for relationship tables
References to target_relationship are still present, which may lead to inconsistencies or runtime errors.
🔗 Analysis chain
Verify schema changes for relationship tables
The changes from target_relationship to resource_relationship need corresponding schema migrations to avoid runtime errors.
Also applies to: 67-67
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if schema changes have been applied
# Test: Check for the existence of both old and new tables/types
# Check for resource_relationship table
rg -g '*.sql' -A 5 'CREATE TABLE.*resource_relationship'
# Check for resource_relationship_type
rg -g '*.sql' -A 5 'CREATE TYPE.*resource_relationship_type'
# Check for any remaining references to target_relationship
rg 'target_relationship'
Length of output: 6368
Script:
#!/bin/bash
# Description: Verify if schema changes have been applied
# Test: Check for the existence of both old and new tables/types
# Check for resource_relationship table
rg -g '*.sql' -A 5 'CREATE TABLE.*resource_relationship'
# Check for resource_relationship_type
rg -g '*.sql' -A 5 'CREATE TYPE.*resource_relationship_type'
# Check for any remaining references to target_relationship
rg 'target_relationship'
Length of output: 35534
Summary by CodeRabbit
New Features
toTargetprocedure.Bug Fixes
Documentation
Refactor
Chores