Standardize remaining drivers to DriverInterface specification#161
Standardize remaining drivers to DriverInterface specification#161
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
|
@copilot fix test |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed! The SQL driver Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
|
@copilot fix ci test |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed! The CI failure was in platform-node package - Jest couldn't parse @objectstack ES module packages. Updated Jest config to:
All 33 platform-node tests now pass (1b19db7). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
There was a problem hiding this comment.
Pull request overview
This pull request standardizes all remaining ObjectQL drivers to implement the DriverInterface specification pattern established by MongoDriver and SqlDriver. The changes enable compatibility with the new kernel-based plugin system from @objectstack/spec while maintaining backward compatibility with the legacy Driver interface.
Changes:
- Added driver metadata (name, version, supports) to all remaining drivers (Memory, FileSystem, Redis, LocalStorage, SDK/Remote, Excel)
- Implemented connect() and checkHealth() lifecycle methods across all drivers
- Added normalizeQuery() method to support both QueryAST and legacy UnifiedQuery formats
- Fixed SQL driver's count() method to properly handle filter arrays
- Updated Jest configuration in platform-node to handle ES module imports from @objectstack packages
- Created a mock implementation of @objectstack/runtime for testing
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/foundation/platform-node/test/mocks/@objectstack/runtime.ts | Mock implementation of ObjectStackKernel that delegates to legacy drivers during migration |
| packages/foundation/platform-node/jest.config.js | Updated Jest config with moduleNameMapper for runtime mock and transformIgnorePatterns for @objectstack packages |
| packages/drivers/sql/src/index.ts | Fixed count() method to properly handle both filter arrays and query objects without spreading arrays |
| packages/drivers/sdk/src/index.ts | Added DriverInterface compatibility: metadata, connect(), checkHealth(), and normalizeQuery() methods |
| packages/drivers/redis/src/index.ts | Added DriverInterface compatibility: metadata, checkHealth(), and normalizeQuery() methods |
| packages/drivers/memory/src/index.ts | Added DriverInterface compatibility: metadata, connect(), checkHealth(), and normalizeQuery() methods |
| packages/drivers/localstorage/src/index.ts | Added DriverInterface compatibility: metadata, connect(), checkHealth(), and normalizeQuery() methods |
| packages/drivers/fs/src/index.ts | Added DriverInterface compatibility: metadata, connect(), checkHealth(), and normalizeQuery() methods |
| packages/drivers/excel/src/index.ts | Added DriverInterface compatibility: metadata, connect(), checkHealth(), and normalizeQuery() methods |
| * Normalizes query format to support both legacy UnifiedQuery and QueryAST formats. | ||
| * This ensures backward compatibility while supporting the new @objectstack/spec interface. | ||
| * | ||
| * QueryAST format uses 'top' for limit, while UnifiedQuery uses 'limit'. | ||
| * QueryAST sort is array of {field, order}, while UnifiedQuery is array of [field, order]. | ||
| */ | ||
| private normalizeQuery(query: any): any { | ||
| if (!query) return {}; | ||
|
|
||
| const normalized: any = { ...query }; | ||
|
|
||
| // Normalize limit/top | ||
| if (normalized.top !== undefined && normalized.limit === undefined) { | ||
| normalized.limit = normalized.top; | ||
| } | ||
|
|
||
| // Normalize sort format | ||
| if (normalized.sort && Array.isArray(normalized.sort)) { | ||
| // Check if it's already in the array format [field, order] | ||
| const firstSort = normalized.sort[0]; | ||
| if (firstSort && typeof firstSort === 'object' && !Array.isArray(firstSort)) { | ||
| // Convert from QueryAST format {field, order} to internal format [field, order] | ||
| normalized.sort = normalized.sort.map((item: any) => [ | ||
| item.field, | ||
| item.order || item.direction || item.dir || 'asc' | ||
| ]); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The normalizeQuery method is duplicated across all drivers (Memory, FileSystem, Redis, LocalStorage, SDK, Excel) with identical implementation. This violates the DRY principle and makes maintenance difficult. Consider extracting this to a shared utility in @objectql/types or a common driver utilities package that all drivers can import.
| * Normalizes query format to support both legacy UnifiedQuery and QueryAST formats. | |
| * This ensures backward compatibility while supporting the new @objectstack/spec interface. | |
| * | |
| * QueryAST format uses 'top' for limit, while UnifiedQuery uses 'limit'. | |
| * QueryAST sort is array of {field, order}, while UnifiedQuery is array of [field, order]. | |
| */ | |
| private normalizeQuery(query: any): any { | |
| if (!query) return {}; | |
| const normalized: any = { ...query }; | |
| // Normalize limit/top | |
| if (normalized.top !== undefined && normalized.limit === undefined) { | |
| normalized.limit = normalized.top; | |
| } | |
| // Normalize sort format | |
| if (normalized.sort && Array.isArray(normalized.sort)) { | |
| // Check if it's already in the array format [field, order] | |
| const firstSort = normalized.sort[0]; | |
| if (firstSort && typeof firstSort === 'object' && !Array.isArray(firstSort)) { | |
| // Convert from QueryAST format {field, order} to internal format [field, order] | |
| normalized.sort = normalized.sort.map((item: any) => [ | |
| item.field, | |
| item.order || item.direction || item.dir || 'asc' | |
| ]); | |
| } | |
| } | |
| * Lightweight runtime check to ensure we only treat plain objects as queries. | |
| */ | |
| private isPlainObject(value: unknown): value is Record<string, unknown> { | |
| return typeof value === 'object' && value !== null && !Array.isArray(value); | |
| } | |
| /** | |
| * Normalizes query format to support both legacy UnifiedQuery and QueryAST formats. | |
| * This ensures backward compatibility while supporting the new @objectstack/spec interface. | |
| * | |
| * QueryAST format uses 'top' for limit, while UnifiedQuery uses 'limit'. | |
| * QueryAST sort is array of {field, order}, while UnifiedQuery is array of [field, order]. | |
| */ | |
| private normalizeQuery(query: unknown): Record<string, unknown> { | |
| if (!this.isPlainObject(query)) { | |
| return {}; | |
| } | |
| const normalized: Record<string, unknown> = { ...query }; | |
| // Normalize limit/top | |
| if (normalized.top !== undefined && normalized.limit === undefined) { | |
| normalized.limit = normalized.top; | |
| } | |
| // Normalize sort format | |
| const sortValue = normalized.sort; | |
| if (Array.isArray(sortValue) && sortValue.length > 0) { | |
| const firstSort = sortValue[0] as unknown; | |
| if (this.isPlainObject(firstSort)) { | |
| // Convert from QueryAST format {field, order} to internal format [field, order] | |
| normalized.sort = (sortValue as readonly unknown[]).map((item) => { | |
| if (!this.isPlainObject(item)) { | |
| return item; | |
| } | |
| const field = item.field as string; | |
| const order = | |
| (item.order as string | undefined) || | |
| (item.direction as string | undefined) || | |
| (item.dir as string | undefined) || | |
| 'asc'; | |
| return [field, order]; | |
| }); | |
| } | |
| } |
| * QueryAST format uses 'top' for limit, while UnifiedQuery uses 'limit'. | ||
| * QueryAST sort is array of {field, order}, while UnifiedQuery is array of [field, order]. | ||
| */ | ||
| private normalizeQuery(query: any): any { |
There was a problem hiding this comment.
The normalizeQuery method uses 'any' type for parameters and return value, which violates strict typing principles. Consider defining proper types for QueryAST and UnifiedQuery formats to ensure type safety during query transformation.
| let actualFilters = filters; | ||
| if (filters && !Array.isArray(filters)) { | ||
| // It's a query object, normalize it and extract filters | ||
| const normalizedQuery = this.normalizeQuery(filters); | ||
| actualFilters = normalizedQuery.filters || filters; | ||
| } | ||
|
|
||
| if (actualFilters) { |
There was a problem hiding this comment.
The count method's filter handling logic could be more defensive. When extracting filters from a normalized query object, if normalizedQuery.filters is falsy, it falls back to the original 'filters' parameter, which could still be an object rather than an array. Consider adding validation to ensure actualFilters is in the expected format before passing to applyFilters.
| let actualFilters = filters; | |
| if (filters && !Array.isArray(filters)) { | |
| // It's a query object, normalize it and extract filters | |
| const normalizedQuery = this.normalizeQuery(filters); | |
| actualFilters = normalizedQuery.filters || filters; | |
| } | |
| if (actualFilters) { | |
| let actualFilters: unknown = filters; | |
| if (filters && !Array.isArray(filters)) { | |
| // It's a query object, normalize it and extract filters | |
| const normalizedQuery = this.normalizeQuery(filters); | |
| if (Array.isArray(normalizedQuery.filters)) { | |
| actualFilters = normalizedQuery.filters; | |
| } else if (Array.isArray(filters)) { | |
| // fall back to original filters only if they are an array | |
| actualFilters = filters; | |
| } else { | |
| // if we can't derive a valid filters array, skip applying filters | |
| actualFilters = undefined; | |
| } | |
| } | |
| if (Array.isArray(actualFilters) && actualFilters.length > 0) { |
| async checkHealth(): Promise<boolean> { | ||
| try { | ||
| const endpoint = `${this.baseUrl.replace(/\/$/, '')}${this.rpcPath}`; | ||
| const res = await fetch(endpoint, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify({ | ||
| op: 'ping', | ||
| object: '_health', | ||
| args: {} | ||
| }) | ||
| }); | ||
| return res.ok; | ||
| } catch (error) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The checkHealth method makes a real network request with a 'ping' operation to '_health' object, but there's no documentation or guarantee that the remote server supports this operation. If the server doesn't implement this ping endpoint, the health check will always fail. Consider documenting this requirement or using a more standard health check endpoint, or making this configurable.
| item.order || item.direction || item.dir || 'asc' | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The normalizeQuery implementation is incomplete compared to MongoDriver and SqlDriver. These reference drivers also handle aggregation normalization (converting 'aggregations' to 'aggregate' format), which is missing in this implementation. For consistency with the reference drivers, consider adding aggregation normalization support.
| } | |
| } | |
| // Normalize aggregations -> aggregate (match MongoDriver/SqlDriver behavior) | |
| if (normalized.aggregations !== undefined && normalized.aggregate === undefined) { | |
| normalized.aggregate = normalized.aggregations; | |
| // Remove legacy key to avoid ambiguity | |
| delete normalized.aggregations; | |
| } |
| async checkHealth(): Promise<boolean> { | ||
| try { | ||
| // Check if data directory is accessible | ||
| if (!fs.existsSync(this.config.dataDir)) { | ||
| return false; | ||
| } | ||
| // Try to read directory | ||
| fs.readdirSync(this.config.dataDir); | ||
| return true; | ||
| } catch (error) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The checkHealth method also uses synchronous file system operations (fs.existsSync, fs.readdirSync) which is inconsistent with the async method signature and could block the event loop. Consider using async fs operations (fs.promises.access or fs.promises.readdir) for better performance and consistency with the async pattern.
| private normalizeQuery(query: any): any { | ||
| if (!query) return {}; | ||
|
|
||
| const normalized: any = { ...query }; | ||
|
|
||
| // Normalize limit/top | ||
| if (normalized.top !== undefined && normalized.limit === undefined) { | ||
| normalized.limit = normalized.top; | ||
| } | ||
|
|
||
| // Normalize sort format | ||
| if (normalized.sort && Array.isArray(normalized.sort)) { | ||
| // Check if it's already in the array format [field, order] | ||
| const firstSort = normalized.sort[0]; | ||
| if (firstSort && typeof firstSort === 'object' && !Array.isArray(firstSort)) { | ||
| // Convert from QueryAST format {field, order} to internal format [field, order] | ||
| normalized.sort = normalized.sort.map((item: any) => [ | ||
| item.field, | ||
| item.order || item.direction || item.dir || 'asc' | ||
| ]); | ||
| } | ||
| } | ||
|
|
||
| return normalized; | ||
| } |
There was a problem hiding this comment.
The normalizeQuery implementation is incomplete compared to MongoDriver and SqlDriver. These reference drivers also handle aggregation normalization (converting 'aggregations' to 'aggregate' format), which is missing in this implementation. For consistency with the reference drivers, consider adding aggregation normalization support.
| item.order || item.direction || item.dir || 'asc' | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The normalizeQuery implementation is incomplete compared to MongoDriver and SqlDriver. These reference drivers also handle aggregation normalization (converting 'aggregations' to 'aggregate' format), which is missing in this implementation. For consistency with the reference drivers, consider adding aggregation normalization support.
| } | |
| } | |
| // Normalize aggregations -> aggregate for aggregation queries | |
| // This aligns the Excel driver with MongoDriver and SqlDriver behavior. | |
| if (normalized.aggregations !== undefined && normalized.aggregate === undefined) { | |
| // Do a simple aliasing to preserve the original aggregation structure. | |
| normalized.aggregate = normalized.aggregations; | |
| } |
| */ | ||
|
|
||
| export class ObjectStackKernel { | ||
| public ql: unknown = null; |
There was a problem hiding this comment.
The property 'ql' is declared as 'unknown = null' but is never assigned or used. Consider removing it or providing proper initialization if it's needed for interface compatibility.
| item.order || item.direction || item.dir || 'asc' | ||
| ]); | ||
| } | ||
| } |
There was a problem hiding this comment.
The normalizeQuery implementation is incomplete compared to MongoDriver and SqlDriver. These reference drivers also handle aggregation normalization (converting 'aggregations' to 'aggregate' format), which is missing in this implementation. For consistency with the reference drivers, consider adding aggregation normalization support.
| } | |
| } | |
| // Normalize aggregations -> aggregate (for consistency with other drivers) | |
| if (normalized.aggregations !== undefined && normalized.aggregate === undefined) { | |
| normalized.aggregate = normalized.aggregations; | |
| } |
Transform Remaining Drivers to Match Mongo/SQL Pattern
Reference: Following the pattern established in MongoDriver and SqlDriver
Summary
Successfully transformed all remaining ObjectQL drivers to implement the standard DriverInterface pattern from @objectstack/spec, ensuring compatibility with the new kernel-based plugin system.
Changes Completed ✅
Key Changes Made
1. Driver Metadata
All drivers now expose:
name: Driver identifier (e.g., 'MemoryDriver', 'FileSystemDriver')version: Version number ('3.0.1')supports: Capability flags (transactions, joins, fullTextSearch, jsonFields, arrayFields)2. DriverInterface Compatibility Methods
connect(): Initialize driver connection (no-op for local drivers)checkHealth(): Verify driver health status3. QueryAST Normalization
Added
normalizeQuery()method to all drivers supporting:top→limitconversion{field, order}→ internal format[field, order]4. SQL Driver Fix
Fixed the
count()method in SQL driver to properly handle filter arrays without spreading them into objects with numeric keys.5. Platform-Node Jest Configuration Fix
Fixed CI test failures by updating the Jest configuration in
@objectql/platform-node:transformIgnorePatternsto allow transformation of@objectstackpackages@objectstack/runtimeto avoid ES module import issuesesModuleInteropandallowSyntheticDefaultImportsto TypeScript configBefore:
After:
Test Results
✅ MemoryDriver: 22/22 tests passing
⚠️ RedisDriver: Tests require Redis server (expected in CI)
✅ FileSystemDriver: 36/36 tests passing
✅ LocalStorageDriver: 31/31 tests passing
✅ SDKDriver: 25/25 tests passing
✅ ExcelDriver: Tests passing
✅ SqlDriver: 63/63 tests passing
✅ MongoDriver: Tests passing
✅ Platform-Node: 33/33 tests passing (was failing in CI)
Notes
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.