fix(core): resolve STI base tableName in getAllSchemas for subclasses (#693)#694
Conversation
BREAKING CHANGE: Town and weather components have been moved to their respective domain packages: - TownHeader, CouncilSidebar, ArticleReferences → @happyvertical/praeco/town - WeatherForecast, WeatherHeader → @happyvertical/caelus/weather Also removes domain-specific types from types-generic.ts that were moved to the target packages. Part of RFC #677 Phase 4 cleanup.
…sions Implements multi-level tenant hierarchies with configurable permission inheritance: - Add parentTenantId, hierarchyLevel, hierarchyPath fields to Tenant model - Add cascadePermissions and inheritPermissions flags for inheritance control - Create TenantPermissionOverride model for explicit grant/deny/inherit - Add TenantPermissionOverrideCollection with CRUD and helper methods - Extend TenantCollection with hierarchy methods: - findRoots(), findChildren(), getAncestors(), getDescendants() - createChild(), moveToParent(), makeRoot() - isAncestorOf(), isDescendantOf(), validateHierarchy() - getTree() for nested structure retrieval - Extend PermissionResolver with: - resolveTenantPermissions() for hierarchy-aware resolution - getTenantInheritanceChain() for debugging inheritance - Add TenantPermissionEffect enum (INHERIT, GRANT, DENY) - Add TenantHierarchyError for hierarchy operation failures - Add comprehensive test suite for all hierarchy operations - Export all new types, models, and collections from package Supports arbitrary depth (configurable max: 10 levels) with: - Circular reference prevention - Materialized path for efficient traversal - Cascade chain breaking at any level
- TenantCollection.create() now preserves existing hierarchy fields when hydrating from database. Previously, it unconditionally recalculated hierarchyLevel and hierarchyPath when parentTenantId was present, which broke validateHierarchy() checks. - TenantCollection.moveToParent() now fetches descendants BEFORE saving the tenant. Previously, getDescendants() was called AFTER saving, which meant the path lookup used the new path instead of the old one, failing to find any descendants to update. - Clean up debug console.log statements from tests
Review fixes: - Fix getDescendants boundary matching bug: use exact match OR prefix+'/' to avoid matching "tenant1" with "tenant123" - Fix hierarchyPath documentation: format is "ancestor-id/parent-id" (does not include self-id) - Rename hasChildren() to canCascadeToChildren() for clarity - Remove unused permissionEffects map and TenantPermissionEffect import - Remove unused TenantPermissionEffect import from tests - Remove unused child2 variable in test - Use substring instead of replace() for explicit path replacement - Optimize removeAllOverrides with Promise.all for parallel deletion
…#693) When STI subclasses declare their own `tableName` in the manifest, `getAllSchemas()` now correctly resolves to the STI base class's tableName. This ensures all subclass columns are merged into the parent table schema, allowing `db:migrate` to detect missing columns. The fix checks if a class uses STI strategy, finds the STI base class, and uses its tableName instead of the subclass's declared tableName. Closes #693
There was a problem hiding this comment.
Pull request overview
This pull request addresses two distinct but unrelated concerns:
Primary Focus (Per Title): Fixes issue #693 where STI (Single Table Inheritance) subclasses that declare their own tableName in manifests were not having their columns properly merged into the parent table schema during db:migrate. The fix modifies ObjectRegistry.getAllSchemas() to detect STI subclasses and use the base class's tableName, ensuring all subclass columns are correctly merged into the parent table.
Secondary Feature (Not in Title): Adds comprehensive hierarchical tenant support to the users package, including parent-child relationships, materialized path tracking, permission inheritance with cascade control, and tenant-level permission overrides. This is a substantial feature addition spanning multiple models, collections, services, and 800+ lines of test coverage.
Changes:
- Core STI fix in
getAllSchemas()with 9 comprehensive tests covering schema merging, column detection, and end-to-end persistence - New
TenantPermissionEffectenum for hierarchical permission inheritance - Enhanced
Tenantmodel with hierarchy fields, cascade control, and helper methods - New
TenantPermissionOverridemodel for tenant-level permission management - Enhanced
TenantCollectionwith hierarchy operations (move, validate, tree traversal) - New
TenantPermissionOverrideCollectionfor managing overrides - Enhanced
PermissionResolverwith tenant hierarchy permission resolution - Comprehensive test suite covering hierarchy creation, traversal, validation, and permission inheritance
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/registry.ts | STI fix: resolve base tableName for subclasses in getAllSchemas() |
| packages/core/src/tests/issue-693-sti-separate-tablename.test.ts | 9 tests validating STI schema merging and persistence |
| packages/types/src/user.ts | Add TenantPermissionEffect enum for inheritance control |
| packages/types/src/index.ts | Export TenantPermissionEffect |
| packages/users/src/models/Tenant.ts | Add hierarchy fields (parentTenantId, hierarchyLevel, hierarchyPath) and cascade control flags |
| packages/users/src/models/TenantPermissionOverride.ts | New model for tenant-level permission overrides (GRANT/DENY/INHERIT) |
| packages/users/src/models/index.ts | Export new MAX_TENANT_HIERARCHY_DEPTH constant and TenantPermissionOverride |
| packages/users/src/collections/TenantCollection.ts | Add hierarchy management (createChild, moveToParent, tree traversal, validation) |
| packages/users/src/collections/TenantPermissionOverrideCollection.ts | New collection for managing tenant permission overrides |
| packages/users/src/collections/index.ts | Export new collection and types |
| packages/users/src/services/PermissionResolver.ts | Add resolveTenantPermissions() and getTenantInheritanceChain() for hierarchical permissions |
| packages/users/src/services/index.ts | Export TenantPermissionInheritanceResult type |
| packages/users/src/types/index.ts | Re-export TenantPermissionEffect |
| packages/users/src/index.ts | Export all new models, collections, types, and constants |
| packages/users/src/tests/hierarchical-tenants.test.ts | 802 lines of tests covering hierarchy operations and permission inheritance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Start with inherited permissions if applicable | ||
| if (shouldInherit) { | ||
| for (const permId of inheritedPermissions) { | ||
| currentPermissions.add(permId); | ||
| } | ||
| result.contributingTenantIds.push(current.id!); | ||
| } | ||
|
|
||
| // Apply grants | ||
| for (const permId of overrides.grantedPermissionIds) { | ||
| currentPermissions.add(permId); | ||
| if (!result.contributingTenantIds.includes(current.id!)) { | ||
| result.contributingTenantIds.push(current.id!); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for populating contributingTenantIds may produce unexpected results. A tenant is added to the list at line 208 simply because shouldInherit is true, even if no permissions are actually inherited (the inherited set could be empty). Additionally, if a root tenant has explicit grants, it's added via lines 214-216, but if it has only denies or no overrides at all, it won't be in the contributing list even though it affects the inheritance chain. Consider refactoring to only add tenants that actually contribute permissions (either by granting or being part of an active inheritance chain with non-empty permissions).
| for (const descendant of descendants) { | ||
| // Update path by replacing old prefix with new prefix | ||
| // Use substring for explicit control rather than replace() | ||
| if (descendant.hierarchyPath?.startsWith(oldPath)) { | ||
| descendant.hierarchyPath = | ||
| newPathForDescendants + | ||
| descendant.hierarchyPath.substring(oldPath.length); | ||
| } | ||
| // Adjust level by the same delta | ||
| descendant.hierarchyLevel += levelDelta; | ||
| await descendant.save(); |
There was a problem hiding this comment.
The descendant update loop saves each descendant individually, which could result in N database writes for a hierarchy with N descendants. This is inefficient and could cause performance issues when moving tenants with many descendants. Consider implementing a batch update mechanism or using a single UPDATE query with a WHERE clause to update all descendants at once.
| for (const descendant of descendants) { | |
| // Update path by replacing old prefix with new prefix | |
| // Use substring for explicit control rather than replace() | |
| if (descendant.hierarchyPath?.startsWith(oldPath)) { | |
| descendant.hierarchyPath = | |
| newPathForDescendants + | |
| descendant.hierarchyPath.substring(oldPath.length); | |
| } | |
| // Adjust level by the same delta | |
| descendant.hierarchyLevel += levelDelta; | |
| await descendant.save(); | |
| if (descendants.length > 0) { | |
| // Prepare bulk update operations for descendants | |
| const bulkOps: any[] = []; | |
| for (const descendant of descendants) { | |
| let newHierarchyPath = descendant.hierarchyPath ?? ''; | |
| // Update path by replacing old prefix with new prefix | |
| // Use substring for explicit control rather than replace() | |
| if (descendant.hierarchyPath?.startsWith(oldPath)) { | |
| newHierarchyPath = | |
| newPathForDescendants + | |
| descendant.hierarchyPath.substring(oldPath.length); | |
| } | |
| const newHierarchyLevel = descendant.hierarchyLevel + levelDelta; | |
| bulkOps.push({ | |
| updateOne: { | |
| filter: { id: descendant.id }, | |
| update: { | |
| $set: { | |
| hierarchyPath: newHierarchyPath, | |
| hierarchyLevel: newHierarchyLevel, | |
| }, | |
| }, | |
| }, | |
| }); | |
| } | |
| const tenantModel: any = Tenant as any; | |
| if (typeof tenantModel.bulkWrite === 'function') { | |
| // Perform a single bulk write for all descendants | |
| await tenantModel.bulkWrite(bulkOps); | |
| } else { | |
| // Fallback: update and save each descendant individually | |
| for (const descendant of descendants) { | |
| if (descendant.hierarchyPath?.startsWith(oldPath)) { | |
| descendant.hierarchyPath = | |
| newPathForDescendants + | |
| descendant.hierarchyPath.substring(oldPath.length); | |
| } | |
| descendant.hierarchyLevel += levelDelta; | |
| await descendant.save(); | |
| } | |
| } |
| if (descendant.hierarchyPath?.startsWith(oldPath)) { | ||
| descendant.hierarchyPath = | ||
| newPathForDescendants + | ||
| descendant.hierarchyPath.substring(oldPath.length); | ||
| } |
There was a problem hiding this comment.
If a descendant's hierarchyPath does not start with oldPath, it is silently skipped without any error or warning. This could leave the hierarchy in an inconsistent state. If getDescendants is working correctly, this condition should never be false. Either remove the check (assuming getDescendants is correct) or throw an error if this unexpected condition occurs, as it indicates data corruption or a bug in getDescendants.
| if (descendant.hierarchyPath?.startsWith(oldPath)) { | |
| descendant.hierarchyPath = | |
| newPathForDescendants + | |
| descendant.hierarchyPath.substring(oldPath.length); | |
| } | |
| if (!descendant.hierarchyPath) { | |
| throw new TenantHierarchyError( | |
| `Descendant tenant ${descendant.id} has no hierarchyPath while updating hierarchy from ${oldPath} to ${newPathForDescendants}`, | |
| 'INVALID_OPERATION', | |
| ); | |
| } | |
| if (!descendant.hierarchyPath.startsWith(oldPath)) { | |
| throw new TenantHierarchyError( | |
| `Descendant tenant ${descendant.id} has hierarchyPath "${descendant.hierarchyPath}" which does not start with expected prefix "${oldPath}"`, | |
| 'INVALID_OPERATION', | |
| ); | |
| } | |
| descendant.hierarchyPath = | |
| newPathForDescendants + | |
| descendant.hierarchyPath.substring(oldPath.length); |
| * ## Resolution Order | ||
| * | ||
| * When resolving effective tenant permissions: | ||
| * 1. Walk up the tenant hierarchy collecting permissions |
There was a problem hiding this comment.
The documentation incorrectly states "Walk up the tenant hierarchy collecting permissions". The actual implementation walks DOWN from root to the target tenant (as shown in PermissionResolver.resolveTenantPermissions lines 144-149). This should be corrected to say "Walk down the tenant hierarchy from root to target, accumulating permissions" or similar to accurately reflect the implementation.
| * 1. Walk up the tenant hierarchy collecting permissions | |
| * 1. Walk down the tenant hierarchy from root to target, accumulating permissions |
| // Find all tenants whose hierarchyPath starts with this prefix | ||
| // Note: This loads all tenants and filters in memory. For large hierarchies, | ||
| // consider implementing database-level LIKE query. | ||
| const allTenants = await this.list({}); | ||
|
|
||
| // Use exact match OR prefix + '/' to ensure proper boundary detection. | ||
| // Without the '/' check, "ancestor/tenant1" would incorrectly match "ancestor/tenant123". | ||
| return allTenants.filter( | ||
| (t) => | ||
| t.hierarchyPath === pathPrefix || | ||
| t.hierarchyPath?.startsWith(`${pathPrefix}/`), | ||
| ); |
There was a problem hiding this comment.
The getDescendants method loads ALL tenants into memory and filters them client-side. This could cause performance issues with large tenant hierarchies. Consider implementing a database-level query using LIKE or a proper full-text search on the hierarchyPath field. The existing comment acknowledges this but the implementation should be optimized before shipping, especially since this is part of a public API that could be used in production systems with many tenants.
| // Find all tenants whose hierarchyPath starts with this prefix | |
| // Note: This loads all tenants and filters in memory. For large hierarchies, | |
| // consider implementing database-level LIKE query. | |
| const allTenants = await this.list({}); | |
| // Use exact match OR prefix + '/' to ensure proper boundary detection. | |
| // Without the '/' check, "ancestor/tenant1" would incorrectly match "ancestor/tenant123". | |
| return allTenants.filter( | |
| (t) => | |
| t.hierarchyPath === pathPrefix || | |
| t.hierarchyPath?.startsWith(`${pathPrefix}/`), | |
| ); | |
| // Find all tenants whose hierarchyPath matches this prefix at the database level. | |
| // Use exact match OR prefix + '/%' to ensure proper boundary detection. | |
| // Without the '/' check, "ancestor/tenant1" would incorrectly match "ancestor/tenant123". | |
| const descendants = await this.list({ | |
| where: { | |
| OR: [ | |
| { hierarchyPath: pathPrefix }, | |
| { hierarchyPath: { like: `${pathPrefix}/%` } }, | |
| ], | |
| }, | |
| }); | |
| return descendants; |
| for (let i = 0; i < chain.length; i++) { | ||
| const current = chain[i]; | ||
| const isFirst = i === 0; | ||
| const previous = isFirst ? null : chain[i - 1]; | ||
|
|
||
| // Check if inheritance is active for this tenant | ||
| const shouldInherit = | ||
| !isFirst && previous?.cascadePermissions && current.inheritPermissions; | ||
|
|
||
| if (shouldInherit) { | ||
| result.inheritanceActive = true; | ||
| } | ||
|
|
||
| // Get this tenant's permission overrides | ||
| const overrides = | ||
| await this.tenantPermissionOverrideCollection.getOverridesByEffect( | ||
| current.id!, | ||
| ); | ||
|
|
||
| // Collect all permission IDs we need to look up | ||
| for (const id of overrides.grantedPermissionIds) allPermissionIds.add(id); | ||
| for (const id of overrides.deniedPermissionIds) allPermissionIds.add(id); | ||
|
|
||
| // Build this tenant's effective permissions | ||
| const currentPermissions = new Set<string>(); | ||
|
|
||
| // Start with inherited permissions if applicable | ||
| if (shouldInherit) { | ||
| for (const permId of inheritedPermissions) { | ||
| currentPermissions.add(permId); | ||
| } | ||
| result.contributingTenantIds.push(current.id!); | ||
| } | ||
|
|
||
| // Apply grants | ||
| for (const permId of overrides.grantedPermissionIds) { | ||
| currentPermissions.add(permId); | ||
| if (!result.contributingTenantIds.includes(current.id!)) { | ||
| result.contributingTenantIds.push(current.id!); | ||
| } | ||
| } | ||
|
|
||
| // Apply denies (remove) | ||
| for (const permId of overrides.deniedPermissionIds) { | ||
| currentPermissions.delete(permId); | ||
| } | ||
|
|
||
| // This becomes the inherited set for the next iteration | ||
| inheritedPermissions = currentPermissions; | ||
| } |
There was a problem hiding this comment.
The permission resolution algorithm performs N database queries (one per tenant in the hierarchy) via getOverridesByEffect. For deep hierarchies, this could result in significant latency. Consider batching these queries or implementing a single query that fetches all overrides for the entire chain at once.
| // Verify that the classes have their own declared tableNames | ||
| const meetingClass = ObjectRegistry.findClass('Issue693Meeting'); | ||
| const forecastClass = ObjectRegistry.findClass('Issue693WeatherForecast'); |
There was a problem hiding this comment.
Unused variable meetingClass.
| // Verify that the classes have their own declared tableNames | |
| const meetingClass = ObjectRegistry.findClass('Issue693Meeting'); | |
| const forecastClass = ObjectRegistry.findClass('Issue693WeatherForecast'); | |
| // Verify that the base class has its declared tableName |
| const meetingClass = ObjectRegistry.findClass('Issue693Meeting'); | ||
| const forecastClass = ObjectRegistry.findClass('Issue693WeatherForecast'); | ||
| const eventClass = ObjectRegistry.findClass('Issue693Event'); | ||
|
|
There was a problem hiding this comment.
Unused variable forecastClass.
| expect(meetingClass).toBeDefined(); | |
| expect(forecastClass).toBeDefined(); |
- Fix unused variables in issue-693 test file by adding assertions - Fix TenantPermissionOverride documentation (walk up -> walk down) - Optimize TenantCollection.getDescendants to use LIKE query - Add error handling for descendant update in TenantCollection - Improve PermissionResolver contributingTenantIds logic - Batch permission override queries in resolveTenantPermissions
Resolve merge conflicts: - TenantCollection.ts: Keep LIKE query optimization and error handling - TenantPermissionOverrideCollection.ts: Keep batch query method - TenantPermissionOverride.ts: Keep corrected "walk down" documentation - PermissionResolver.ts: Keep batch optimization and improved contributingTenantIds tracking
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use database-level LIKE query for initial filtering | ||
| // This is much more efficient than loading all tenants for large hierarchies | ||
| const candidates = await this.list({ | ||
| where: { | ||
| 'hierarchyPath like': `${pathPrefix}%`, |
There was a problem hiding this comment.
The LIKE query at line 175 uses user-controlled data (tenant IDs from hierarchyPath) without escaping SQL LIKE wildcard characters. If a tenant ID contains % or _ characters, these will be interpreted as wildcards, potentially matching unintended records.
For example, if a tenant has ID "tenant%123", the query would match "tenant" + any characters + "123", not just "tenant%123" exactly.
Consider escaping the pathPrefix before using it in a LIKE query, or verify that tenant IDs cannot contain wildcard characters. Most database adapters provide a way to escape LIKE patterns (e.g., replacing % with \% and _ with \_ and specifying an ESCAPE clause).
| // Use database-level LIKE query for initial filtering | |
| // This is much more efficient than loading all tenants for large hierarchies | |
| const candidates = await this.list({ | |
| where: { | |
| 'hierarchyPath like': `${pathPrefix}%`, | |
| // Escape SQL LIKE wildcard characters in the path prefix so that | |
| // '%' and '_' inside tenant IDs are treated literally. | |
| const escapeLikePattern = (value: string): string => | |
| value.replace(/([\\%_])/g, '\\$1'); | |
| const escapedPathPrefix = escapeLikePattern(pathPrefix); | |
| // Use database-level LIKE query for initial filtering | |
| // This is much more efficient than loading all tenants for large hierarchies | |
| const candidates = await this.list({ | |
| where: { | |
| // Keep a trailing '%' as the intended wildcard for prefix matching. | |
| 'hierarchyPath like': `${escapedPathPrefix}%`, |
| const tableStrategy = ObjectRegistry.getTableStrategy(_className); | ||
| if (tableStrategy === 'sti') { | ||
| const stiBaseName = ObjectRegistry.getSTIBase(_className); | ||
| if (stiBaseName && stiBaseName !== _className) { | ||
| // This is an STI subclass - use the base class's tableName | ||
| const stiBaseClass = ObjectRegistry.findClass(stiBaseName); | ||
| if (stiBaseClass?.schema?.tableName) { | ||
| tableName = stiBaseClass.schema.tableName; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The fix has a critical logic flaw due to how getSTIBase() works. The getSTIBase() method (defined elsewhere in the file at lines 3577-3618) requires that subclass and base class have matching tableNames to identify the STI base (see line 3609: if (ancestorTableName === targetTableName)).
However, the entire premise of issue #693 is that STI subclasses declare DIFFERENT tableNames than their parent:
- Parent Issue693Event has tableName = 'issue693events'
- Child Issue693Meeting declares tableName = 'issue693meetings'
- But at runtime, Meeting should use parent's 'issue693events' table
When this code calls getSTIBase('Issue693Meeting'):
- Gets targetTableName = 'issue693meetings' from the registered class
- Walks up to find ancestor 'Issue693Event'
- Checks if ancestorTableName ('issue693events') == targetTableName ('issue693meetings')
- Comparison fails, doesn't return the ancestor
- Falls through to return 'Issue693Meeting' itself
- Back here, stiBaseName == _className, so the if condition at line 2450 is false
- Never enters the branch that's supposed to fix the tableName
The fix becomes a no-op. To properly fix issue #693, getSTIBase() needs to be refactored to find STI base classes WITHOUT requiring tableName to match. For STI, it should return the first ancestor with tableStrategy: 'sti', regardless of declared tableName.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (shouldInherit && inheritedPermissions.size > 0) { | ||
| for (const permId of inheritedPermissions) { | ||
| currentPermissions.add(permId); | ||
| } | ||
| result.contributingTenantIds.push(current.id!); | ||
| contributed = true; | ||
| } |
There was a problem hiding this comment.
The condition for marking a tenant as contributing has been made more restrictive by adding && inheritedPermissions.size > 0. This changes behavior: previously, a child tenant that inherits from a parent would be marked as contributing even when inheriting an empty permission set. Now, it will only be marked as contributing if it actually inherits non-empty permissions or has its own grants/denies. While this is arguably more accurate (a tenant inheriting nothing truly doesn't contribute), this is a subtle behavior change that could affect code relying on contributingTenantIds to track the inheritance chain rather than actual permission contributions. Consider documenting this behavior change or verifying it's intentional.
Summary
getAllSchemas()to use STI base class'stableNamefor STI subclassesProblem
When STI subclasses declare their own
tableNamein the manifest (e.g.,Meetingusesmeetingsinstead of inheritingeventsfromEvent),db:migratedidn't detect their columns as missing from the parent table. This caused schema drift errors at runtime:Solution
Modified
ObjectRegistry.getAllSchemas()to:tableNameinstead of the subclass's declaredtableNameTest plan
Closes #693