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 integration schemas and types to @keyshade/schema #547

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Nov 22, 2024

User description

Description

@keyshade/schema

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

@keyshade/api-client

  • Remove existing integration.types.d.ts.
  • Change imports for integration types to @keyshade/schema.

Related to #519

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 integration schemas and types to the @keyshade/schema package.
  • Updated api-client to import integration types from @keyshade/schema.
  • Removed deprecated integration.types.d.ts file from api-client.
  • Added new integration schemas and types in the schema package.
  • Implemented comprehensive tests for the new integration schemas.
  • Refactored ProjectSchema to use a base schema for consistency.

Changes walkthrough 📝

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

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

  • Updated import paths for integration types to use @keyshade/schema.
  • +1/-1     
    integration.types.d.ts
    Remove deprecated integration types file                                 

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

  • Removed the entire file as integration types are now in
    @keyshade/schema.
  • +0/-89   
    index.types.ts
    Export integration types from new module                                 

    packages/schema/src/index.types.ts

    • Exported integration types from the new integration module.
    +1/-4     
    index.ts
    Add integration schemas for requests and responses             

    packages/schema/src/integration/index.ts

  • Added new integration schemas including request and response schemas.
  • +64/-0   
    index.types.ts
    Add type definitions for integration schemas                         

    packages/schema/src/integration/index.types.ts

    • Added type definitions inferred from integration schemas.
    +54/-0   
    index.ts
    Refactor ProjectSchema to use BaseProjectSchema                   

    packages/schema/src/project/index.ts

    • Refactored ProjectSchema to use BaseProjectSchema.
    +22/-22 
    Bug fix
    user.ts
    Correct URL path in validateEmailChangeOTP method               

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

    • Fixed URL path in validateEmailChangeOTP method.
    +1/-1     
    Tests
    integration.spec.ts
    Add tests for integration schemas and types                           

    packages/schema/tests/integration.spec.ts

  • Added comprehensive tests for integration schemas.
  • Included validation tests for all integration schema types.
  • +347/-10

    💡 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 integration folder with schema and types files
    • Added zod schemas for integration base type, requests and responses
    • Added type exports in integration/index.types.ts
    • Updated api-client to use types from schema package
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Schema Validation
    Verify that the metadata field validation in IntegrationSchema is sufficient for all integration types, as it currently accepts any string key-value pairs

    Type Safety
    The UpdateIntegrationRequestSchema makes all fields optional which could lead to empty update requests. Consider adding validation for minimum required fields

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Move sensitive data from URL query parameters to request body

    Pass the OTP value in the request body instead of as a query parameter to avoid
    exposing sensitive data in URLs and server logs.

    packages/api-client/src/controllers/user.ts [48]

     const response = await this.apiClient.post(
    -  `/api/user/validate-email-change-otp?otp=${request.otp}`,
    -  request,
    +  '/api/user/validate-email-change-otp',
    +  { ...request, otp: request.otp },
       headers
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Moving OTP from URL query parameters to request body is a critical security improvement, as sensitive data in URLs can be exposed in logs and browser history.

    9
    General
    Add validation to prevent empty string values in metadata records

    Add validation to ensure metadata values are not empty strings, as empty metadata
    values could cause issues with integrations.

    packages/schema/src/integration/index.ts [12]

     export const IntegrationSchema = z.object({
    -  metadata: z.record(z.string()),
    +  metadata: z.record(z.string().min(1)),
     })
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding validation to prevent empty metadata values is important for data integrity and preventing potential integration issues. Empty strings could cause problems with integration functionality.

    7

    💡 Need additional feedback ? start a PR chat

    @muntaxir4 muntaxir4 requested a review from rajdip-b November 22, 2024 06:41
    @muntaxir4 muntaxir4 changed the title feat(schema, api-client): Migrate event schemas and types to @keyshade/schema feat(schema, api-client): Migrate integration schemas and types to @keyshade/schema Nov 22, 2024
    @rajdip-b rajdip-b merged commit 08868c3 into keyshade-xyz:develop Nov 22, 2024
    7 checks passed
    @muntaxir4 muntaxir4 deleted the integration-schemas branch November 23, 2024 21:16
    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