authorization resolver #4798
authorization resolver #4798
Conversation
WalkthroughThis pull request introduces a new system for querying user privileges through GraphQL. It adds a new TypeScript class Changes
Possibly Related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/services/api/lookup/lookup.resolver.my.privileges.fields.ts (2)
44-74: Constructor has too many dependencies.This large constructor with many injected services can become difficult to maintain and test. Consider refactoring into smaller services or employing a dedicated facade to reduce complexity and improve readability.
92-427: Consider abstracting repetitive field resolvers.Multiple resolvers follow the same pattern of retrieving the entity with authorization relations and returning privileges. Extracting a generic method or leveraging a map-based approach could eliminate duplication and reduce boilerplate.
src/services/api/lookup/dto/lookup.query.my.privileges.results.ts (1)
4-17: Consider adding documentation for each field.Though the fields are self-explanatory, adding short JSDoc or GraphQL field descriptions would clarify how each privilege array is intended to be used. This can reduce ambiguity for clients and future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/services/api/lookup/dto/lookup.query.my.privileges.results.ts(1 hunks)src/services/api/lookup/lookup.module.ts(2 hunks)src/services/api/lookup/lookup.resolver.fields.ts(2 hunks)src/services/api/lookup/lookup.resolver.my.privileges.fields.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/services/api/lookup/dto/lookup.query.my.privileges.results.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/services/api/lookup/lookup.module.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/services/api/lookup/lookup.resolver.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
src/services/api/lookup/lookup.resolver.my.privileges.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
🔇 Additional comments (4)
src/services/api/lookup/lookup.module.ts (2)
36-36: Import statement for LookupMyPrivilegesResolverFields looks good.
No issues detected. It's properly aligned with the new provider usage.
72-77: Addition of LookupMyPrivilegesResolverFields to providers is consistent.
The new resolver is correctly added to the module’s providers array, ensuring it is available at runtime.
src/services/api/lookup/lookup.resolver.fields.ts (2)
70-70: Import of LookupMyPrivilegesQueryResults is well-defined.
No issues detected with the import statement.
149-156: Method myPrivileges is returning an empty object.
Currently, this resolver method returns an empty object cast to LookupMyPrivilegesQueryResults. Ensure correct data integration by either delegating to the new LookupMyPrivilegesResolverFields or implementing the privilege retrieval logic directly.
| if (!authorizable.authorization) { | ||
| throw new RelationshipNotFoundException( | ||
| `Unable to load Authorization for ${authorizable.constructor.name} with ID ${authorizable.id}`, | ||
| LogContext.API | ||
| ); | ||
| } |
There was a problem hiding this comment.
Potential mismatch between the GraphQL return type annotation and actual return value.
The method is decorated with @ResolveField(() => IAuthorizationPolicy) but returns Promise<AuthorizationPrivilege[]>. This could lead to schema inconsistencies or confusion for clients expecting an object rather than an array of privileges.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/api/lookup/lookup.resolver.my.privileges.fields.ts (1)
91-105: Resolver method for 'space' is consistent but note repeated patterns.
The logic is correct for retrieving the entity and returning privileges. However, the same approach is repeated across many resolvers below, leading to duplication. Consider a generic or reusable approach to reduce boilerplate code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/api/lookup/lookup.resolver.my.privileges.fields.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/api/lookup/lookup.resolver.my.privileges.fields.ts (1)
Pattern src/**/*.{ts,js}: Review the TypeScript/JavaScript code for NestJS best practices, dependency injection, module structure, and potential bugs.
Context Files (Do Not Review):
docs/Design.md- Design overview of the projectdocs/Pagination.md- Pagination design overviewdocs/Developing.md- Development setup overviewdocs/graphql-typeorm-usage.md- overview of GraphQL and TypeORM usage and how they are used together with NestJS in the projectdocs/database-definitions.md- guidelines for creating TypeORM entity defnitionssrc/core/error-handling/graphql.exception.filter.ts- GraphQL error handlingsrc/core/error-handling/http.exception.filter.ts- HTTP error handlingsrc/core/error-handling/rest.error.response.ts- REST error responsesrc/core/error-handling/unhandled.exception.filter.ts- Global exception handler
Guidelines:
- Our project uses global exception handlers (
UnhandledExceptionFilter), so avoid suggesting additionaltry/catchblocks unless handling specific cases. - Use NestJS latest documentation from
https://docs.nestjs.com/for reference on NestJS best practices. - Use TypeORM latest documentation from
https://typeorm.io/for reference on TypeORM best practices. - Refer to the design overview in the context files for better understanding.
🔇 Additional comments (3)
src/services/api/lookup/lookup.resolver.my.privileges.fields.ts (3)
75-89: Exceptional handling strategy is well-defined, but confirm partial loads aren't needed.
You are throwing a RelationshipNotFoundException if the authorization relation is missing. This is acceptable since a missing relation indicates a broken use case. Just confirm you do not require a scenario to return partial data when the relation is absent.
79-84: Past Mismatch Issue No Longer Present
A previous review flagged a mismatch between the decorator return type and the actual return value. In the current version, the return type is an array of AuthorizationPrivilege while the decorator is set to [AuthorizationPrivilege], so this mismatch is resolved.
1-40: Ensure consistent import usage and remove any unused imports if necessary.
Although these service imports seem relevant, confirm that no import is unused or can be omitted. Maintaining a clean import list supports code clarity and prevents dead dependencies.
✅ Verification successful
All imported services are actively used in the resolver class
After reviewing the complete file content, all imported services are properly injected in the constructor and used in corresponding resolver methods. Each service is used to fetch entities and check privileges. There are no unused imports in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if all imported services are used within this file.
# Utilizing 'rg' to search for references of these services.
rg "new (CommunityService|CollaborationService|ContextService|ProfileService|PostService|CalloutService|InnovationFlowService|RoomService|CalendarEventService|CalendarService|ApplicationService|InvitationService|WhiteboardService|DocumentService|StorageAggregatorService|UserService|SpaceService|CommunityGuidelinesService|VirtualContributorService|StorageBucketService|InnovationHubService|InnovationPackService|AccountService|TemplateService|TemplatesSetService|TemplatesManagerService|RoleSetService|LicenseService)\(" -A 5
Length of output: 528
Script:
#!/bin/bash
# Let's try a different approach to find service usage
# First, let's get the full content of the file to see the class implementation
cat src/services/api/lookup/lookup.resolver.my.privileges.fields.ts
# As a backup, let's also search for any usage of these services without the "new" keyword
rg -l "CommunityService|CollaborationService|ContextService|ProfileService|PostService|CalloutService|InnovationFlowService|RoomService|CalendarEventService|CalendarService|ApplicationService|InvitationService|WhiteboardService|DocumentService|StorageAggregatorService|UserService|SpaceService|CommunityGuidelinesService|VirtualContributorService|StorageBucketService|InnovationHubService|InnovationPackService|AccountService|TemplateService|TemplatesSetService|TemplatesManagerService|RoleSetService|LicenseService" src/services/api/lookup/lookup.resolver.my.privileges.fields.ts
Length of output: 19983
| relations: { authorization: true }, | ||
| }); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, room); | ||
| } | ||
|
|
||
| @ResolveField(() => [AuthorizationPrivilege], { | ||
| nullable: true, | ||
| description: 'Lookup myPrivileges on the specified InnovationFlow', | ||
| }) | ||
| async innovationFlow( | ||
| @CurrentUser() agentInfo: AgentInfo, | ||
| @Args('ID', { type: () => UUID }) id: string | ||
| ): Promise<AuthorizationPrivilege[]> { | ||
| const innovationFlow = | ||
| await this.innovationFlowService.getInnovationFlowOrFail(id, { | ||
| relations: { authorization: true }, | ||
| }); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, innovationFlow); | ||
| } | ||
|
|
||
| @ResolveField(() => [AuthorizationPrivilege], { | ||
| nullable: true, | ||
| description: 'Lookup myPrivileges on the specified Template', | ||
| }) | ||
| async template( | ||
| @CurrentUser() agentInfo: AgentInfo, | ||
| @Args('ID', { type: () => UUID }) id: string | ||
| ): Promise<AuthorizationPrivilege[]> { | ||
| const template = await this.templateService.getTemplateOrFail(id, { | ||
| relations: { authorization: true }, | ||
| }); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, template); | ||
| } | ||
|
|
||
| @ResolveField(() => [AuthorizationPrivilege], { | ||
| nullable: true, | ||
| description: 'Lookup myPrivileges on the specified TemplatesSet', | ||
| }) | ||
| async templatesSet( | ||
| @CurrentUser() agentInfo: AgentInfo, | ||
| @Args('ID', { type: () => UUID }) id: string | ||
| ): Promise<AuthorizationPrivilege[]> { | ||
| const templatesSet = await this.templatesSetService.getTemplatesSetOrFail( | ||
| id, | ||
| { relations: { authorization: true } } | ||
| ); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, templatesSet); | ||
| } | ||
|
|
||
| @ResolveField(() => [AuthorizationPrivilege], { | ||
| nullable: true, | ||
| description: 'Lookup myPrivileges on the specified TemplatesManager', | ||
| }) | ||
| async templatesManager( | ||
| @CurrentUser() agentInfo: AgentInfo, | ||
| @Args('ID', { type: () => UUID }) id: string | ||
| ): Promise<AuthorizationPrivilege[]> { | ||
| const templatesManager = | ||
| await this.templatesManagerService.getTemplatesManagerOrFail(id, { | ||
| relations: { authorization: true }, | ||
| }); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, templatesManager); | ||
| } | ||
|
|
||
| @ResolveField(() => [AuthorizationPrivilege], { | ||
| nullable: true, | ||
| description: 'Lookup myPrivileges on the specified Community guidelines', | ||
| }) | ||
| async communityGuidelines( | ||
| @CurrentUser() agentInfo: AgentInfo, | ||
| @Args('ID', { type: () => UUID }) id: string | ||
| ): Promise<AuthorizationPrivilege[]> { | ||
| const guidelines = | ||
| await this.guidelinesService.getCommunityGuidelinesOrFail(id, { | ||
| relations: { authorization: true }, | ||
| }); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, guidelines); | ||
| } | ||
|
|
||
| @ResolveField(() => [AuthorizationPrivilege], { | ||
| nullable: true, | ||
| description: 'Lookup myPrivileges on the specified License', | ||
| }) | ||
| async license( | ||
| @CurrentUser() agentInfo: AgentInfo, | ||
| @Args('ID', { type: () => UUID }) id: string | ||
| ): Promise<AuthorizationPrivilege[]> { | ||
| const license = await this.licenseService.getLicenseOrFail(id, { | ||
| relations: { authorization: true }, | ||
| }); | ||
|
|
||
| return this.getMyPrivilegesOnAuthorizable(agentInfo, license); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce duplication across multiple resolver methods.
All the entity-specific resolvers follow the same pattern of:
- Fetching the entity with
{ relations: { authorization: true } } - Returning
getMyPrivilegesOnAuthorizable(agentInfo, entity).
Consider implementing a higher-order function or a generic approach that reuses this pattern, accepting the relevant service method and entity ID. This would consolidate logic, improve readability, and facilitate easier maintenance.
- async space(@CurrentUser() agentInfo: AgentInfo, @Args('ID') id: string) {
- const space = await this.spaceService.getSpaceOrFail(id, { relations: { authorization: true } });
- return this.getMyPrivilegesOnAuthorizable(agentInfo, space);
- }
- ...
+ private async fetchPrivileges<T extends IAuthorizable>(
+ serviceMethod: (id: string, options?: any) => Promise<T>,
+ agentInfo: AgentInfo,
+ id: string,
+ ): Promise<AuthorizationPrivilege[]> {
+ const entity = await serviceMethod(id, { relations: { authorization: true } });
+ return this.getMyPrivilegesOnAuthorizable(agentInfo, entity);
+ }
+
+ @ResolveField(() => [AuthorizationPrivilege], { nullable: true })
+ async space(@CurrentUser() agentInfo: AgentInfo, @Args('ID') id: string) {
+ return this.fetchPrivileges(this.spaceService.getSpaceOrFail.bind(this.spaceService), agentInfo, id);
+ }
+ // ... apply pattern to other resolversCommittable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes