Conversion of VC type; transfer of callout to another calloutsSet #4914
Conversation
…lloutsSet, with storage updated
WalkthroughThis update introduces a callout transfer feature with new modules, resolvers, and services to support transferring callouts between callout sets. Spell-check settings were also modified, while the credentials and authorization flow have been enhanced with new constants and methods. Several AI persona service methods were updated, including new update functionality and the removal of an event bus dependency. In addition, the conversion module now supports converting virtual contributors by integrating callout transfer and virtual contributor management logic. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as CalloutTransferResolver
participant CS as CalloutsSetService
participant A as AuthorizationService
participant CTS as CalloutTransferService
C->>R: transferCallout(transferData)
R->>CS: getCalloutOrFail(transferData.calloutID)
CS-->>R: callout data
R->>A: grantAccessOrFail(source & target)
A-->>R: authorization confirmed
R->>CTS: transferCallout(callout, targetSet)
CTS->>CTS: update storage aggregator & tagsets
CTS-->>R: transferred callout
R->>R: reset authorization policy
R-->>C: return updated callout
sequenceDiagram
participant C as Client
participant CR as ConversionResolver
participant VC as VirtualContributorService
participant CTS as CalloutTransferService
C->>CR: convertVirtualContributorToUseKnowledgeBase(conversionData)
CR->>VC: fetch VirtualContributor details
VC-->>CR: virtual contributor data
CR->>CTS: transfer each callout from source to target
CTS-->>CR: updated callout(s)
CR->>CR: update authorization policy and notify changes
CR-->>C: return updated Virtual Contributor
Suggested reviewers
✨ Finishing Touches
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: 3
🧹 Nitpick comments (9)
src/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.ts (2)
22-33: Validate or Roll Back Partial Changes When Transfer Fails
The constructor injects multiple services but doesn't handle partial rollbacks if the transfer fails midway. Consider whether wrapping related calls in a transaction or explicitly managing partial failures is necessary to ensure data consistency.
81-97: Potential Missing Transaction Handling
While the code successfully calls transferCallout and resets policies, if an error arises after the callout is already moved, partial updates might remain. Consider a transaction-based approach for consistent rollback.src/domain/collaboration/callout-transfer/callout.transfer.service.ts (1)
50-62: Leverage Transactional Consistency
Multiple related updates (callout save, aggregator update, tagsets update) occur in sequence. If any step fails, the state may be partially updated. Investigate using a transaction if atomicity is desired.src/services/api/conversion/conversion.resolver.mutations.ts (2)
19-22: Include Detailed Logging for Conversion
Adding more logging around RelationshipNotFoundException or ValidationException can speed up debugging by clarifying conversion steps prior to the exception.
140-272: Convert VC to KnowledgeBase with Transfer
The new mutation properly checks entity relationships, ensures user privileges, and transfers each callout in a loop. For large sets of callouts, consider restricting batch sizes or using streaming patterns to avoid potential performance bottlenecks.src/domain/collaboration/callouts-set/callouts.set.service.ts (1)
472-609: Consider optimizing the filtering logic in getCalloutsFromCollaboration.The method contains complex filtering logic with multiple conditions. Consider extracting the filtering logic into separate methods for better maintainability.
+ private filterCalloutsByGroups(callout: ICallout, groupNames: string[]): boolean { + if (groupNames.length === 0) return true; + return callout.framing.profile.tagsets?.some( + tagset => + tagset.name === TagsetReservedName.CALLOUT_GROUP && + tagset.tags.length > 0 && + groupNames?.includes(tagset.tags[0]) + ) ?? false; + } + + private filterCalloutsByTagsets(callout: ICallout, tagsets?: { name: string; tags: string[] }[]): boolean { + if (!tagsets?.length) return true; + return callout.framing.profile?.tagsets?.some(calloutTagset => + tagsets.some( + argTagset => + argTagset.name === calloutTagset.name && + argTagset.tags.some(argTag => calloutTagset.tags.includes(argTag)) + ) + ) ?? false; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 498-498: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 538-538: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/services/adapters/ai-server-adapter/dto/ai.server.adapter.dto.update.ai.persona.service.ts (1)
8-12: Consider adding documentation for enum values.While the enum values are self-explanatory, adding JSDoc comments would improve code maintainability.
export enum InvocationResultAction { + /** Post a reply in the conversation */ POST_REPLY = 'postReply', + /** Post a new message in the conversation */ POST_MESSAGE = 'postMessage', + /** No action needed */ NONE = 'none', }src/services/adapters/ai-server-adapter/ai.server.adapter.ts (1)
67-69: Add error handling for the update operation.While the implementation is correct, consider adding error handling and logging similar to other methods in the class.
async updateAiPersonaService(updateData: UpdateAiPersonaServiceInput) { + this.logger.verbose?.( + `Updating AI Persona service ${updateData.ID}`, + LogContext.AI_SERVER_ADAPTER + ); return this.aiServer.updateAiPersonaService(updateData); }src/services/ai-server/ai-persona-service/ai.persona.service.service.ts (1)
99-108: Consider using object spread for cleaner updates.The field-by-field update can be simplified using object spread operator.
- if (aiPersonaServiceData.bodyOfKnowledgeID) { - aiPersonaService.bodyOfKnowledgeID = - aiPersonaServiceData.bodyOfKnowledgeID; - } - - if (aiPersonaServiceData.bodyOfKnowledgeType) { - aiPersonaService.bodyOfKnowledgeType = - aiPersonaServiceData.bodyOfKnowledgeType; - } + Object.assign(aiPersonaService, { + bodyOfKnowledgeID: aiPersonaServiceData.bodyOfKnowledgeID ?? aiPersonaService.bodyOfKnowledgeID, + bodyOfKnowledgeType: aiPersonaServiceData.bodyOfKnowledgeType ?? aiPersonaService.bodyOfKnowledgeType, + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.vscode/settings.json(1 hunks)src/app.module.ts(2 hunks)src/common/constants/authorization/credential.rule.types.constants.ts(1 hunks)src/domain/collaboration/callout-transfer/callout.transfer.module.ts(1 hunks)src/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.ts(1 hunks)src/domain/collaboration/callout-transfer/callout.transfer.service.ts(1 hunks)src/domain/collaboration/callout-transfer/dto/callouts.set.dto.transfer.callout.ts(1 hunks)src/domain/collaboration/callouts-set/callouts.set.resolver.mutations.ts(0 hunks)src/domain/collaboration/callouts-set/callouts.set.service.authorization.ts(3 hunks)src/domain/collaboration/callouts-set/callouts.set.service.ts(3 hunks)src/domain/common/tagset/tagset.interface.ts(2 hunks)src/services/adapters/ai-server-adapter/ai.server.adapter.ts(2 hunks)src/services/adapters/ai-server-adapter/dto/ai.server.adapter.dto.update.ai.persona.service.ts(1 hunks)src/services/ai-server/ai-persona-service/ai.persona.service.service.ts(1 hunks)src/services/ai-server/ai-persona-service/dto/ai.persona.service.dto.update.ts(2 hunks)src/services/ai-server/ai-server/ai.server.service.ts(2 hunks)src/services/api/conversion/conversion.module.ts(2 hunks)src/services/api/conversion/conversion.resolver.mutations.ts(3 hunks)src/services/api/conversion/dto/conversion.dto.vc.space.to.vc.kb.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/domain/collaboration/callouts-set/callouts.set.resolver.mutations.ts
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
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 handlerGuidelines:
- 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/app.module.tssrc/domain/common/tagset/tagset.interface.tssrc/services/api/conversion/conversion.module.tssrc/services/adapters/ai-server-adapter/ai.server.adapter.tssrc/common/constants/authorization/credential.rule.types.constants.tssrc/services/api/conversion/dto/conversion.dto.vc.space.to.vc.kb.tssrc/domain/collaboration/callout-transfer/dto/callouts.set.dto.transfer.callout.tssrc/domain/collaboration/callout-transfer/callout.transfer.module.tssrc/services/ai-server/ai-server/ai.server.service.tssrc/services/ai-server/ai-persona-service/ai.persona.service.service.tssrc/services/ai-server/ai-persona-service/dto/ai.persona.service.dto.update.tssrc/domain/collaboration/callouts-set/callouts.set.service.authorization.tssrc/services/api/conversion/conversion.resolver.mutations.tssrc/services/adapters/ai-server-adapter/dto/ai.server.adapter.dto.update.ai.persona.service.tssrc/domain/collaboration/callout-transfer/callout.transfer.service.tssrc/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.tssrc/domain/collaboration/callouts-set/callouts.set.service.ts
🔇 Additional comments (27)
src/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.ts (4)
34-43: Ensure Return Type Matches GraphQL Schema
Returning ICallout is correct for a TypeScript interface, but verify that the corresponding GraphQL type also expects the same structure. Mismatched or incomplete fields can lead to runtime issues at the GraphQL boundary.
44-58: Check for Non-Null Callouts Set
Good job verifying the presence of callout.calloutsSet before proceeding. This check prevents potential null reference errors.
59-64: Block Template Transfer
Preventing template callouts from transfer helps maintain constraints. This is a sound validation step that avoids unexpected state mutations.
65-80: Comprehensive Source & Target Authorization
Granting access for both the source and target callouts set ensures robust authorization. This two-level check is essential to prevent unauthorized inter-set transfers.src/domain/collaboration/callout-transfer/callout.transfer.service.ts (6)
20-30: Adhere to NestJS Dependency Injection Best Practices
This constructor properly injects multiple domain services, promoting clean architecture and testability.
31-39: Validate Name Collision
Ensuring a unique callout.nameID within the target set is crucial to avoid duplicates and maintain data integrity across callouts sets. Good check here.
41-49: Confirm Storage Aggregator Availability
Retrieving the storage aggregator for the target set before updating the callout is logical. However, if the aggregator or the retrieval fails, consider capturing the error for diagnostic logging.
64-128: Ensure Contributions Are Properly Initialized
Throwing an EntityNotInitializedException when callout.contributions are missing prevents undefined references later. This approach is good defensive programming.
130-138: Handle Missing Storage Buckets Gracefully
updateStorageBucketAggregator safely ignores undefined storage buckets, which reduces null-reference surprises.
140-176: Rebuild Non-Default Tagsets
Removing old tagsets (except the default) and creating new ones from templates ensures alignment with target callouts set’s configuration. Keep watch for any needed data migrations for old tagset associations.src/services/api/conversion/conversion.resolver.mutations.ts (2)
24-28: Injecting Virtual Contributor Services
Injecting both VirtualContributorService and VirtualContributorAuthorizationService together is good. It clarifies domain responsibilities and fosters modular design.
42-45: Callout Transfer Service Integration
Introducing the calloutTransferService is valuable for reusing existing logic. Centralizing callout movement reduces duplication across resolvers.src/services/api/conversion/dto/conversion.dto.vc.space.to.vc.kb.ts (1)
1-11: LGTM! Well-structured input type class.The class follows NestJS and GraphQL best practices with proper decorators, scalar types, and documentation.
src/domain/collaboration/callout-transfer/dto/callouts.set.dto.transfer.callout.ts (1)
1-18: LGTM! Well-structured input type class.The class follows NestJS and GraphQL best practices with proper decorators, scalar types, and documentation.
src/domain/common/tagset/tagset.interface.ts (1)
19-19: 🛠️ Refactor suggestionAdd Field decorator for profile property.
The profile property should be decorated with
@Fieldto expose it in the GraphQL schema, similar to other properties in the class.- profile?: IProfile; + @Field(() => IProfile, { nullable: true }) + profile?: IProfile;Likely invalid or redundant comment.
src/domain/collaboration/callout-transfer/callout.transfer.module.ts (1)
1-28: LGTM! Well-structured module.The module follows NestJS best practices with proper imports, providers, and exports.
src/services/api/conversion/conversion.module.ts (1)
12-14: LGTM! Module imports align with PR objectives.The added modules (CalloutTransferModule, AiServerAdapterModule, VirtualContributorModule) provide the necessary dependencies for handling callout transfers and virtual contributor conversions.
Also applies to: 27-29
src/domain/collaboration/callouts-set/callouts.set.service.authorization.ts (2)
55-57: LGTM! Authorization policy correctly updated.The credential rules for callout transfers are properly integrated into the authorization policy.
77-100: LGTM! Well-structured credential rules implementation.The implementation:
- Correctly separates offer and accept privileges
- Properly restricts access to GLOBAL_ADMIN
- Prevents unintended cascading of permissions
src/common/constants/authorization/credential.rule.types.constants.ts (1)
9-12: LGTM! Constants follow naming conventions.The new credential rule type constants for callout transfers are properly defined and follow the established naming pattern.
src/app.module.ts (1)
91-91: LGTM! Module properly integrated.The CalloutTransferModule is correctly imported and added to the AppModule's imports array.
Also applies to: 306-306
src/domain/collaboration/callouts-set/callouts.set.service.ts (2)
355-367: LGTM! Well-structured validation method.The new validation method properly checks for nameID conflicts and provides clear error messages.
326-326: LGTM! Appropriate logging context.The change from SPACES to COLLABORATION better reflects the domain context of the operation.
.vscode/settings.json (1)
22-26: LGTM! Appropriate spell check additions.The additions of "tagset" and "tagsets" to the spell check dictionary align with the codebase's terminology.
src/services/adapters/ai-server-adapter/dto/ai.server.adapter.dto.update.ai.persona.service.ts (1)
3-6: LGTM! Clear and concise enum definition.The
InvocationOperationenum clearly defines the supported operations.src/services/ai-server/ai-persona-service/dto/ai.persona.service.dto.update.ts (1)
21-29: LGTM! Well-structured input type with proper validation.The new fields are properly decorated with GraphQL Field decorators and MaxLength validation.
src/services/ai-server/ai-persona-service/ai.persona.service.service.ts (1)
63-75: Address the race condition in event publishing.The commented-out code indicates a race condition in the authorization setup. This should be properly addressed rather than being disabled.
Would you like me to help investigate the race condition and propose a solution?
src/services/adapters/ai-server-adapter/dto/ai.server.adapter.dto.update.ai.persona.service.ts
Show resolved
Hide resolved
src/services/adapters/ai-server-adapter/dto/ai.server.adapter.dto.update.ai.persona.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.ts (1)
65-74: Additional Transaction Handling Might Be Beneficial
While the logic for obtaining the target callouts set and verifying authorization seems solid, consider wrapping this entire operation in a database transaction, especially if multiple processes might simultaneously transfer callouts. This would avoid partial updates if an error arises mid-operation.src/domain/collaboration/callout-transfer/callout.transfer.service.ts (1)
145-181: Tagset Removal and Creation
The logic to remove all non-default tagsets and then create new ones from the templates is clear. If any new tagsets fail to save, consider whether some partial changes might leave the callout in a confusing state. Using a transaction could help ensure atomic updates.src/services/api/conversion/conversion.resolver.mutations.ts (2)
247-254: Efficiency Consideration for Callout Transfers
Transferring callouts one by one might be time-consuming if there is a large set. Consider a bulk or batched approach for performance and possibly wrap it in a transaction to reduce partial state issues if failures occur mid-transfer.
256-273: AI Persona Update and Authorization Reset
Updating the AI persona service and resetting authorization policies is well sequenced. However, similar to other methods, a transaction or rollback strategy within each step might offer additional safety if any step fails.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.ts(1 hunks)src/domain/collaboration/callout-transfer/callout.transfer.service.ts(1 hunks)src/services/api/conversion/conversion.resolver.mutations.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/**/*.{ts,js}`: Review the TypeScript/JavaScript code fo...
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 handlerGuidelines:
- 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/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.tssrc/services/api/conversion/conversion.resolver.mutations.tssrc/domain/collaboration/callout-transfer/callout.transfer.service.ts
🔇 Additional comments (9)
src/domain/collaboration/callout-transfer/callout.transfer.resolver.mutations.ts (4)
22-32: Constructor Injection Looks Good
The constructor properly injects the required services, adhering to NestJS best practices for dependency injection.
34-38: Clear Mutation Definition
This mutation is clearly defined with a GraphQL guard and description. Good job documenting the mutation's experimental nature and restricted usage for certain roles.
39-58: Validation Checks are Comprehensive
The checks for the callout’s existence, relationship with a callouts set, and the template status are thorough. Throwing specific exceptions helps maintain clarity around relationship errors and invalid states.
81-98: Ensuring Post-Transfer Consistency
The approach of transferring the callout and then resetting the authorization policy is clean and logically separated. However, if policy application fails after the transfer, you might end up with a partially updated state. A transaction or rollback strategy could protect data integrity if the policy update fails.src/domain/collaboration/callout-transfer/callout.transfer.service.ts (2)
23-30: Well-structured Constructor
Services are neatly injected, which aligns with NestJS guidelines. No issues found.
69-133: Potential Edge Case for Empty Contributions
The EntityNotInitializedException is thrown if no contributions are found, but there may be valid scenarios where a callout has zero contributions. Evaluate whether this should be handled as an error or simply be tolerated as an empty state.src/services/api/conversion/conversion.resolver.mutations.ts (3)
140-154: Mutation for VirtualContributor Conversion
This mutation’s purpose is clearly described. Proper guards and privilege checks are in place to ensure only qualified users can execute the conversion.
155-205: Robust Relationship Checks
You comprehensively verify the presence of the knowledge base, callouts set, and AI persona. These checks enforce strict relationship integrity, preventing conversion from proceeding in invalid states.
208-238: Space Account Consistency
Ensuring the account of the VC matches the space is an excellent validation step. This prevents cross-account data leaks or unauthorized callout transfers.
New module to transfer a callout to another callouts set
New mutation to leverage the transfer Callout capability to convert the type of a VC.
Tested with simple conversion + transfer.
Bonus is the mutation to transfer a callout from one callouts set to another, which likely will be also well appreciated by support. Allows us to understand the implication for transfering - for now only accessible to global admins.
Summary by CodeRabbit
• Introduced callout transfer functionality, enabling seamless movement of callouts between sets.
• Enhanced AI persona updates now support modifications to knowledge base attributes.
• Added virtual contributor conversion capability, allowing efficient migration of virtual contributor spaces to a dedicated knowledge base.