Conversation
chore(PM-2539): added timeout for prisma service
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| # Prisma configuration | ||
|
|
||
| IDENTITY_SERVICE_PRISMA_TIMEOUT=10000 No newline at end of file |
There was a problem hiding this comment.
[💡 style]
Consider adding a newline at the end of the file to adhere to POSIX standards, which can help prevent issues with certain tools and version control systems.
| jobs: | ||
| trivy-scan: | ||
| name: Use Trivy | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
[maintainability]
Consider using a stable version of ubuntu-latest instead of ubuntu-24.04 to ensure compatibility and reduce maintenance overhead when new Ubuntu versions are released.
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Run Trivy scanner in repo mode | ||
| uses: aquasecurity/trivy-action@0.33.1 |
There was a problem hiding this comment.
[maintainability]
Ensure that the version 0.33.1 of aquasecurity/trivy-action is the intended version and not a placeholder. Locking to a specific version can help avoid unexpected changes, but it also requires regular updates to benefit from new features and security patches.
| github-pat: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Upload Trivy scan results to GitHub Security tab | ||
| uses: github/codeql-action/upload-sarif@v3 |
There was a problem hiding this comment.
[maintainability]
Verify that github/codeql-action/upload-sarif@v3 is the correct and intended version. Using a specific major version can provide stability, but ensure it aligns with your update and security policies.
| it('should throw ConflictException if assignment already exists', async () => { | ||
| it('should ignore duplicate assignment if already exists', async () => { | ||
| mockPrisma.role.count.mockResolvedValue(1); | ||
| mockPrisma.roleAssignment.create.mockRejectedValue({ code: 'P2002' }); |
There was a problem hiding this comment.
[❗❗ correctness]
The change from rejecting with a ConflictException to resolving with undefined when a duplicate assignment is detected alters the method's contract. Ensure that all consumers of assignRoleToSubject are aware of this change in behavior, as they may be expecting an exception to handle duplicates.
| service.assignRoleToSubject(roleId, subjectId, operatorId), | ||
| ).rejects.toThrow(ConflictException); | ||
| ).resolves.toBeUndefined(); | ||
| expect(mockLogger.warn).toHaveBeenCalledWith( |
There was a problem hiding this comment.
[💡 maintainability]
The use of mockLogger.warn to log duplicate assignments is a good practice for observability. However, ensure that the logging level and message format align with the overall logging strategy of the application to maintain consistency.
| if (error.code === Constants.prismaUniqueConflictcode) { | ||
| this.logger.warn( | ||
| `Attempt to assign role ${roleId} to subject ${subjectId} which already exists.`, | ||
| `Attempt to assign role ${roleId} to subject ${subjectId} which already exists. Ignoring duplicate.`, |
There was a problem hiding this comment.
[correctness]
The change from throwing a ConflictException to returning silently when a duplicate role assignment is detected may lead to silent failures. Consider whether the caller needs to be informed of this situation to handle it appropriately.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
| constructor() { | ||
| super({ | ||
| transactionOptions: { | ||
| timeout: process.env.IDENTITY_SERVICE_PRISMA_TIMEOUT |
There was a problem hiding this comment.
[❗❗ correctness]
Consider validating that process.env.IDENTITY_SERVICE_PRISMA_TIMEOUT is a valid number before using parseInt. If the environment variable is not a valid number, parseInt will return NaN, which could lead to unexpected behavior.
| stats.fetches += 1; | ||
| stats.matched += count; | ||
| if (cliOptions.mode === 'delta' && count === 0 && stats.fetches === 1) { | ||
| console.warn(`[delta] ${label}: first fetch returned 0 rows during delta migration.`); |
There was a problem hiding this comment.
[💡 maintainability]
The warning message for the first fetch returning 0 rows during delta migration might be too verbose if this is expected behavior for certain tables. Consider adding a mechanism to suppress this warning for known cases where 0 rows are expected.
| await client.$queryRawUnsafe('SELECT 1'); | ||
| console.log(`[delta] Connectivity check OK: ${label}`); | ||
| } catch (err: any) { | ||
| throw new Error(`[delta] Connectivity check failed for ${label}: ${err.message}`); |
There was a problem hiding this comment.
[❗❗ security]
Using client.$queryRawUnsafe('SELECT 1') can be risky if the query string is dynamically constructed elsewhere. Ensure that any dynamic parts of the query are properly sanitized to prevent SQL injection vulnerabilities.
| console.log(`[delta] ${label}: counts OK (${sourceCount}).`); | ||
| } | ||
| } catch (err: any) { | ||
| console.warn(`[delta] ${label}: unable to verify counts (${err.message}).`); |
There was a problem hiding this comment.
[maintainability]
Consider adding more detailed error handling for the count verification process. Currently, errors are logged with a warning, but it might be beneficial to categorize errors (e.g., network issues, permission issues) for easier troubleshooting.
| // assign primary role | ||
| await this.roleService.assignRoleByName(primaryRole, userId, userId); | ||
| // Assign 'Topcoder User' at the same time, to avoid weird issues after the first login | ||
| if (primaryRole == 'Topcoder Talent') { |
There was a problem hiding this comment.
[correctness]
Consider using === instead of == for strict equality check between primaryRole and 'Topcoder Talent'. This ensures type safety and avoids potential issues with type coercion.
3 participants
No description provided.