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(api,cli,api-client,schema): Make get secret and get variable response to be consistent with their getAll #625

Closed

Conversation

muntaxir4
Copy link
Contributor

@muntaxir4 muntaxir4 commented Jan 12, 2025

User description

Description

Created methods under common directory to parse the prisma response to required ones.

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, Bug fix, Tests


Description

  • Refactored Secret and Variable schemas to include nested structures for consistency.

  • Updated API and CLI logic to align with new schema structure.

  • Enhanced test cases to validate updated schema and API responses.

  • Fixed workspace invitation client implementation and pagination issues.


Changes walkthrough 📝

Relevant files
Tests
9 files
variable.spec.ts
Updated tests for Variable schema structure changes.         
+84/-59 
secret.spec.ts
Updated tests for Secret schema structure changes.             
+70/-50 
project.e2e.spec.ts
Adjusted project controller tests for schema updates.       
+120/-104
secret.e2e.spec.ts
Updated Secret controller tests for schema changes.           
+35/-31 
variable.e2e.spec.ts
Updated Variable controller tests for schema changes.       
+32/-28 
event.e2e.spec.ts
Updated Event controller tests for schema changes.             
+33/-29 
variable.spec.ts
Updated API client Variable tests for schema changes.       
+7/-7     
secret.spec.ts
Updated API client Secret tests for schema changes.           
+6/-6     
event.spec.ts
Updated API client Event tests for schema changes.             
+6/-2     
Enhancement
9 files
index.ts
Refactored Secret schema to include nested structures.     
+34/-49 
index.ts
Refactored Variable schema to include nested structures. 
+26/-41 
variable.service.ts
Refactored Variable service to use new schema structure. 
+17/-8   
secret.service.ts
Refactored Secret service to use new schema structure.     
+15/-9   
secret-response.ts
Added helper for Secret response transformation.                 
+26/-0   
variable-response.ts
Added helper for Variable response transformation.             
+26/-0   
create.secret.ts
Updated CLI Secret creation to use new schema.                     
+5/-3     
create.variable.ts
Updated CLI Variable creation to use new schema.                 
+5/-3     
index.ts
Updated Event schema to align with new Secret and Variable structures.
+2/-2     
Bug fix
2 files
workspace.spec.ts
Fixed workspace invitation tests for pagination issues.   
+1/-5     
workspace.ts
Fixed workspace invitation client pagination logic.           
+1/-1     
Additional files
2 files
setup.ts +1/-1     
pnpm-lock.yaml +816/-409

💡 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: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Race Condition

Adding a 2 second sleep before running migrations could mask potential race conditions. Consider implementing proper database connection handling and readiness checks instead.

await executeCommand('cd ../.. && sleep 2 && pnpm db:deploy-migrations', {
Debug Code

Console.log statement left in test code that should be removed before merging to production.

console.log(result.error)

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Fix incorrect array length assertion on an object type

The test assertion expect(invitations.metadata).toHaveLength(0) is incorrect as
metadata is likely an object, not an array. Consider testing specific metadata
properties instead.

packages/api-client/tests/workspace.spec.ts [50-51]

 expect(invitations.items).toHaveLength(0)
-expect(invitations.metadata).toHaveLength(0)
+expect(Object.keys(invitations.metadata)).toBeDefined()
  • Apply this suggestion
Suggestion importance[1-10]: 9

Why: The suggestion correctly identifies a critical bug where the test attempts to use array method toHaveLength() on metadata which is an object, as evident from the removed code showing metadata had properties like totalCount and links.

9
Ensure function parameters match the expected signature to prevent runtime errors

The parsePaginationUrl function is called with request parameter but the old version
used headers. Verify that the function signature matches this change to avoid
potential runtime errors.

packages/api-client/src/controllers/workspace.ts [119]

-const url = parsePaginationUrl('/api/workspace/invitations', request)
+const url = parsePaginationUrl('/api/workspace/invitations', request, headers)
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion identifies a potential bug where the function parameters have changed (headers to request) without verifying the function signature, which could lead to runtime errors.

8
General
Add validation tests for new required numeric field

Add test cases to verify that invalid values for version field are properly
rejected, since it's a new required field.

packages/schema/tests/variable.spec.ts [36]

 +            version: 1,
++    it('should reject invalid version values', () => {
++      expect(VariableSchema.safeParse({
++        variable: { /* valid variable data */ },
++        values: [{ version: -1 /* other valid fields */ }]
++      }).success).toBe(false);
++    });
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Adding validation tests for the new required 'version' field is important for ensuring data integrity and catching potential issues early, especially since this is a schema change.

8
Remove debug logging statement from test code

Remove the debug console.log(result.error) statement as it's not needed in the test
and could pollute test output.

packages/schema/tests/variable.spec.ts [45]

-+      console.log(result.error)
 
+
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: Debug logging statements should not be included in test code as they can clutter test output and make it harder to identify actual test failures.

7
Replace fixed sleep with proper service readiness check

Adding a fixed sleep duration is not a reliable way to wait for services to be
ready. Consider implementing a proper health check mechanism instead.

packages/api-client/tests/config/setup.ts [7]

-await executeCommand('cd ../.. && sleep 2 && pnpm db:deploy-migrations', {
+await executeCommand('cd ../.. && pnpm wait-for-db && pnpm db:deploy-migrations', {
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion addresses a potential reliability issue in the test setup. Using a fixed sleep duration can lead to flaky tests, and implementing a proper health check would make the tests more robust.

7

Copy link
Member

Choose a reason for hiding this comment

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

Build is failing as of now, so comment out this step

})
)
})

export const CreateSecretRequestSchema = z.object({
projectSlug: z.string(),
name: SecretSchema.shape.name,
name: z.string(),
Copy link
Member

Choose a reason for hiding this comment

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

I'm dropping this comment for just Create Secret, but this wouldd be applicable to all the name fields of create and update schemas of all the entities.

We would need to check for this:

  • For secrets and variables, charset can only contain A-Z,a-z.0-9,-,_
  • For others, charset will ALSO contain a whtespace character
  • The name must be atleast one character long
  • The name must have atleast one character from A-Z or a-z

@muntaxir4
Copy link
Contributor Author

should I start working on the changes? @rajdip-b

@rajdip-b
Copy link
Member

Hey, no you can drop this PR. I have merged the changes into develop in another branch.

@rajdip-b rajdip-b closed this Jan 15, 2025
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