chore: fix codeql config #1915
chore: fix codeql config #1915
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new regression test file Changes
Sequence DiagramsequenceDiagram
participant Test as Regression Test
participant DB as Database
Test->>DB: Create Role
Test->>DB: Add Permissions to Role
Test->>DB: Update Role Name
Test->>DB: Add Foo Entry
Test->>DB: Modify Permissions
Test->>DB: Delete Existing Permissions
Test->>DB: Create New Permission
Test->>DB: Update Permission Name
Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/regression/tests/issue-1898.test.ts (2)
30-30: Consider adding onDelete behavior for Foo->Role relationThe
Permissionmodel hasonDelete: CascadebutFoodoesn't. This might lead to referential integrity issues when deleting roles.- role Role @relation(fields: [roleId], references: [id]) + role Role @relation(fields: [roleId], references: [id], onDelete: Cascade)
33-33: Consider disabling verbose logging in CIThe test enables verbose logging which might affect CI performance and log readability. Consider making it conditional:
-{ logPrismaQuery: true, prismaClientOptions: { log: ['query', 'info'] } } +{ + logPrismaQuery: process.env.DEBUG === 'true', + prismaClientOptions: { + log: process.env.DEBUG === 'true' ? ['query', 'info'] : [] + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/codeql.ymlis excluded by!**/*.yml.github/workflows/config/codeql-config.ymlis excluded by!**/*.yml.github/workflows/management-changelog.ymlis excluded by!**/*.yml.github/workflows/security-defender-for-devops.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
tests/regression/tests/issue-1898.test.ts(1 hunks)
🔇 Additional comments (2)
tests/regression/tests/issue-1898.test.ts (2)
14-14: Review permissive access rules
The @@allow('all', true) rules grant unrestricted access to these models. Consider using more specific access rules unless this is intentionally permissive for testing purposes.
Also applies to: 23-23
1-71: Verify PR title and changes alignment
The PR title mentions "fix codeql config" but this appears to be a regression test for issue #1898. Please verify if this change is related to the CodeQL configuration fix or if the PR title needs updating.
✅ Verification successful
PR changes align with its title "fix codeql config"
The changes in this PR include modifications to CodeQL configuration files (.github/workflows/codeql.yml and .github/workflows/config/codeql-config.yml) along with a regression test (issue-1898.test.ts). While the regression test is part of the changes, the PR's primary focus on fixing CodeQL config is accurately reflected in the title, as evidenced by the changes to multiple CodeQL-related configuration files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CodeQL config changes
rg -l "codeql" .github/
Length of output: 130
Script:
#!/bin/bash
# Check git diff for changes in CodeQL config files and the test file
git diff --name-only origin/main | grep -E "\.github/.*codeql|issue-1898\.test\.ts"
# Check PR title and description
gh pr view --json title,body
Length of output: 282
| { logPrismaQuery: true, prismaClientOptions: { log: ['query', 'info'] } } | ||
| ); | ||
|
|
||
| const db = enhance(); |
There was a problem hiding this comment.
Unused database instance
The enhanced database instance db is created but never used in the test. Either use it or remove it.
- const db = enhance();| const role = await prisma.role.create({ | ||
| data: { | ||
| name: 'regular', | ||
| permissions: { | ||
| create: [ | ||
| { id: 1, name: 'read' }, | ||
| { id: 2, name: 'write' }, | ||
| ], | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoded IDs and add assertions
Two concerns with the role creation:
- Using hardcoded IDs (
id: 1, id: 2) might cause conflicts in parallel test runs - Missing assertions to verify the created role and permissions
const role = await prisma.role.create({
data: {
name: 'regular',
permissions: {
create: [
- { id: 1, name: 'read' },
- { id: 2, name: 'write' },
+ { name: 'read' },
+ { name: 'write' },
],
},
},
+ include: { permissions: true },
});
+expect(role.name).toBe('regular');
+expect(role.permissions).toHaveLength(2);
+expect(role.permissions.map(p => p.name)).toEqual(['read', 'write']);📝 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.
| const role = await prisma.role.create({ | |
| data: { | |
| name: 'regular', | |
| permissions: { | |
| create: [ | |
| { id: 1, name: 'read' }, | |
| { id: 2, name: 'write' }, | |
| ], | |
| }, | |
| }, | |
| }); | |
| const role = await prisma.role.create({ | |
| data: { | |
| name: 'regular', | |
| permissions: { | |
| create: [ | |
| { name: 'read' }, | |
| { name: 'write' }, | |
| ], | |
| }, | |
| }, | |
| include: { permissions: true }, | |
| }); | |
| expect(role.name).toBe('regular'); | |
| expect(role.permissions).toHaveLength(2); | |
| expect(role.permissions.map(p => p.name)).toEqual(['read', 'write']); |
| model Foo { | ||
| id Int @id @default(autoincrement()) | ||
| name String | ||
| roleId Int | ||
| role Role @relation(fields: [roleId], references: [id]) | ||
| } |
There was a problem hiding this comment.
Add missing access rules for Foo model
The Foo model is missing access rules (@@allow) that are present in other models. This inconsistency might affect test behavior.
model Foo {
id Int @id @default(autoincrement())
name String
roleId Int
role Role @relation(fields: [roleId], references: [id])
+ @@allow('all', true)
}📝 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.
| model Foo { | |
| id Int @id @default(autoincrement()) | |
| name String | |
| roleId Int | |
| role Role @relation(fields: [roleId], references: [id]) | |
| } | |
| model Foo { | |
| id Int @id @default(autoincrement()) | |
| name String | |
| roleId Int | |
| role Role @relation(fields: [roleId], references: [id]) | |
| @@allow('all', true) | |
| } |
| const updatedRole = await prisma.role.update({ | ||
| where: { id: role.id }, | ||
| data: { | ||
| name: 'admin', | ||
| foos: { | ||
| create: { name: 'foo1' }, | ||
| }, | ||
| permissions: { | ||
| deleteMany: { | ||
| roleId: role.id, | ||
| }, | ||
| create: { id: 3, name: 'delete' }, | ||
| update: { where: { id: 3 }, data: { name: 'delete1' } }, | ||
| }, | ||
| deletable: false, | ||
| }, | ||
| include: { permissions: true }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify complex update operation and fix logical issues
Several issues with the update operation:
- Using hardcoded ID (
id: 3) for new permission - Attempting to update a permission in the same transaction as its creation
- Complex operation mixing multiple concerns
Consider splitting this into multiple test cases or at least multiple operations:
const updatedRole = await prisma.role.update({
where: { id: role.id },
data: {
name: 'admin',
foos: {
create: { name: 'foo1' },
},
permissions: {
deleteMany: {
roleId: role.id,
},
- create: { id: 3, name: 'delete' },
- update: { where: { id: 3 }, data: { name: 'delete1' } },
+ create: { name: 'delete' },
},
deletable: false,
},
include: { permissions: true },
});
+expect(updatedRole.name).toBe('admin');
+expect(updatedRole.permissions).toHaveLength(1);
+expect(updatedRole.permissions[0].name).toBe('delete');📝 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.
| const updatedRole = await prisma.role.update({ | |
| where: { id: role.id }, | |
| data: { | |
| name: 'admin', | |
| foos: { | |
| create: { name: 'foo1' }, | |
| }, | |
| permissions: { | |
| deleteMany: { | |
| roleId: role.id, | |
| }, | |
| create: { id: 3, name: 'delete' }, | |
| update: { where: { id: 3 }, data: { name: 'delete1' } }, | |
| }, | |
| deletable: false, | |
| }, | |
| include: { permissions: true }, | |
| }); | |
| const updatedRole = await prisma.role.update({ | |
| where: { id: role.id }, | |
| data: { | |
| name: 'admin', | |
| foos: { | |
| create: { name: 'foo1' }, | |
| }, | |
| permissions: { | |
| deleteMany: { | |
| roleId: role.id, | |
| }, | |
| create: { name: 'delete' }, | |
| }, | |
| deletable: false, | |
| }, | |
| include: { permissions: true }, | |
| }); | |
| expect(updatedRole.name).toBe('admin'); | |
| expect(updatedRole.permissions).toHaveLength(1); | |
| expect(updatedRole.permissions[0].name).toBe('delete'); |
| include: { permissions: true }, | ||
| }); | ||
|
|
||
| console.log(updatedRole); |
There was a problem hiding this comment.
Replace console.log with proper assertions
Using console.log is not a proper way to verify test results. Add explicit assertions to validate the expected state.
-console.log(updatedRole);
+expect(updatedRole).toMatchObject({
+ name: 'admin',
+ deletable: false,
+ permissions: expect.arrayContaining([
+ expect.objectContaining({ name: 'delete' })
+ ])
+});Committable suggestion skipped: line range outside the PR's diff.
No description provided.