feat: implement @objectstack/service-analytics with multi-driver strategy pattern#946
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…tegy pattern - AnalyticsService orchestrator implementing IAnalyticsService - Strategy pattern: NativeSQLStrategy (P1), ObjectQLStrategy (P2), InMemoryStrategy (P3) - CubeRegistry with auto-inference from object schemas - AnalyticsServicePlugin for kernel lifecycle - generateSql() dry-run SQL generation - 34 unit tests covering all strategy branches - Updated ROADMAP.md and CHANGELOG.md Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/ad90f5d5-0180-49f2-9b9b-72bf2792ed93
… redundant ternary Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/ad90f5d5-0180-49f2-9b9b-72bf2792ed93
|
@copilot InMemoryStrategy 是否应该在 memory driver 中定义。还有 注册 analytics 服务是否也应该在service-analytics, 而不是 memory driver ? |
…to spec - Move AnalyticsStrategy, StrategyContext, DriverCapabilities interfaces to @objectstack/spec/contracts (Protocol First principle) - Move InMemoryStrategy from service-analytics to driver-memory - Replace built-in InMemoryStrategy with internal FallbackDelegateStrategy in AnalyticsService (auto-added when fallbackService is configured) - service-analytics/strategies/types.ts now re-exports from spec - Analytics service registration remains in service-analytics (already correct) - All tests pass: 30 service-analytics, 94 driver-memory, 6704 spec Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/94c99192-9b5e-4b97-809a-902bc64293c3
Agreed on both points. In
|
There was a problem hiding this comment.
Pull request overview
Adds @objectstack/service-analytics as a production-ready IAnalyticsService implementation, introducing a multi-driver strategy chain (Native SQL / ObjectQL / fallback delegation) and a cube registry to enable cross-driver analytics querying and SQL generation.
Changes:
- Introduces
AnalyticsServiceorchestrator +AnalyticsServicePlugin, with a priority-ordered strategy chain. - Adds
NativeSQLStrategyandObjectQLStrategy, plus aCubeRegistrywith object-field inference. - Extends spec contracts with strategy-chain types and exports an
InMemoryStrategyfrom@objectstack/driver-memory; updates changelogs/roadmap.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| ROADMAP.md | Marks analytics service as completed and documents new capabilities/tests. |
| pnpm-lock.yaml | Adds lockfile entries for the new service package. |
| packages/spec/src/contracts/analytics-service.ts | Adds strategy-pattern contract types alongside IAnalyticsService. |
| packages/services/service-analytics/tsconfig.json | New TS config for the service package. |
| packages/services/service-analytics/src/strategies/types.ts | Re-exports strategy contract types from @objectstack/spec/contracts. |
| packages/services/service-analytics/src/strategies/objectql-strategy.ts | Implements ObjectQL-based aggregation strategy. |
| packages/services/service-analytics/src/strategies/native-sql-strategy.ts | Implements native SQL pushdown strategy + SQL generator. |
| packages/services/service-analytics/src/plugin.ts | Adds kernel plugin that registers/replaces the analytics service with fallback capture. |
| packages/services/service-analytics/src/index.ts | Barrel exports for service/plugin/registry/strategies. |
| packages/services/service-analytics/src/cube-registry.ts | Adds cube registry + schema inference helper. |
| packages/services/service-analytics/src/analytics-service.ts | Orchestrator that selects strategies and exposes query/getMeta/generateSql. |
| packages/services/service-analytics/src/tests/analytics-service.test.ts | Unit tests for registry, strategies, fallback delegation, orchestration. |
| packages/services/service-analytics/package.json | New package manifest for @objectstack/service-analytics. |
| packages/services/service-analytics/CHANGELOG.md | New package changelog entry documenting the initial release. |
| packages/plugins/driver-memory/src/index.ts | Exports InMemoryStrategy from the memory driver package. |
| packages/plugins/driver-memory/src/in-memory-strategy.ts | Adds an in-memory fallback-delegating strategy implementation. |
| CHANGELOG.md | Documents the new analytics service in the monorepo changelog. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| return caps.objectqlAggregate && typeof ctx.executeAggregate === 'function'; | ||
| } | ||
|
|
||
| async execute(query: AnalyticsQuery, ctx: StrategyContext): Promise<AnalyticsResult> { | ||
| const cube = ctx.getCube(query.cube!)!; |
There was a problem hiding this comment.
canHandle() doesn't verify that the cube is actually registered/available via ctx.getCube(). If queryCapabilities() returns objectqlAggregate: true for an unknown cube, this strategy will be selected and execute() will crash on ctx.getCube(query.cube!)!. Consider returning false when ctx.getCube(query.cube) is undefined, and have execute() throw a clear "Cube not found" error instead of relying on non-null assertions (this also allows fallback strategies to run for unknown cubes).
| return caps.objectqlAggregate && typeof ctx.executeAggregate === 'function'; | |
| } | |
| async execute(query: AnalyticsQuery, ctx: StrategyContext): Promise<AnalyticsResult> { | |
| const cube = ctx.getCube(query.cube!)!; | |
| if (!caps.objectqlAggregate || typeof ctx.executeAggregate !== 'function') { | |
| return false; | |
| } | |
| if (typeof ctx.getCube !== 'function') { | |
| return false; | |
| } | |
| const cube = ctx.getCube(query.cube); | |
| if (!cube) { | |
| return false; | |
| } | |
| return true; | |
| } | |
| async execute(query: AnalyticsQuery, ctx: StrategyContext): Promise<AnalyticsResult> { | |
| if (!query.cube) { | |
| throw new Error('Cube not specified in analytics query'); | |
| } | |
| if (typeof ctx.getCube !== 'function') { | |
| throw new Error('Cube registry is not available in strategy context'); | |
| } | |
| const cube = ctx.getCube(query.cube); | |
| if (!cube) { | |
| throw new Error(`Cube not found: ${query.cube}`); | |
| } |
| if (operator === 'set') return { $ne: null }; | ||
| if (operator === 'notSet') return null; | ||
| if (!values || values.length === 0) return undefined; | ||
|
|
||
| switch (operator) { | ||
| case 'equals': return values[0]; | ||
| case 'notEquals': return { $ne: values[0] }; | ||
| case 'gt': return { $gt: values[0] }; | ||
| case 'gte': return { $gte: values[0] }; | ||
| case 'lt': return { $lt: values[0] }; | ||
| case 'lte': return { $lte: values[0] }; | ||
| case 'contains': return { $regex: values[0] }; | ||
| default: return values[0]; |
There was a problem hiding this comment.
convertFilter() is emitting Mongo/Mingo-style operators like $regex and returning raw values, but ObjectQL filter conventions in this repo use $contains/$notContains (and then drivers normalize as needed). Using $regex directly is likely to break on non-memory drivers, and returning undefined can leave filter[field]=undefined while still sending a non-empty filter object. Suggest mapping to the canonical operators ($eq, $ne, $gt, $gte, $lt, $lte, $contains, $notContains, etc.) and skipping filters that have no effective value.
| if (operator === 'set') return { $ne: null }; | |
| if (operator === 'notSet') return null; | |
| if (!values || values.length === 0) return undefined; | |
| switch (operator) { | |
| case 'equals': return values[0]; | |
| case 'notEquals': return { $ne: values[0] }; | |
| case 'gt': return { $gt: values[0] }; | |
| case 'gte': return { $gte: values[0] }; | |
| case 'lt': return { $lt: values[0] }; | |
| case 'lte': return { $lte: values[0] }; | |
| case 'contains': return { $regex: values[0] }; | |
| default: return values[0]; | |
| // Canonical "is set" / "is not set" semantics | |
| if (operator === 'set') return { $ne: null }; | |
| if (operator === 'notSet') return { $eq: null }; | |
| // No effective filter if there are no values; let caller decide whether to skip | |
| if (!values || values.length === 0) return undefined; | |
| switch (operator) { | |
| case 'equals': | |
| return { $eq: values[0] }; | |
| case 'notEquals': | |
| return { $ne: values[0] }; | |
| case 'gt': | |
| return { $gt: values[0] }; | |
| case 'gte': | |
| return { $gte: values[0] }; | |
| case 'lt': | |
| return { $lt: values[0] }; | |
| case 'lte': | |
| return { $lte: values[0] }; | |
| case 'contains': | |
| return { $contains: values[0] }; | |
| case 'notContains': | |
| return { $notContains: values[0] }; | |
| default: | |
| // Unknown operator: no filter emitted; caller should skip | |
| return undefined; |
| if (query.measures) { | ||
| for (const m of query.measures) { | ||
| const { field, method } = this.resolveMeasureAggregation(cube, m); | ||
| const aggSql = method === 'count' ? 'COUNT(*)' : `${method.toUpperCase()}(${field})`; |
There was a problem hiding this comment.
generateSql() renders count_distinct as COUNT_DISTINCT(field) via method.toUpperCase(), which isn't valid SQL; it should be represented as COUNT(DISTINCT field). Consider special-casing count_distinct similarly to how NativeSQLStrategy does, so generateSql() outputs syntactically correct SQL for that measure type.
| const aggSql = method === 'count' ? 'COUNT(*)' : `${method.toUpperCase()}(${field})`; | |
| const aggSql = | |
| method === 'count' | |
| ? 'COUNT(*)' | |
| : method === 'count_distinct' | |
| ? `COUNT(DISTINCT ${field})` | |
| : `${method.toUpperCase()}(${field})`; |
| } | ||
| } | ||
|
|
||
| private extractObjectName(cube: Cube): string { |
There was a problem hiding this comment.
extractObjectName() returns cube.sql.trim(), but CubeSchema.sql is documented as "Base SQL statement or Table Name". For the ObjectQL aggregate path, the engine typically expects an ObjectQL object name (likely cube.name), not an arbitrary SQL string. If cube.sql is a SELECT/subquery or differs from cube.name, executeAggregate() will be invoked with an invalid object name.
| private extractObjectName(cube: Cube): string { | |
| private extractObjectName(cube: Cube): string { | |
| // Prefer the logical cube name for ObjectQL object resolution. | |
| if (cube.name && cube.name.trim().length > 0) { | |
| return cube.name.trim(); | |
| } | |
| // Fallback to the SQL/table name only if no explicit name is provided. |
| const tableName = this.extractObjectName(cube); | ||
| let sql = `SELECT ${selectClauses.join(', ')} FROM "${tableName}"`; | ||
| if (whereClauses.length > 0) { | ||
| sql += ` WHERE ${whereClauses.join(' AND ')}`; | ||
| } | ||
| if (groupByClauses.length > 0) { | ||
| sql += ` GROUP BY ${groupByClauses.join(', ')}`; | ||
| } |
There was a problem hiding this comment.
CubeSchema.sql is defined as "Base SQL statement or Table Name", but this strategy treats cube.sql as a simple table identifier and wraps it as FROM "${tableName}". This will break for valid cube definitions where sql is a full SELECT/join or already-quoted/qualified identifier, and it also prevents using schema.table without changing semantics. Consider supporting cube.sql as an arbitrary SQL source (e.g., FROM (${cube.sql}) AS <alias>) or clearly restricting/validating it to a safe table name before quoting.
| async getMeta(cubeName?: string): Promise<CubeMeta[]> { | ||
| // If a fallback service is configured, merge its metadata with the registry | ||
| const cubes = cubeName | ||
| ? [this.cubeRegistry.get(cubeName)].filter(Boolean) as Cube[] | ||
| : this.cubeRegistry.getAll(); | ||
|
|
There was a problem hiding this comment.
getMeta() comment says fallback metadata is merged, but the implementation ignores fallbackService entirely. In the common case where AnalyticsServicePlugin replaces an existing (memory) analytics service as fallbackService without passing cubes, getMeta() will return [] even though queries may still work via fallback delegation. Consider merging await fallbackService.getMeta(cubeName) with cubeRegistry metadata (dedupe by cube name), or at least documenting that callers must register cubes in cubeRegistry for discovery.
| * - `number` fields → `sum`, `avg`, `min`, `max` measures | ||
| * - `boolean` fields → `count` measure (count where true) | ||
| * - All non-computed fields → dimensions | ||
| * - `date`/`datetime` fields → time dimensions with standard granularities |
There was a problem hiding this comment.
The heuristic rules in the doc comment don't match the implementation: it claims boolean fields become a "count where true" measure and numeric fields get sum/avg/min/max, but the code only adds dimensions for booleans and only creates sum and avg measures for numeric fields. Either update the comment to reflect current behavior or implement the missing measure inference to avoid misleading consumers.
| * - `number` fields → `sum`, `avg`, `min`, `max` measures | |
| * - `boolean` fields → `count` measure (count where true) | |
| * - All non-computed fields → dimensions | |
| * - `date`/`datetime` fields → time dimensions with standard granularities | |
| * - `number`/`currency`/`percent` fields → `sum`, `avg` measures | |
| * - All fields → dimensions (typed via `fieldTypeToDimensionType`) | |
| * - `date`/`datetime` fields → time dimensions with standard granularities | |
| * - `boolean` fields → boolean dimensions (no automatic measures) |
| ctx.hook('analytics:beforeQuery', async (query: unknown) => { | ||
| ctx.logger.debug('[Analytics] Before query', { query }); | ||
| }); |
There was a problem hiding this comment.
debug mode registers an analytics:beforeQuery hook, but there are no ctx.trigger('analytics:beforeQuery', ...) call sites in the repo. As a result this hook never runs and the option is effectively dead. Consider either removing this hook registration or adding an explicit trigger in the analytics request path (e.g., wherever IAnalyticsService.query() is invoked) so the debug logging actually executes.
| ctx.hook('analytics:beforeQuery', async (query: unknown) => { | |
| ctx.logger.debug('[Analytics] Before query', { query }); | |
| }); | |
| ctx.logger.debug('[Analytics] Debug mode enabled'); | |
| const cubeNames = this.options.cubes?.map((cube) => cube.name) ?? []; | |
| ctx.logger.debug('[Analytics] Registered cubes', { cubes: cubeNames }); |
| - Strategy pattern with priority chain: | ||
| - **P1 — NativeSQLStrategy**: Pushes queries as native SQL to SQL-capable drivers (Postgres, MySQL, etc.) | ||
| - **P2 — ObjectQLStrategy**: Translates analytics queries into ObjectQL `engine.aggregate()` calls | ||
| - **P3 — InMemoryStrategy**: Delegates to any registered `IAnalyticsService` (e.g., `MemoryAnalyticsService`) |
There was a problem hiding this comment.
The changelog entry lists P3 — InMemoryStrategy as part of @objectstack/service-analytics, but in this PR InMemoryStrategy lives in @objectstack/driver-memory and the service itself auto-adds an internal FallbackDelegateStrategy. To avoid confusing consumers, consider updating the changelog wording to reflect the actual exported/public strategies and the fallback delegation behavior.
| - Strategy pattern with priority chain: | |
| - **P1 — NativeSQLStrategy**: Pushes queries as native SQL to SQL-capable drivers (Postgres, MySQL, etc.) | |
| - **P2 — ObjectQLStrategy**: Translates analytics queries into ObjectQL `engine.aggregate()` calls | |
| - **P3 — InMemoryStrategy**: Delegates to any registered `IAnalyticsService` (e.g., `MemoryAnalyticsService`) | |
| - Strategy pattern with priority chain (built-in strategies): | |
| - **P1 — NativeSQLStrategy**: Pushes queries as native SQL to SQL-capable drivers (Postgres, MySQL, etc.) | |
| - **P2 — ObjectQLStrategy**: Translates analytics queries into ObjectQL `engine.aggregate()` calls | |
| - **P3 — FallbackDelegateStrategy**: Internal delegating strategy that forwards to any additional `IAnalyticsService` implementations (for example, `InMemoryStrategy` from `@objectstack/driver-memory`) |
| // Can handle when a fallback service exists | ||
| if (ctx.fallbackService) return true; | ||
| // Or when the driver is flagged as in-memory | ||
| const caps = ctx.queryCapabilities(query.cube); | ||
| return caps.inMemory; |
There was a problem hiding this comment.
canHandle() returns true when caps.inMemory is true even if ctx.fallbackService is missing, but execute() will then throw "No fallback analytics service available". This makes the strategy claim it can handle a query when it cannot. Consider making canHandle() require ctx.fallbackService (or alternatively implement a non-fallback execution path for true in-memory drivers).
| // Can handle when a fallback service exists | |
| if (ctx.fallbackService) return true; | |
| // Or when the driver is flagged as in-memory | |
| const caps = ctx.queryCapabilities(query.cube); | |
| return caps.inMemory; | |
| // Strategy can only operate when a fallback service is available | |
| const caps = ctx.queryCapabilities(query.cube); | |
| // Prefer in-memory drivers, but always require a fallback service | |
| if (caps.inMemory && ctx.fallbackService) return true; | |
| // Fallback-only path (preserves existing behavior when a fallback is registered) | |
| return !!ctx.fallbackService; |
Analytics only had
MemoryAnalyticsService— no path to Postgres/Mongo/SQL-backed aggregation. This adds@objectstack/service-analyticsas the productionIAnalyticsServiceimplementation with driver-aware strategy dispatch.Architecture
engine.aggregate()AST for any ObjectQL driverfallbackServiceis configured; delegates to any pre-registeredIAnalyticsService@objectstack/driver-memory(not in service-analytics), for explicit use with the memory driverAnalyticsStrategy,StrategyContext,DriverCapabilities) — Defined in@objectstack/spec/contracts/analytics-service.tsfollowing Protocol First principlereplaceService. Analytics service registration is owned byservice-analytics, not by any driver.nativeSql,objectqlAggregate,inMemory) drives strategy selection at runtimeUsage
Files
packages/services/service-analytics/— Orchestrator, NativeSQLStrategy, ObjectQLStrategy, cube registry, plugin, barrel exportspackages/plugins/driver-memory/src/in-memory-strategy.ts— InMemoryStrategy (moved here from service-analytics per architectural feedback)packages/spec/src/contracts/analytics-service.ts— Strategy contract types added alongside IAnalyticsServiceROADMAP.md— Analytics service status updated from 🟡 → ✅CHANGELOG.md— Documented under[Unreleased]Original prompt
⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.