Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: fix codeql config #1915

Merged
merged 4 commits into from
Dec 16, 2024
Merged

chore: fix codeql config #1915

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
afbdbf9
chore: fix codeql config
ymc9 Dec 16, 2024
8764735
fix config
ymc9 Dec 16, 2024
469b26e
clean up workflows
ymc9 Dec 16, 2024
bab1142
delete wrong test
ymc9 Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/codeql.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ jobs:
- name: Initialize CodeQL
uses: github/codeql-action/init@v3
with:
config-file: ./.github/workflows/config/codeql-config.yml
languages: ${{ matrix.language }}
build-mode: ${{ matrix.build-mode }}
# If you wish to specify custom queries, you can do so here or in a config file.
Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/config/codeql-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
paths:
- 'packages'
paths-ignore:
- '**/*.test.js'
- '**/*.test.ts'
- '**/*.test.tsx'
- '**/__tests__/**'
- 'packages/ide/**'
58 changes: 0 additions & 58 deletions .github/workflows/management-changelog.yml
View file
Open in desktop

This file was deleted.

66 changes: 0 additions & 66 deletions .github/workflows/security-defender-for-devops.yml
View file
Open in desktop

This file was deleted.

71 changes: 71 additions & 0 deletions tests/regression/tests/issue-1898.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { loadSchema } from '@zenstackhq/testtools';

describe('issue 1898', () => {
it('regression', async () => {
const { enhance, prisma } = await loadSchema(
`
model Role {
id Int @id @default(autoincrement())
name String @unique
permissions Permission[]
foos Foo[]
deletable Boolean @default(true)

@@allow('all', true)
}

model Permission {
id Int @id @default(autoincrement())
name String
roleId Int
role Role @relation(fields: [roleId], references: [id], onDelete: Cascade)

@@allow('all', true)
}

model Foo {
id Int @id @default(autoincrement())
name String
roleId Int
role Role @relation(fields: [roleId], references: [id])
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)
}

`,
{ logPrismaQuery: true, prismaClientOptions: { log: ['query', 'info'] } }
);

const db = enhance();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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' },
],
},
},
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid hardcoded IDs and add assertions

Two concerns with the role creation:

  1. Using hardcoded IDs (id: 1, id: 2) might cause conflicts in parallel test runs
  2. 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.

Suggested change
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']);


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 },
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify complex update operation and fix logical issues

Several issues with the update operation:

  1. Using hardcoded ID (id: 3) for new permission
  2. Attempting to update a permission in the same transaction as its creation
  3. 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.

Suggested change
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');


console.log(updatedRole);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

});
});
Loading