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.

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(api): Change type of id field of models to uuid #232

Closed

Conversation

Ratnesh2003
Copy link
Contributor

@Ratnesh2003 Ratnesh2003 commented May 20, 2024

User description

Description

Modified all the id fields inside each model in prisma and changed their types to uuid

Fixes #231

Dependencies

Mention any dependencies/packages used

Future Improvements

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

Mentions

@rajdip-b

Screenshots of relevant screens

Add screenshots of relevant screens

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement, dependencies


Description

  • Changed the default value of id fields from cuid() to uuid() in multiple Prisma models to ensure consistency and uniqueness.
  • Updated dependencies in pnpm-lock.yaml to reflect the changes.

Changes walkthrough 📝

Relevant files
Enhancement
schema.prisma
Change `id` field default value to `uuid()` in Prisma models.

apps/api/src/prisma/schema.prisma

  • Changed the default value of id fields from cuid() to uuid() in
    multiple models.
  • Updated models include Event, Notification, User, Subscription,
    Integration, Environment, Project, ProjectWorkspaceRoleAssociation,
    WorkspaceRole, WorkspaceMemberRoleAssociation, WorkspaceMember,
    SecretVersion, Secret, VariableVersion, Variable, ApiKey, Otp,
    Workspace, Approval, and ChangeNotificationSocketMap.
  • +19/-19 
    Dependencies
    pnpm-lock.yaml
    Update dependencies in `pnpm-lock.yaml`.                                 

    pnpm-lock.yaml

    • Updated dependencies as part of the changes.
    +10783/-8812

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

    Copy link
    Contributor

    PR Description updated to latest commit (6868f2d)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and consistent across multiple models, primarily involving the replacement of 'cuid()' with 'uuid()' for ID fields. The scope is limited to type changes in the schema file, which is easy to verify.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Data Migration Needed: Changing the ID generation method from CUID to UUID might require data migration or updates in the database if existing records use the old ID format. This needs to be handled to avoid issues with foreign key relations or data consistency.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Plan a migration strategy for existing ids from cuid() to uuid()

    Consider adding a migration strategy for existing records to convert their id from cuid()
    to uuid(), ensuring data consistency and integrity.

    apps/api/src/prisma/schema.prisma [182]

    -id          String         @id @default(uuid())
    +id          String         @id @default(uuid()) // Plan migration for existing cuid() ids
     
    Suggestion importance[1-10]: 9

    Why: Planning a migration strategy for existing records is essential for maintaining data consistency and integrity. This suggestion addresses a significant aspect of the change and ensures a smooth transition from cuid() to uuid().

    9
    Possible issue
    Verify the availability of the uuid() function

    Ensure that the uuid() function is correctly imported or available in the Prisma
    environment to prevent runtime errors.

    apps/api/src/prisma/schema.prisma [182]

    -id          String         @id @default(uuid())
    +id          String         @id @default(uuid()) // Ensure uuid() is imported or available
     
    Suggestion importance[1-10]: 7

    Why: Ensuring that the uuid() function is correctly imported or available is important to prevent runtime errors. This suggestion addresses a potential issue that could cause failures if not checked.

    7
    Database compatibility
    Confirm database support for using uuid() as a default value

    Review the database schema to ensure that the uuid() function is supported as a default
    value and that it complies with the database's requirements for UUIDs.

    apps/api/src/prisma/schema.prisma [182]

    -id          String         @id @default(uuid())
    +id          String         @id @default(uuid()) // Confirm DB support for uuid() as default
     
    Suggestion importance[1-10]: 6

    Why: Verifying database compatibility for the uuid() function as a default value is crucial for ensuring that the schema works as intended across different database systems. This suggestion is useful but somewhat redundant if the environment is already known to support it.

    6
    Enhancement
    Enforce UUID format by specifying the type explicitly

    Consider adding an explicit type for the id fields to ensure they are UUIDs. Currently,
    the type is set to String which is too generic and might not enforce the UUID format
    strictly.

    apps/api/src/prisma/schema.prisma [182]

    -id          String         @id @default(uuid())
    +id          String @uuid   @id @default(uuid())
     
    Suggestion importance[1-10]: 3

    Why: While specifying the type explicitly as UUID could enhance clarity, Prisma does not support a specific UUID type. The current implementation using String with a default of uuid() is standard practice in Prisma.

    3

    @rajdip-b rajdip-b changed the title Modify(api): Change type of id field of models to uuid chore(api): Change type of id field of models to uuid May 20, 2024
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    You forgot to generate the migration files! Do so using pnpm db:generate-migrations

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    Also, we would need the uuid to be v4 explicitely. Follow up the thread I dropped in the issue to understand how that works.

    @Ratnesh2003
    Copy link
    Contributor Author

    Using this should do the job, right?

    model Event {
      id      String     @id @db.Uuid @default(dbgenerated("gen_random_uuid()"))
      ...
    }

    @rajdip-b
    Copy link
    Member

    rajdip-b commented May 20, 2024

    I think you will need to use uuid_generate_v4 instead of gen_random_uuid. Also, test if it works for you or not!

    @Ratnesh2003
    Copy link
    Contributor Author

    Both of the functions gen_random_uuid() and uuid_generate_v4() generates a V4 UUID string.
    https://www.postgresql.org/docs/current/functions-uuid.html#FUNCTIONS-UUID

    But to be specific about version, yes I think uuid_generate_v4() would be better.

    @rajdip-b
    Copy link
    Member

    Wow! Okay I didn't know that, but yeah, staying version specific would help!

    @rajdip-b rajdip-b force-pushed the modify/id-field-type branch from 6868f2d to e77dba3 Compare May 20, 2024 14:12
    Copy link

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    @rajdip-b rajdip-b closed this May 23, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    API: Convert all id types in schema.prisma to be of type UUID v4
    2 participants