-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] Adjust objectql to integrate with dataengine plugin #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Created data-engine.zod.ts in spec/system with Zod schemas - Updated ObjectQL class to implement IDataEngine interface - Modified method signatures to match IDataEngine contract - Updated ObjectQLPlugin to register as both 'objectql' and 'data-engine' - Re-exported IDataEngine from runtime for backward compatibility Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Fixed Zod schema to use manual TypeScript interface for better type safety - Added proper QueryOptions to QueryAST conversion (sort Record to orderBy array) - Created comprehensive test suite for IDataEngine interface - Updated test-objectql-plugin.ts import path - All tests passing (spec package: 1718 tests, custom tests: 100%) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot 参考以下内容进一步优化,这是一个非常深刻的架构分层问题。
export interface IDatabaseDriver {
/**
/**
/**
B. 第二层:Data Engine (核心逻辑) export class DataEngine { constructor(driver: IDatabaseDriver) { // 对外暴露的 API }
class PostgresDriver implements IDatabaseDriver { export const PostgresDriverPlugin: Plugin = { Kernel 的组装逻辑 (bootstrap): 总结
|
| import { ObjectKernel } from './packages/runtime/src/mini-kernel.js'; | ||
| import { ObjectQLPlugin } from './packages/runtime/src/objectql-plugin.js'; | ||
| import { DriverPlugin } from './packages/runtime/src/driver-plugin.js'; | ||
| import { ObjectQL } from './packages/objectql/src/index.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
To fix an unused import, you remove it if it truly is not used, or you start using it if the omission was a bug. Here, the cleanest fix that does not affect current behavior is to delete the ObjectQL import statement.
Concretely, in test-dataengine-interface.ts, remove line 13:
import { ObjectQL } from './packages/objectql/src/index.js';No other code changes are required, as no references to ObjectQL exist in the provided snippet. This will eliminate the unused import warning without changing any functionality.
| @@ -10,7 +10,6 @@ | ||
| import { ObjectKernel } from './packages/runtime/src/mini-kernel.js'; | ||
| import { ObjectQLPlugin } from './packages/runtime/src/objectql-plugin.js'; | ||
| import { DriverPlugin } from './packages/runtime/src/driver-plugin.js'; | ||
| import { ObjectQL } from './packages/objectql/src/index.js'; | ||
| import type { IDataEngine } from './packages/spec/src/system/data-engine.zod.js'; | ||
|
|
||
| // Mock driver for testing |
|
|
||
| // Verify objectql service is still available (backward compatibility) | ||
| try { | ||
| const ql = kernel.getService('objectql'); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
In general, to fix an unused variable warning, remove the variable declaration or use the variable meaningfully. Here, the variable ql at line 75 is only used to hold the result of kernel.getService('objectql') and is never read; the test only needs to ensure that the call succeeds without throwing. The cleanest fix that preserves existing behavior is to drop the binding and just call kernel.getService('objectql'); as a standalone statement inside the try block.
Concretely, in test-dataengine-interface.ts, within testDataEngineService, replace the line const ql = kernel.getService('objectql'); with kernel.getService('objectql');. No imports, new methods, or additional definitions are required; we just remove the unused variable binding.
-
Copy modified line R75
| @@ -72,7 +72,7 @@ | ||
|
|
||
| // Verify objectql service is still available (backward compatibility) | ||
| try { | ||
| const ql = kernel.getService('objectql'); | ||
| kernel.getService('objectql'); | ||
| console.log('✅ objectql service still available (backward compatibility)'); | ||
| } catch (e: any) { | ||
| throw new Error(`FAILED: objectql service not found: ${e.message}`); |
| */ | ||
|
|
||
| import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from '../packages/runtime/src'; | ||
| import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from './packages/runtime/src/index.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 7 hours ago
In general, unused imports should be removed from the import list to avoid confusion and keep the codebase clean. Since SchemaRegistry is not referenced anywhere in test-objectql-plugin.ts, the best fix is to delete it from the named imports on line 10.
Concretely, edit test-objectql-plugin.ts at the import statement on line 10, changing:
import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from './packages/runtime/src/index.js';to:
import { ObjectKernel, ObjectQLPlugin, ObjectQL } from './packages/runtime/src/index.js';No other code changes, imports, or definitions are required, and this does not alter any existing functionality since SchemaRegistry was never used.
-
Copy modified line R10
| @@ -7,7 +7,7 @@ | ||
| * 3. Multiple plugins with ObjectQL work | ||
| */ | ||
|
|
||
| import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from './packages/runtime/src/index.js'; | ||
| import { ObjectKernel, ObjectQLPlugin, ObjectQL } from './packages/runtime/src/index.js'; | ||
|
|
||
| async function testPluginBasedRegistration() { | ||
| console.log('\n=== Test 1: Plugin-based ObjectQL Registration ==='); |
- Fixed ObjectQL.delete() to return boolean directly from driver - Removed incorrect comment about driver returning record/null - Fixed MockDriver.delete() to return boolean per DriverInterface spec - Renamed QueryFilter/QueryOptions to DataEngineFilter/DataEngineQueryOptions to avoid naming collisions with existing data protocol types - All tests passing Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
感谢您详细的架构指导!我确认当前实现已经正确遵循了您描述的三层架构模式: 1. Consumer 层 (API/Flow/UI)
2. DataEngine 层 (ObjectQL)
3. Driver 层 (Postgres/Mongo/Memory)
这样做到了业务逻辑不重复 - 所有的权限检查、Hooks 执行、校验等只在 ObjectQL 中实现一次,各个 Driver 只专注于数据持久化。 最新提交 (bc82cda) 修复了:
|
|
@copilot dataengine 应该在 spec/data下还是 spec/system下 |
应该在 理由:
当前位置正确 ✅ 但需要删除 |
|
@copilot 帮我继续修改 |
- Renamed QueryFilterSchema to DataEngineFilterSchema - Renamed QueryOptionsSchema to DataEngineQueryOptionsSchema - Updated all imports in objectql and runtime packages - This prevents the doc generator from creating duplicate files in data/ - Confirmed data-engine.zod.ts should remain in spec/system (infrastructure layer) - All tests passing Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
请问具体需要修改什么呢? 当前已完成:
如果有具体的改进点或新需求,请告知,我会继续优化。 |
| */ | ||
|
|
||
| import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, QueryOptions } from './index.js'; | ||
| import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, DataEngineQueryOptions } from './index.js'; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
To fix the problem, we should remove the unused imports IHttpRequest and IHttpResponse from the import list in packages/runtime/src/test-interfaces.ts. This eliminates the unused program elements without changing any runtime or type-checking behavior, since they are not referenced elsewhere in the file.
Concretely, in packages/runtime/src/test-interfaces.ts, edit the import on line 8 so that only the actually used symbols remain. Replace:
import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, DataEngineQueryOptions } from './index.js';with:
import { IHttpServer, IDataEngine, RouteHandler, Middleware, DataEngineQueryOptions } from './index.js';No additional methods, imports, or definitions are needed. This is a minimal change that only removes unused imports and preserves all existing functionality.
-
Copy modified line R8
| @@ -5,7 +5,7 @@ | ||
| * and IDataEngine interfaces without depending on concrete implementations. | ||
| */ | ||
|
|
||
| import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, DataEngineQueryOptions } from './index.js'; | ||
| import { IHttpServer, IDataEngine, RouteHandler, Middleware, DataEngineQueryOptions } from './index.js'; | ||
|
|
||
| /** | ||
| * Example: Mock HTTP Server Plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements the IDataEngine interface for ObjectQL, following a three-layer architecture pattern (Consumer → DataEngine → Driver). The implementation creates a standardized interface in @objectstack/spec and integrates it with the existing ObjectQL engine while maintaining backward compatibility.
Changes:
- Created IDataEngine schema definition following Zod-first approach in the spec package
- Modified ObjectQL class to implement the IDataEngine interface with proper query conversion logic
- Updated ObjectQLPlugin to register ObjectQL as both 'data-engine' and 'objectql' services for backward compatibility
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/system/data-engine.zod.ts | New schema definition for IDataEngine interface with DataEngineQueryOptions and DataEngineFilter types |
| packages/spec/src/system/index.ts | Exports the new data-engine protocol definitions |
| packages/spec/json-schema/system/DataEngine*.json | Generated JSON schemas for data engine types |
| packages/objectql/src/index.ts | Modified ObjectQL to implement IDataEngine, converting DataEngineQueryOptions to internal QueryAST format |
| packages/runtime/src/interfaces/data-engine.ts | Consolidated interface file to re-export from spec package |
| packages/runtime/src/objectql-plugin.ts | Registers ObjectQL as both 'data-engine' and 'objectql' services |
| packages/runtime/src/test-interfaces.ts | Updated to use DataEngineQueryOptions type |
| test-dataengine-interface.ts | New test file validating IDataEngine interface compliance |
| test-objectql-plugin.ts | Minor updates to test error messages and import paths |
| content/docs/references/system/data-engine.mdx | Documentation for the new data engine protocol |
| content/docs/references/system/index.mdx | Added data-engine to the system references index |
| content/docs/references/system/meta.json | Added data-engine to navigation metadata |
|
|
||
| ```typescript | ||
| import { DataEngineSchema, DataEngineFilterSchema, DataEngineQueryOptionsSchema } from '@objectstack/spec/system'; | ||
| import type { DataEngine, DataEngineFilter, DataEngineQueryOptions } from '@objectstack/spec/system'; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation references a type DataEngine that doesn't exist in the schema file. The schema file exports DataEngineSchema but doesn't export a corresponding type derived from it using z.infer.
Following the established pattern in other schema files (like driver.zod.ts which exports export type DriverInterface = z.infer<typeof DriverInterfaceSchema>), you should add:
export type DataEngine = z.infer<typeof DataEngineSchema>;after line 98 in the schema file. However, note that the manually defined IDataEngine interface (lines 102-107) serves the same purpose but with better type safety for optional parameters. Consider whether you need both or if the documentation should reference IDataEngine instead.
| import { z } from 'zod'; | ||
|
|
||
| /** | ||
| * Data Engine Protocol | ||
| * | ||
| * Defines the standard interface for data persistence engines. | ||
| * This allows different data engines (ObjectQL, Prisma, TypeORM, etc.) | ||
| * to be used interchangeably through a common interface. | ||
| * | ||
| * Following the Dependency Inversion Principle - plugins depend on this interface, | ||
| * not on concrete database implementations. | ||
| */ | ||
|
|
||
| /** | ||
| * Data Engine Query filter conditions | ||
| * Simple key-value filter structure for IDataEngine.find() operations | ||
| */ | ||
| export const DataEngineFilterSchema = z.record(z.any()).describe('Data Engine query filter conditions'); | ||
|
|
||
| /** | ||
| * Query options for IDataEngine.find() operations | ||
| */ | ||
| export const DataEngineQueryOptionsSchema = z.object({ | ||
| /** Filter conditions */ | ||
| filter: DataEngineFilterSchema.optional(), | ||
| /** Fields to select */ | ||
| select: z.array(z.string()).optional(), | ||
| /** Sort order */ | ||
| sort: z.record(z.union([z.literal(1), z.literal(-1), z.literal('asc'), z.literal('desc')])).optional(), | ||
| /** Limit number of results (alternative name for top, used by some drivers) */ | ||
| limit: z.number().optional(), | ||
| /** Skip number of results (for pagination) */ | ||
| skip: z.number().optional(), | ||
| /** Maximum number of results (OData-style, takes precedence over limit if both specified) */ | ||
| top: z.number().optional(), | ||
| }).describe('Query options for IDataEngine.find() operations'); | ||
|
|
||
| /** | ||
| * Data Engine Interface Schema | ||
| * | ||
| * Defines the contract for data engine implementations. | ||
| */ | ||
| export const DataEngineSchema = z.object({ | ||
| /** | ||
| * Insert a new record | ||
| * | ||
| * @param objectName - Name of the object/table (e.g., 'user', 'order') | ||
| * @param data - Data to insert | ||
| * @returns Promise resolving to the created record (including generated ID) | ||
| */ | ||
| insert: z.function() | ||
| .args(z.string(), z.any()) | ||
| .returns(z.promise(z.any())) | ||
| .describe('Insert a new record'), | ||
|
|
||
| /** | ||
| * Find records matching a query | ||
| * | ||
| * @param objectName - Name of the object/table | ||
| * @param query - Query conditions (optional) | ||
| * @returns Promise resolving to an array of matching records | ||
| */ | ||
| find: z.function() | ||
| .args(z.string()) | ||
| .returns(z.promise(z.array(z.any()))) | ||
| .describe('Find records matching a query'), | ||
|
|
||
| /** | ||
| * Update a record by ID | ||
| * | ||
| * @param objectName - Name of the object/table | ||
| * @param id - Record ID | ||
| * @param data - Updated data (partial update) | ||
| * @returns Promise resolving to the updated record | ||
| */ | ||
| update: z.function() | ||
| .args(z.string(), z.any(), z.any()) | ||
| .returns(z.promise(z.any())) | ||
| .describe('Update a record by ID'), | ||
|
|
||
| /** | ||
| * Delete a record by ID | ||
| * | ||
| * @param objectName - Name of the object/table | ||
| * @param id - Record ID | ||
| * @returns Promise resolving to true if deleted, false otherwise | ||
| */ | ||
| delete: z.function() | ||
| .args(z.string(), z.any()) | ||
| .returns(z.promise(z.boolean())) | ||
| .describe('Delete a record by ID'), | ||
| }).describe('Data Engine Interface'); | ||
|
|
||
| /** | ||
| * TypeScript types derived from schemas | ||
| */ | ||
| export type DataEngineFilter = z.infer<typeof DataEngineFilterSchema>; | ||
| export type DataEngineQueryOptions = z.infer<typeof DataEngineQueryOptionsSchema>; | ||
|
|
||
| // Define the TypeScript interface manually for better type safety | ||
| // Zod function schema doesn't handle optional parameters well | ||
| export interface IDataEngine { | ||
| insert(objectName: string, data: any): Promise<any>; | ||
| find(objectName: string, query?: DataEngineQueryOptions): Promise<any[]>; | ||
| update(objectName: string, id: any, data: any): Promise<any>; | ||
| delete(objectName: string, id: any): Promise<boolean>; | ||
| } |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the established testing pattern in this codebase (as seen in driver.test.ts, datasource.test.ts, audit.test.ts, etc.), a test file should be created for the data engine schema to validate the schema definitions.
Create a new file packages/spec/src/system/data-engine.test.ts that validates the DataEngineQueryOptionsSchema, DataEngineFilterSchema, and the IDataEngine interface implementation. This ensures the schema behaves as expected and provides examples of valid/invalid data structures.
| */ | ||
|
|
||
| import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, QueryOptions } from './index.js'; | ||
| import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, DataEngineQueryOptions } from './index.js'; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused imports IHttpRequest, IHttpResponse.
| import { IHttpServer, IDataEngine, RouteHandler, IHttpRequest, IHttpResponse, Middleware, DataEngineQueryOptions } from './index.js'; | |
| import { IHttpServer, IDataEngine, RouteHandler, Middleware, DataEngineQueryOptions } from './index.js'; |
| import { ObjectKernel } from './packages/runtime/src/mini-kernel.js'; | ||
| import { ObjectQLPlugin } from './packages/runtime/src/objectql-plugin.js'; | ||
| import { DriverPlugin } from './packages/runtime/src/driver-plugin.js'; | ||
| import { ObjectQL } from './packages/objectql/src/index.js'; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import ObjectQL.
| import { ObjectQL } from './packages/objectql/src/index.js'; |
| // Verify objectql service is still available (backward compatibility) | ||
| try { | ||
| const ql = kernel.getService('objectql'); | ||
| console.log('✅ objectql service still available (backward compatibility)'); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable ql.
| console.log('✅ objectql service still available (backward compatibility)'); | |
| console.log('✅ objectql service still available (backward compatibility). Service type:', ql.constructor.name); |
| */ | ||
|
|
||
| import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from '../packages/runtime/src'; | ||
| import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from './packages/runtime/src/index.js'; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import SchemaRegistry.
| import { ObjectKernel, ObjectQLPlugin, ObjectQL, SchemaRegistry } from './packages/runtime/src/index.js'; | |
| import { ObjectKernel, ObjectQLPlugin, ObjectQL } from './packages/runtime/src/index.js'; |
Summary
Successfully implemented the IDataEngine interface for ObjectQL following the three-layer architecture pattern:
Consumer → DataEngine (ObjectQL) → Driver
Confirmed
data-engine.zod.tsshould remain inspec/system/(infrastructure layer) notspec/data/(business metadata layer), following the pattern where system/ contains runtime interfaces like DriverInterface and PluginInterface.All tests passing ✅
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.