search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

fix(API): Update the functionality by which slugs are generated for entities #419

Closed Nil2000 closed 3 weeks ago

Nil2000 commented 3 weeks ago

User description

Description

Updated keyshade/apps/api/src/common/slug-generator.ts as per mentioned in the issue

Fixes #416

Dependencies

Mention any dependencies/packages used

Future Improvements

Mention any improvements to be done in future related to any file/feature

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

If changes are made in the code:

Documentation Update

PR Type

enhancement, bug_fix

Description

Changes walkthrough 📝

Relevant files
Bug fix
api-key.service.ts
Refine error messages in API key service                                 
apps/api/src/api-key/service/api-key.service.ts
  • Updated error messages to use apiKeySlug instead of apiKeyId.
  • Improved clarity of error handling in API key operations.
    		•  +3/-3     
    Enhancement
    slug-generator.ts
    Enhance slug generation with counter mechanism                     
    apps/api/src/common/slug-generator.ts
  • Modified generateSlug function to include a counter for uniqueness.
  • Implemented a loop to increment the counter if a slug exists.
  • Enhanced slug generation logic for various entity types.
    		•  +12/-3   

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-free[bot] commented 3 weeks ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Performance Issue
    The while loop for generating unique slugs might run indefinitely if there's a high collision rate. Consider adding a maximum retry limit. Code Duplication
    The switch statement contains repetitive code for each entity type. Consider refactoring to reduce duplication.
    codiumai-pr-agent-free[bot] commented 3 weeks ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Reorder operations to prevent potential null reference errors ___ **Consider moving the apiKey.id assignment after the null check to avoid potential
    runtime errors if apiKey is null.** [apps/api/src/api-key/service/api-key.service.ts [88-97]](https://github.com/keyshade-xyz/keyshade/pull/419/files#diff-9fefafaa7fbbd90c1f65410a2414bfab311e6faeb1277c532fe372060a33ebefR88-R97) ```diff const apiKey = await this.prisma.apiKey.findUnique({ where: { slug: apiKeySlug } }) -const apiKeyId = apiKey.id if (!apiKey) { throw new NotFoundException(`API key ${apiKeySlug} not found`) } +const apiKeyId = apiKey.id + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: The suggestion addresses a potential runtime error by ensuring that `apiKey` is not null before accessing its `id` property, which is a crucial fix for preventing application crashes.
    		9
    Performance
    Optimize the slug generation and uniqueness check process ___ **Consider implementing a more efficient approach to handle slug generation and
    uniqueness checks, such as using a database transaction or a more optimized loop
    structure.** [apps/api/src/common/slug-generator.ts [92-157]](https://github.com/keyshade-xyz/keyshade/pull/419/files#diff-ff09e6d1b82950e221781927d734548ae6fd8a7d2001bb0cadddfaefcda25bbeR92-R157) ```diff export default async function generateEntitySlug( name: string, - entityType: - | 'WORKSPACE_ROLE' - | 'WORKSPACE' - | 'PROJECT' - | 'VARIABLE' - | 'SECRET' - | 'INTEGRATION' - | 'ENVIRONMENT' - | 'API_KEY', + entityType: 'WORKSPACE_ROLE' | 'WORKSPACE' | 'PROJECT' | 'VARIABLE' | 'SECRET' | 'INTEGRATION' | 'ENVIRONMENT' | 'API_KEY', prisma: PrismaService ): Promise { - let counter=0 - while (true) { - const slug = generateSlug(name,counter) - switch (entityType) { - case 'WORKSPACE_ROLE': - if (await checkWorkspaceRoleSlugExists(slug, prisma)) { - counter++ - continue - } - return slug - case 'WORKSPACE': - if (await checkWorkspaceSlugExists(slug, prisma)) { - counter++ - continue - } - return slug - // ... other cases - } - } + const checkSlugExists = getSlugExistenceChecker(entityType, prisma); + let counter = 0; + let slug: string; + + do { + slug = generateSlug(name, counter); + counter++; + } while (await checkSlugExists(slug)); + + return slug; } +function getSlugExistenceChecker(entityType: string, prisma: PrismaService) { + const checkers = { + 'WORKSPACE_ROLE': checkWorkspaceRoleSlugExists, + 'WORKSPACE': checkWorkspaceSlugExists, + // ... other entity types + }; + return checkers[entityType] || (() => false); +} + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: The suggestion optimizes the slug generation process by reducing repetitive code and improving performance, which is beneficial for scalability and maintainability.
    		8
    Enhancement
    Enhance slug uniqueness by incorporating a timestamp ___ **Consider using a more robust method for generating unique slugs, such as
    incorporating a timestamp or a UUID, to reduce the likelihood of collisions.** [apps/api/src/common/slug-generator.ts [11-24]](https://github.com/keyshade-xyz/keyshade/pull/419/files#diff-ff09e6d1b82950e221781927d734548ae6fd8a7d2001bb0cadddfaefcda25bbeR11-R24) ```diff -const generateSlug = (name: string,counter:number): string => { - // Convert to lowercase +const generateSlug = (name: string, counter: number): string => { const lowerCaseName = name.trim().toLowerCase() - - // Replace spaces with hyphens const hyphenatedName = lowerCaseName.replace(/\s+/g, '-') - - // Replace all non-alphanumeric characters with hyphens const alphanumericName = hyphenatedName.replace(/[^a-zA-Z0-9-]/g, '-') - - // Append the name with 5 alphanumeric characters - const slug = - alphanumericName + '-' + counter.toString(36) + const timestamp = Date.now().toString(36) + const slug = `${alphanumericName}-${timestamp}-${counter.toString(36)}` return slug } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Incorporating a timestamp enhances the uniqueness of slugs, reducing collision risk, which is a valuable improvement for ensuring data integrity.
    		7
    Best practice
    Implement a maximum length limit for generated slugs ___ **Consider adding a maximum length limit to the generated slug to ensure it doesn't
    exceed database or URL length restrictions.** [apps/api/src/common/slug-generator.ts [11-24]](https://github.com/keyshade-xyz/keyshade/pull/419/files#diff-ff09e6d1b82950e221781927d734548ae6fd8a7d2001bb0cadddfaefcda25bbeR11-R24) ```diff -const generateSlug = (name: string,counter:number): string => { - // Convert to lowercase +const MAX_SLUG_LENGTH = 100; // Adjust as needed + +const generateSlug = (name: string, counter: number): string => { const lowerCaseName = name.trim().toLowerCase() - - // Replace spaces with hyphens const hyphenatedName = lowerCaseName.replace(/\s+/g, '-') - - // Replace all non-alphanumeric characters with hyphens const alphanumericName = hyphenatedName.replace(/[^a-zA-Z0-9-]/g, '-') - - // Append the name with 5 alphanumeric characters - const slug = - alphanumericName + '-' + counter.toString(36) - return slug + const counterSuffix = '-' + counter.toString(36) + const truncatedName = alphanumericName.slice(0, MAX_SLUG_LENGTH - counterSuffix.length) + return truncatedName + counterSuffix } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Adding a maximum length limit is a good practice to prevent potential issues with database or URL length restrictions, improving code robustness.
    		6