feat: establish formal relationship between KnowledgeUnit and Ebook (#35) #43
Reference in New Issue
Block a user
Delete Branch "fix/issue-35-knowledge-unit-relationship"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR finalizes the implementation of issue #35 by establishing a formal foreign key relationship between
KnowledgeUnitandEbook.Closes #35
Changes:
KnowledgeUnitto useGuid EbookIdand added navigation property.AppDbContextfluent configuration and generated a new migration.IKnowledgeServiceand its implementations to propagateebookId.Verification:
dotnet build).While the schema migration and domain configurations correctly establish the formal relationship between
KnowledgeUnitandEbook, there is a critical disconnect between the strict database schema constraints and the loosely-typed Service implementations.By making
ebookIdoptional in the interface but required in the domain/database, the system will experience silent failures (unhandledDbUpdateExceptionfor Foreign Key constraint violations) when clients likeKnowledgeCoordinatorinvoke the API without an explicitebookIdand the service defaults toGuid.Empty.Please address the inline comments to either strictly enforce
EbookIdpresence or gracefully handle 'Global' knowledge units as nullable foreign keys.@@ -9,3 +8,1 @@Task<Result<KnowledgePacket>> GetGraphDataAsync(string text, string tenantId, CancellationToken cancellationToken = default);Task<Result<KnowledgePacket>> GetKnowledgeMapAsync(string text, string tenantId, CancellationToken cancellationToken = default);Task<Result<KnowledgePacket>> GetSummaryAndQuizAsync(string text, string tenantId, CancellationToken cancellationToken = default);Task<Result<KnowledgePacket>> GetKnowledgeAsync(string text, string tenantId, Guid? ebookId = null, CancellationToken cancellationToken = default);Because
KnowledgeUnitnow strictly requires anEbookIdat the database level,ebookIdshould NOT be an optional parameter (Guid? ebookId = null) in theIKnowledgeServicecontract. By making it optional, callers likeKnowledgeCoordinatorare failing to provide it, which leads toGuid.Emptybeing inserted and crashing the application with a Foreign Key constraint violation. Please make this a requiredGuidor nullable in the database.@@ -14,3 +14,2 @@[Required][MaxLength(128)]public string SourceId { get; set; } = string.Empty;public Guid EbookId { get; set; }If the business requirement is that a
KnowledgeUnitmust always belong to anEbook, this[Required]constraint is correct. However, if 'Global' knowledge units exist (as noted in your service comments), thenEbookIdmust be a nullableGuid?. Please clarify the domain requirement and align the Database Schema, Domain Entity, and Service Interface to either strictly require the ID or explicitly allow nulls.@@ -211,0 +212,4 @@// If not provided, we should ideally have a "Global" ebook or make it nullable.// For now, we use Guid.Empty which will likely fail FK if not present,// or we make the property nullable in the entity.unit.EbookId = ebookId ?? Guid.Empty;Assigning
Guid.Emptyas a fallback for a required Foreign Key is a dangerous anti-pattern that circumvents compiler safety only to fail at runtime with a database exception. This violates our "Result Pattern / Zero exceptions for flow control" guideline. If a Knowledge Unit must be linked to an Ebook, require a validGuidin the method signature. If it can be unlinked, update the Domain Entity and EF Core configuration to support a nullableGuid? EbookId.I have addressed the review feedback by making
EbookIdnullable in theKnowledgeUnitdomain entity and database schema.This change:
KnowledgeCoordinatorand other callers to function correctly when no specific ebook context is present (e.g., global knowledge units).DbUpdateException(Foreign Key violations) by removing theGuid.Emptyfallback.A new migration
MakeKnowledgeUnitEbookIdNullablehas been added to apply these changes.