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

feat(schema, api-client): Migrate project types and schemas to @keyshade/schema #537

Closed
wants to merge 2 commits into from

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Nov 15, 2024

User description

Description

@keyshade/schema

  • Add schemas and types for project.
  • Add tests for project schemas.
  • Export project schemas from index and its types from index.types

@keyshade/api-client

  • Remove existing project.types.
  • Change imports for project types to @keyshade/schema.

Related to #519

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

  • 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

  • Migrated project types and schemas from api-client to @keyshade/schema.
  • Updated api-client to import project types from @keyshade/schema.
  • Added comprehensive tests for the new project schemas in the schema package.
  • Removed redundant project type definitions from api-client.
  • Enhanced project schema definitions with detailed validation logic.

Changes walkthrough 📝

Relevant files
Enhancement
project.ts
Update project type imports to use schema package               

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

  • Updated imports to use project types from @keyshade/schema.
+1/-1     
project.types.d.ts
Remove project type definitions from api-client                   

packages/api-client/src/types/project.types.d.ts

  • Removed all project-related type definitions.
+0/-87   
index.ts
Update export path for project schemas                                     

packages/schema/src/index.ts

  • Updated export path for project schemas.
+3/-1     
index.types.ts
Refactor project type exports in schema index                       

packages/schema/src/index.types.ts

  • Removed direct exports of project schemas.
  • Added export for project types from project.types.ts.
  • +2/-8     
    project.ts
    Remove old project schema definitions                                       

    packages/schema/src/project.ts

    • Removed old project schema definitions.
    +0/-19   
    project.ts
    Add detailed project schema definitions                                   

    packages/schema/src/project/project.ts

    • Added detailed project schema definitions.
    +99/-0   
    project.types.ts
    Add inferred type definitions for project schemas               

    packages/schema/src/project/project.types.ts

    • Added type definitions inferred from project schemas.
    +62/-0   
    Tests
    project.spec.ts
    Add comprehensive tests for project schemas                           

    packages/schema/tests/project.spec.ts

  • Added extensive tests for project schemas.
  • Validated various project schema scenarios.
  • +557/-20

    💡 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:

    🎫 Ticket compliance analysis ✅

    519 - Fully compliant

    Compliant requirements:

    • Created project folder in schema package
    • Added project.ts with zod schemas
    • Added project.types.ts with inferred types
    • Updated index.ts to export project schemas
    • Updated api-client to use project types from schema

    Non-compliant requirements:

    None

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The ProjectSchema includes privateKey and publicKey fields that are exposed in responses. Consider masking or excluding these sensitive fields from non-essential response schemas to minimize potential security risks.

    ⚡ Recommended focus areas for review

    Code Smell
    The ProjectSchema validation logic for isForked and forkedFromId relationship could be simplified and made more explicit

    Validation Gap
    The UpdateProjectRequestSchema allows regenerateKeyPair without requiring privateKey, which may lead to inconsistent states

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Add validation for cryptographic key formats to prevent invalid key data

    Add URL validation for the publicKey and privateKey fields to ensure they contain
    valid key data in the expected format.

    packages/schema/src/project/project.ts [14-15]

     export const ProjectSchema = z
       .object({
    -    publicKey: z.string(),
    -    privateKey: z.string(),
    +    publicKey: z.string().regex(/^[A-Za-z0-9+/=_-]+$/),
    +    privateKey: z.string().regex(/^[A-Za-z0-9+/=_-]+$/),
         // ...other fields
       })
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Security-critical suggestion that adds validation for cryptographic keys to ensure they contain valid base64-like characters, preventing potential security issues with malformed keys.

    9
    Best practice
    Add string length validation for input fields to prevent invalid data

    Add validation for the name field length to ensure project names aren't too long or
    too short. Consider adding a minimum length of 1 and maximum length of 100
    characters.

    packages/schema/src/project/project.ts [29-36]

     export const CreateProjectRequestSchema = z.object({
    -  name: z.string(),
    +  name: z.string().min(1).max(100),
       workspaceSlug: z.string(),
       description: z.string().optional(),
       storePrivateKey: z.boolean().optional(),
       environments: CreateEnvironmentSchema.array().optional(),
       accessLevel: projectAccessLevelEnum
     })
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding length validation for the project name is crucial to prevent data issues and ensure consistent UI display. The suggestion provides specific bounds that help maintain data quality.

    8
    Add validation for URL-safe slug format to ensure consistent naming

    Add validation for the slug field to ensure it follows a valid slug format with only
    lowercase alphanumeric characters and hyphens.

    packages/schema/src/project/project.ts [10]

     export const ProjectSchema = z
       .object({
    -    slug: z.string(),
    +    slug: z.string().regex(/^[a-z0-9-]+$/),
         // ...other fields
       })
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Important validation that ensures slugs follow a consistent URL-safe format, preventing potential routing and display issues in the application.

    7

    💡 Need additional feedback ? start a PR chat

    @rajdip-b
    Copy link
    Member

    I think you have mistakenly added the changes for project module into #538. Can you remove those changes?

    @muntaxir4
    Copy link
    Contributor Author

    I think you have mistakenly added the changes for project module into #538. Can you remove those changes?

    The project schema depends on CreateEnvironmentSchema from @/environment. However, with the new environment schemas in @/environment/environment, the schema changes to CreateEnvironmentRequestSchema.

    If I make #538 independent of this PR, #538 will remove @/environment.ts, causing test and build errors since this PR depends on it.

    I could close this PR and update #538's description to include changes for both project and environment schemas.

    @rajdip-b
    Copy link
    Member

    Yes i think that would be a better choice!

    @muntaxir4 muntaxir4 closed this Nov 18, 2024
    @muntaxir4 muntaxir4 deleted the project-schema branch November 19, 2024 04:42
    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