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

refactor(api-client, schema): Add workspace's schemas and types in @keyshade/schema #520

Merged
merged 4 commits into from
Nov 10, 2024

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Nov 6, 2024

User description

  • Modify tsconfig to include strictNullChecks:true in order to fix properties becoming optional.
  • Create zod schemas and types for workspace and include additional tests.
  • Add extra required schemas: PageRequest, PageResponse, ResponseError and ClientResponse in pagination folder. Additionally create tests for this.
  • Add the schema package as a dependency for api-client and update the existing code to replace dependencies on the workspace's types with the new types from the schema package for better compatibility.

Related to #519

Description

The changes aim to improve schema validation, update dependencies, and enhance test coverage for Workspace schemas.

Affected apps: packages/api-client and packages/schema

Dependencies

N/A

Future Improvements

N/A

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, tests


Description

  • Introduced new Zod schemas for workspace operations, including CRUD, data export, and global search.
  • Added extensive test cases to validate the new workspace schemas and ensure proper error handling.
  • Updated the api-client to use the new schema package, replacing old workspace types.
  • Configured the schema package with necessary dependencies and exports.
  • Updated pnpm-lock.yaml to reflect changes in dependencies and package versions.

Changes walkthrough 📝

Relevant files
Tests
workspace.spec.ts
Add test cases for workspace schemas validation                   

