Created at test #3460
Created at test #3460
Conversation
…awa-api into develop-postgres
…ation/talawa-api into develop-postgres
…awa-api into develop-postgres
WalkthroughThe changes extract the resolver logic for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLServer
participant createdAtResolver
participant Database
Client->>GraphQLServer: Query Fund.createdAt
GraphQLServer->>createdAtResolver: Call with Fund, args, ctx
createdAtResolver->>createdAtResolver: Check authentication
alt Not authenticated
createdAtResolver-->>GraphQLServer: Throw "unauthenticated" error
GraphQLServer-->>Client: Error response
else Authenticated
createdAtResolver->>Database: Fetch user and membership roles
alt User not found
createdAtResolver-->>GraphQLServer: Throw "unauthenticated" error
GraphQLServer-->>Client: Error response
else Not admin
createdAtResolver-->>GraphQLServer: Throw "unauthorized_action" error
GraphQLServer-->>Client: Error response
else Authorized
createdAtResolver-->>GraphQLServer: Return createdAt date
GraphQLServer-->>Client: createdAt date
end
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/graphql/types/Fund/createdAt.ts(1 hunks)src/utilities/graphqLimits.ts(1 hunks)test/graphql/types/Fund/createdAt.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/graphql/types/Fund/createdAt.ts (2)
src/graphql/context.ts (1)
GraphQLContext(55-55)src/utilities/TalawaGraphQLError.ts (1)
TalawaGraphQLError(264-277)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (8)
src/graphql/types/Fund/createdAt.ts (2)
7-8: LGTM! Great use of environment configuration for complexity.The extraction of the complexity value as a named constant improves code readability and maintainability.
66-72: LGTM! Effective use of extracted components.The implementation nicely leverages the extracted resolver function and complexity constant, making the code more maintainable and easier to test independently.
test/graphql/types/Fund/createdAt.test.ts (6)
1-50: Well-structured test setup with comprehensive mocking.The test file has a clear organization with proper mocking of the GraphQL context and fund object. Good use of TypeScript types for mock data and proper isolation of tests using beforeEach.
51-65: Good test coverage for schema complexity validation.This test ensures that the complexity value is correctly applied in the GraphQL schema, which is important for preventing abuse through overly complex queries.
67-137: Excellent authentication and authorization test coverage.Comprehensive tests covering all access control scenarios:
- Unauthenticated users
- Non-admin users without organization membership
- Non-admin users with member-level organization membership
- System administrators
- Organization administrators
This ensures the resolver properly enforces the security boundaries.
139-155: LGTM! Proper verification of data retrieval.The test confirms that the resolver correctly returns the fund's createdAt value when accessed by authorized users.
157-185: Thorough error handling test coverage.Good tests for different types of database errors, ensuring they're properly caught, logged, and converted to appropriate GraphQL errors.
187-255: Excellent concurrent access testing.The tests thoroughly validate the database query filters for both user ID and organization ID, ensuring that the resolver uses the correct filters even under concurrent access scenarios. Also includes proper error handling tests for database failures during concurrent access.
| @@ -9,6 +9,7 @@ const schema = Type.Pick(envConfigSchema, [ | |||
| "API_GRAPHQL_LIST_FIELD_COST", | |||
| "API_GRAPHQL_NON_PAGINATED_LIST_FIELD_COST", | |||
| "API_GRAPHQL_MUTATION_BASE_COST", | |||
| "API_GRAPHQL_SCALAR_RESOLVER_FIELD_COST", | |||
There was a problem hiding this comment.
Remove duplicate environment variable key.
The environment variable key "API_GRAPHQL_SCALAR_RESOLVER_FIELD_COST" is already included at line 7, making this entry redundant. Duplicating keys in the schema can lead to confusion and potential maintenance issues.
- "API_GRAPHQL_SCALAR_RESOLVER_FIELD_COST",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "API_GRAPHQL_SCALAR_RESOLVER_FIELD_COST", |
| export const createdAtResolver = async ( | ||
| parent: FundType, | ||
| _args: Record<string, never>, | ||
| ctx: GraphQLContext, | ||
| ) => { | ||
| if (!ctx.currentClient.isAuthenticated) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { code: "unauthenticated" }, | ||
| }); | ||
| } | ||
|
|
||
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | ||
| columns: { | ||
| role: true, | ||
| }, | ||
| with: { | ||
| organizationMembershipsWhereMember: { | ||
| columns: { | ||
| role: true, | ||
| }, | ||
| where: (fields, operators) => | ||
| operators.eq(fields.organizationId, parent.organizationId), | ||
| }, | ||
| }, | ||
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | ||
| }); | ||
| try { | ||
| const currentUserId = ctx.currentClient.user.id; | ||
|
|
||
| if (currentUser === undefined) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthenticated", | ||
| }, | ||
| }); | ||
| } | ||
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | ||
| columns: { role: true }, | ||
| with: { | ||
| organizationMembershipsWhereMember: { | ||
| columns: { role: true }, | ||
| where: (fields, operators) => | ||
| operators.eq(fields.organizationId, parent.organizationId), | ||
| }, | ||
| }, | ||
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | ||
| }); | ||
|
|
||
| const currentUserOrganizationMembership = | ||
| currentUser.organizationMembershipsWhereMember[0]; | ||
| if (currentUser === undefined) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { code: "unauthenticated" }, | ||
| }); | ||
| } | ||
|
|
||
| if ( | ||
| currentUser.role !== "administrator" && | ||
| (currentUserOrganizationMembership === undefined || | ||
| currentUserOrganizationMembership.role !== "administrator") | ||
| ) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { | ||
| code: "unauthorized_action", | ||
| }, | ||
| }); | ||
| } | ||
| const currentUserOrganizationMembership = | ||
| currentUser.organizationMembershipsWhereMember[0]; | ||
|
|
||
| return parent.createdAt; | ||
| }, | ||
| if ( | ||
| currentUser.role !== "administrator" && | ||
| (!currentUserOrganizationMembership || | ||
| currentUserOrganizationMembership.role !== "administrator") | ||
| ) { | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { code: "unauthorized_action" }, | ||
| }); | ||
| } | ||
|
|
||
| return parent.createdAt; | ||
| } catch (error) { | ||
| ctx.log.error("An unexpected error occurred in createdAtResolver", { | ||
| error, | ||
| }); | ||
| throw new TalawaGraphQLError({ | ||
| extensions: { code: "unexpected" }, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Excellent implementation of the resolver function with proper access control.
The resolver logic has been nicely extracted into a standalone function with comprehensive authentication and authorization checks. The error handling is robust, using try-catch to capture unexpected errors.
However, there appears to be some redundancy in the authentication checks:
- First check at lines 15-19
- Second check at lines 36-40 after fetching the user
Consider consolidating these checks for cleaner code flow.
export const createdAtResolver = async (
parent: FundType,
_args: Record<string, never>,
ctx: GraphQLContext,
) => {
if (!ctx.currentClient.isAuthenticated) {
throw new TalawaGraphQLError({
extensions: { code: "unauthenticated" },
});
}
try {
const currentUserId = ctx.currentClient.user.id;
const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({
columns: { role: true },
with: {
organizationMembershipsWhereMember: {
columns: { role: true },
where: (fields, operators) =>
operators.eq(fields.organizationId, parent.organizationId),
},
},
where: (fields, operators) => operators.eq(fields.id, currentUserId),
});
- if (currentUser === undefined) {
- throw new TalawaGraphQLError({
- extensions: { code: "unauthenticated" },
- });
- }
+ if (currentUser === undefined) {
+ ctx.log.warn(`User ${currentUserId} not found in database despite being authenticated`);
+ throw new TalawaGraphQLError({
+ extensions: { code: "unauthenticated" },
+ });
+ }
const currentUserOrganizationMembership =
currentUser.organizationMembershipsWhereMember[0];
if (
currentUser.role !== "administrator" &&
(!currentUserOrganizationMembership ||
currentUserOrganizationMembership.role !== "administrator")
) {
throw new TalawaGraphQLError({
extensions: { code: "unauthorized_action" },
});
}
return parent.createdAt;
} catch (error) {
- ctx.log.error("An unexpected error occurred in createdAtResolver", {
+ ctx.log.error(`An unexpected error occurred in createdAtResolver for fund ${parent.id}`, {
error,
});
throw new TalawaGraphQLError({
extensions: { code: "unexpected" },
});
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createdAtResolver = async ( | |
| parent: FundType, | |
| _args: Record<string, never>, | |
| ctx: GraphQLContext, | |
| ) => { | |
| if (!ctx.currentClient.isAuthenticated) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unauthenticated" }, | |
| }); | |
| } | |
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | |
| columns: { | |
| role: true, | |
| }, | |
| with: { | |
| organizationMembershipsWhereMember: { | |
| columns: { | |
| role: true, | |
| }, | |
| where: (fields, operators) => | |
| operators.eq(fields.organizationId, parent.organizationId), | |
| }, | |
| }, | |
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | |
| }); | |
| try { | |
| const currentUserId = ctx.currentClient.user.id; | |
| if (currentUser === undefined) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { | |
| code: "unauthenticated", | |
| }, | |
| }); | |
| } | |
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | |
| columns: { role: true }, | |
| with: { | |
| organizationMembershipsWhereMember: { | |
| columns: { role: true }, | |
| where: (fields, operators) => | |
| operators.eq(fields.organizationId, parent.organizationId), | |
| }, | |
| }, | |
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | |
| }); | |
| const currentUserOrganizationMembership = | |
| currentUser.organizationMembershipsWhereMember[0]; | |
| if (currentUser === undefined) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unauthenticated" }, | |
| }); | |
| } | |
| if ( | |
| currentUser.role !== "administrator" && | |
| (currentUserOrganizationMembership === undefined || | |
| currentUserOrganizationMembership.role !== "administrator") | |
| ) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { | |
| code: "unauthorized_action", | |
| }, | |
| }); | |
| } | |
| const currentUserOrganizationMembership = | |
| currentUser.organizationMembershipsWhereMember[0]; | |
| return parent.createdAt; | |
| }, | |
| if ( | |
| currentUser.role !== "administrator" && | |
| (!currentUserOrganizationMembership || | |
| currentUserOrganizationMembership.role !== "administrator") | |
| ) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unauthorized_action" }, | |
| }); | |
| } | |
| return parent.createdAt; | |
| } catch (error) { | |
| ctx.log.error("An unexpected error occurred in createdAtResolver", { | |
| error, | |
| }); | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unexpected" }, | |
| }); | |
| } | |
| }; | |
| export const createdAtResolver = async ( | |
| parent: FundType, | |
| _args: Record<string, never>, | |
| ctx: GraphQLContext, | |
| ) => { | |
| if (!ctx.currentClient.isAuthenticated) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unauthenticated" }, | |
| }); | |
| } | |
| try { | |
| const currentUserId = ctx.currentClient.user.id; | |
| const currentUser = await ctx.drizzleClient.query.usersTable.findFirst({ | |
| columns: { role: true }, | |
| with: { | |
| organizationMembershipsWhereMember: { | |
| columns: { role: true }, | |
| where: (fields, operators) => | |
| operators.eq(fields.organizationId, parent.organizationId), | |
| }, | |
| }, | |
| where: (fields, operators) => operators.eq(fields.id, currentUserId), | |
| }); | |
| if (currentUser === undefined) { | |
| ctx.log.warn(`User ${currentUserId} not found in database despite being authenticated`); | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unauthenticated" }, | |
| }); | |
| } | |
| const currentUserOrganizationMembership = | |
| currentUser.organizationMembershipsWhereMember[0]; | |
| if ( | |
| currentUser.role !== "administrator" && | |
| (!currentUserOrganizationMembership || | |
| currentUserOrganizationMembership.role !== "administrator") | |
| ) { | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unauthorized_action" }, | |
| }); | |
| } | |
| return parent.createdAt; | |
| } catch (error) { | |
| ctx.log.error( | |
| `An unexpected error occurred in createdAtResolver for fund ${parent.id}`, | |
| { error } | |
| ); | |
| throw new TalawaGraphQLError({ | |
| extensions: { code: "unexpected" }, | |
| }); | |
| } | |
| }; |
Summary by CodeRabbit