packages/schema/tests/workspace.spec.ts

  • Added extensive test cases for new workspace schemas.
  • Validated input for various workspace-related schemas.
  • Ensured proper error handling for invalid inputs.
  • +394/-1 
    Enhancement
    workspace.ts
    Define Zod schemas for workspace operations                           

    packages/schema/src/workspace/workspace.ts

  • Defined new Zod schemas for workspace operations.
  • Included request and response schemas for CRUD operations.
  • Added schemas for data export and global search.
  • +155/-0 
    index.ts
    Add pagination and response schemas                                           

    packages/schema/src/index.ts

  • Introduced pagination and response error schemas.
  • Added client response schema for API responses.
  • Exported new workspace schemas.
  • +52/-2   
    workspace.types.ts
    Create TypeScript types for workspace schemas                       

    packages/schema/src/workspace/workspace.types.ts

  • Created TypeScript types for workspace schemas.
  • Mapped Zod schemas to TypeScript types.
  • +62/-0   
    workspace.ts
    Update workspace controller to use new schemas                     

    packages/api-client/src/controllers/workspace.ts

  • Updated imports to use new schema package.
  • Replaced old workspace types with new schema types.
  • +2/-2     
    Dependencies
    pnpm-lock.yaml
    Update dependencies for schema integration                             

    pnpm-lock.yaml

  • Updated dependencies to include new schema package.
  • Adjusted package versions and dependencies.
  • +260/-133
    package.json
    Add schema package dependency                                                       

    packages/api-client/package.json

    • Added schema package as a dependency.
    +3/-1     
    Configuration changes
    package.json
    Configure schema package dependencies and exports               

    packages/schema/package.json

  • Added Zod as a dependency.
  • Configured exports for the schema package.
  • +6/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Oversight
    The UpdateWorkspaceRequestSchema extends CreateWorkspaceRequestSchema.partial(), which might allow updating fields that should be immutable after creation. Consider explicitly defining which fields can be updated.

    Type Safety
    The ClientResponseSchema uses .nullable() for both error and data fields. This might lead to ambiguous states where both fields are null. Consider using a union type instead to ensure only one is defined at a time.

    Test Coverage
    While there are many test cases, some schemas like InviteMemberSchema lack comprehensive tests. Consider adding more test cases to cover all possible scenarios and edge cases.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Nov 6, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    ✅ Use a more appropriate data type for date fields to ensure data integrity
    Suggestion Impact:The rotateAt field was changed from z.string() to rotateAfterEnum, which suggests an improvement in type safety, similar to the suggestion's intent.

    code diff:

    -          rotateAt: z.string(),
    +          rotateAt: rotateAfterEnum,

    The rotateAt field in the secrets array is currently defined as a string. Consider
    using z.date() instead to ensure it's a valid date.

    packages/schema/src/workspace/workspace.ts [96]

    -rotateAt: z.string(),
    +rotateAt: z.date(),
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using z.date() instead of z.string() for date fields is a significant improvement. It ensures proper date validation and can prevent issues related to invalid date formats.

    8
    Enhancement
    Implement stricter validation for color code fields to ensure they are valid hex color codes

    Consider using a more specific type for the colorCode field in the workspaceRoles
    array. Instead of using z.string(), you could use a regex pattern to ensure it's a
    valid hex color code.

    packages/schema/src/workspace/workspace.ts [73]

    -colorCode: z.string(),
    +colorCode: z.string().regex(/^#[0-9A-Fa-f]{6}$/),
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves data validation by ensuring that color codes are in the correct format. It's a valuable enhancement that can prevent potential issues with invalid color codes.

    7
    Add constraints to numeric fields to prevent invalid input values

    In the PageRequestSchema, consider adding constraints to the page and limit fields
    to ensure they are positive integers.

    packages/schema/src/index.ts [4-5]

    -page: z.number().optional(),
    -limit: z.number().optional(),
    +page: z.number().int().positive().optional(),
    +limit: z.number().int().positive().optional(),
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding constraints to ensure positive integers for page and limit fields is a valuable enhancement. It prevents potential issues with invalid pagination parameters, improving the robustness of the API.

    7
    ✅ Add TypeScript declaration file specification for improved type support

    Consider adding a "types" field to specify the TypeScript declaration file for
    better type support when other packages import this schema package.

    packages/schema/package.json [4-9]

     "main": "dist/src/index.js",
    +"types": "dist/src/index.d.ts",
     "description": "This package holds the schemas that other applications can use to validate the input data.",
     "private": true,
     "exports": {
    -  ".": "./dist/src/index.js"
    +  ".": {
    +    "types": "./dist/src/index.d.ts",
    +    "default": "./dist/src/index.js"
    +  }
     },

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: This suggestion improves TypeScript integration by explicitly specifying the types file, enhancing type support for consumers of the schema package.

    7
    Maintainability
    ✅ Define enums separately to improve code organization and reusability
    Suggestion Impact:The accessLevel field was changed to use a separately defined enum, projectAccessLevelEnum, as suggested.

    code diff:

    -      accessLevel: z.enum(['GLOBAL', 'PRIVATE', 'INTERNAL']),
    +      accessLevel: projectAccessLevelEnum,

    The accessLevel field in the projects array uses an enum with string literals.
    Consider defining this enum separately for better reusability and maintainability.

    packages/schema/src/workspace/workspace.ts [84]

    -accessLevel: z.enum(['GLOBAL', 'PRIVATE', 'INTERNAL']),
    +accessLevel: ProjectAccessLevelEnum,
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Defining enums separately enhances code maintainability and reusability. While it's a good practice, the impact is moderate as it primarily affects code organization rather than functionality.

    6

    💡 Need additional feedback ? start a PR chat

    @muntaxir4 muntaxir4 changed the title PACKAGE: Add workspace's schemas and types in @keyshade/schema refactor: Add workspace's schemas and types in @keyshade/schema Nov 6, 2024
    @rajdip-b rajdip-b changed the title refactor: Add workspace's schemas and types in @keyshade/schema refactor(package): Add workspace's schemas and types in @keyshade/schema Nov 6, 2024
    @muntaxir4 muntaxir4 changed the title refactor(package): Add workspace's schemas and types in @keyshade/schema refactor(api-client, schema): Add workspace's schemas and types in @keyshade/schema Nov 6, 2024
    packages/schema/src/index.ts Outdated Show resolved Hide resolved
    packages/schema/src/index.ts Show resolved Hide resolved
    packages/schema/src/workspace/workspace.ts Outdated Show resolved Hide resolved
    packages/schema/src/workspace/workspace.ts Outdated Show resolved Hide resolved
    packages/schema/src/workspace/workspace.ts Outdated Show resolved Hide resolved
    packages/schema/src/workspace/workspace.ts Outdated Show resolved Hide resolved
    packages/schema/src/workspace/workspace.ts Outdated Show resolved Hide resolved
    packages/schema/src/workspace/workspace.ts Outdated Show resolved Hide resolved
    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.

    image
    Think you missed this

    @rajdip-b rajdip-b force-pushed the zod-schema branch 2 times, most recently from 7d0c278 to 510ba40 Compare November 6, 2024 17:09
    … package
    
    - Export zod schemas and types for workspace and include its tests.
    - Add extra schemas required in modules PageRequest.
    - Add schema package as dependency for api-client and replace existing dependency on workspace's types.
    …hanges to workspace schema
    
    - Add pagination tests
    - Change enums imports
    - Add strictNullChecks in tsconfig
    @muntaxir4 muntaxir4 requested a review from rajdip-b November 7, 2024 13:22
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Nov 7, 2024

    I think this looks good. I'll set up this PR locally and test it out

    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.

    The code looks perfect to me. Once you make the changes, we can merge

    packages/schema/src/enums/enums.types.ts Outdated Show resolved Hide resolved
    packages/schema/src/enums/enums.ts Outdated Show resolved Hide resolved
    Copy link

    codecov bot commented Nov 9, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 96.29%. Comparing base (ce50743) to head (bc3412c).
    Report is 213 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #520      +/-   ##
    ===========================================
    + Coverage    91.71%   96.29%   +4.58%     
    ===========================================
      Files          111       23      -88     
      Lines         2510      351    -2159     
      Branches       469        8     -461     
    ===========================================
    - Hits          2302      338    -1964     
    + Misses         208       13     -195     
    Flag Coverage Δ
    api-client 95.47% <ø> (?)
    api-e2e-tests ?
    schema 100.00% <100.00%> (?)

    Flags with carried forward coverage won't be shown. Click here to find out more.

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @muntaxir4
    Copy link
    Contributor Author

    muntaxir4 commented Nov 9, 2024

    The code looks perfect to me. Once you make the changes, we can merge

    Okay, removed the enums directory and kept only enums.ts

    @muntaxir4 muntaxir4 requested a review from rajdip-b November 9, 2024 11:48
    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.

    LGTM

    @rajdip-b rajdip-b merged commit 7c8ee5d into keyshade-xyz:develop Nov 10, 2024
    11 checks passed
    @muntaxir4 muntaxir4 deleted the zod-schema branch November 19, 2024 04:42
    rajdip-b pushed a commit that referenced this pull request Dec 3, 2024
    ## [2.8.0](v2.7.0...v2.8.0) (2024-12-03)
    
    ### 🚀 Features
    
    * **api:** Add workspace removal notification email template ([#476](#476)) ([40b754f](40b754f))
    * **cli:** Store `metrics_enabled` key in profile config ([#536](#536)) ([9283b22](9283b22))
    * **package, api, cli:** Add api-key schemas and types; Fix schema inconsistencies; Minor fix for CLI build errors  ([#557](#557)) ([126d024](126d024))
    * **platform:** Added screen for CREATE NEW PROJECT ([#540](#540)) ([b644633](b644633))
    * **platform:** Updated the empty state of dashboard ([#522](#522)) ([28739d9](28739d9))
    * **schema, api-client:** Migrate auth types to @keyshade/schema ([#532](#532)) ([d880098](d880098))
    * **schema, api-client:** Migrate event schemas and types to @keyshade/schema ([#546](#546)) ([a3267de](a3267de))
    * **schema, api-client:** Migrate integration schemas and types to @keyshade/schema ([#547](#547)) ([08868c3](08868c3))
    * **schema, api-client:** Migrate project schemas and environment schemas along with their types to @keyshade/schema ([#538](#538)) ([c468af0](c468af0))
    * **schema, api-client:** Migrate [secure] types and schemas to @keyshade/schema ([#539](#539)) ([bc3100b](bc3100b))
    * **schema, api-client:** Migrate user types and schemas to @keyshade/schema ([#535](#535)) ([c24695e](c24695e))
    * **schema, api-client:** Migrate variable schemas and types to @keyshade/schema ([#545](#545)) ([0ee8f9a](0ee8f9a))
    * **schema, api-client:** Migrate workspace-membership schemas and types to @keyshade/schema ([#569](#569)) ([4398969](4398969))
    * **schema, api-client:** Migrate workspace-role schemas and types to @keyshade/schema ([#568](#568)) ([9efbf2d](9efbf2d))
    * **schema:** Add User type inference from UserSchema ([#574](#574)) ([84c1db5](84c1db5))
    
    ### 🐛 Bug Fixes
    
    * **api:** Incorrect oauth redirect url ([58d96e5](58d96e5))
    * **platform:** Resolve loading SVG blocking input field interaction ([#571](#571)) ([30f4f65](30f4f65))
    
    ### 📚 Documentation
    
    * Add pictures to Bruno setup ([#541](#541)) ([210c0fd](210c0fd))
    * Migrate to Bruno ([#525](#525)) ([1793d92](1793d92))
    
    ### 🔧 Miscellaneous Chores
    
    * **ci:** Add script to validate schema package ([59e4280](59e4280))
    * Fixed codecov client version ([a998ae4](a998ae4))
    * **package:** Fixed tests and did housekeeping ([#544](#544)) ([40008e3](40008e3))
    * Update test coverage settings ([5b27e32](5b27e32))
    * Update Turbo to 2.3.1 ([#564](#564)) ([3a63823](3a63823))
    * **web:** Update dockerfile ([10d9cc5](10d9cc5))
    
    ### 🔨 Code Refactoring
    
    * **api-client, schema:** Add workspace's schemas and types in @keyshade/schema ([#520](#520)) ([7c8ee5d](7c8ee5d))
    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.

    2 participants