feat(core): add replaceService to PluginContext#597
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…gration hooks Add replaceService<T>(name, implementation) to PluginContext interface, enabling plugins to replace/enhance kernel internals (metadata registry, hook manager, connection pooling) using the decorator pattern. Implements upstream spec change 5.1.2 from objectql DESIGN_CORE_REFACTOR.md. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a replaceService() API to PluginContext so plugins (notably optimization/decorator-style plugins) can swap existing kernel services at runtime, aligning core with the upstream refactor spec.
Changes:
- Extend
PluginContextwithreplaceService<T>(name, implementation)and wire it throughObjectKernel,ObjectKernelBase(LiteKernel),PluginLoader, andSecurePluginContext. - Implement existence validation + replacement behavior across the kernel and loader.
- Add new tests in
kernel.test.tsandlite-kernel.test.tscovering basic replacement and error cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/types.ts | Adds PluginContext.replaceService() to the public interface. |
| packages/core/src/kernel.ts | Implements replaceService() in the primary kernel context with dual-write + logging. |
| packages/core/src/kernel-base.ts | Implements replaceService() for LiteKernel / base-kernel context. |
| packages/core/src/plugin-loader.ts | Adds replaceService() to update loader-held service instances with existence validation. |
| packages/core/src/security/plugin-permission-enforcer.ts | Exposes replaceService() via SecurePluginContext with permission enforcement. |
| packages/core/src/kernel.test.ts | Adds ObjectKernel tests for service replacement + decorator pattern. |
| packages/core/src/lite-kernel.test.ts | Adds LiteKernel tests for service replacement + missing-service error. |
| * @param name - Service name to replace | ||
| * @param implementation - New service implementation | ||
| * @throws Error if the service does not exist | ||
| */ | ||
| replaceService<T>(name: string, implementation: T): void; |
There was a problem hiding this comment.
PluginContext now requires replaceService, but several typed test mocks in the repo construct PluginContext objects without this method (e.g. packages/plugins/plugin-auth/src/auth-plugin.test.ts and packages/core/src/security/plugin-permission-enforcer.test.ts). This will cause TypeScript compile errors; update those mocks to include a replaceService stub (or use a helper factory for PluginContext test doubles).
| * @param name - Service name to replace | |
| * @param implementation - New service implementation | |
| * @throws Error if the service does not exist | |
| */ | |
| replaceService<T>(name: string, implementation: T): void; | |
| * NOTE: This is optional for backward compatibility with existing plugins | |
| * and test contexts that may not implement service replacement. | |
| * | |
| * @param name - Service name to replace | |
| * @param implementation - New service implementation | |
| * @throws Error if the service does not exist | |
| */ | |
| replaceService?<T>(name: string, implementation: T): void; |
| // IServiceRegistry implementation | ||
| if (!this.services.has(name)) { | ||
| throw new Error(`[Kernel] Service '${name}' not found. Use registerService() to add new services.`); | ||
| } |
There was a problem hiding this comment.
In the IServiceRegistry branch, replaceService calls this.services.register(name, implementation) after confirming the service already exists. IServiceRegistry.register() is specified to throw if the name is already registered, so this path will fail whenever services is an IServiceRegistry. Use unregister(name) (or a dedicated replace/set API) before re-registering.
| } | |
| } | |
| // unregister existing service before re-registering to avoid duplicate registration errors | |
| this.services.unregister(name); |
| replaceService(name: string, service: any): void { | ||
| if (!this.hasService(name)) { | ||
| throw new Error(`Service '${name}' not found`); | ||
| } | ||
| this.serviceInstances.set(name, service); | ||
| } |
There was a problem hiding this comment.
PluginLoader.replaceService() sets serviceInstances, but PluginLoader.getService() always prefers a serviceFactory when one exists. That means replacing a service that was registered via registerServiceFactory() will not take effect for async consumers (getService/getServiceAsync). Consider making getService() prefer an explicitly replaced instance, or have replaceService() remove/override the factory registration and clear any scoped caches for that service.
| replaceService: <T>(name: string, implementation: T): void => { | ||
| const hasService = this.services.has(name) || this.pluginLoader.hasService(name); | ||
| if (!hasService) { | ||
| throw new Error(`[Kernel] Service '${name}' not found. Use registerService() to add new services.`); | ||
| } | ||
| this.services.set(name, implementation); | ||
| this.pluginLoader.replaceService(name, implementation); | ||
| this.logger.info(`Service '${name}' replaced`, { service: name }); |
There was a problem hiding this comment.
ObjectKernel’s replaceService currently allows replacement when the service exists only as a factory (via pluginLoader.hasService). However, without additional changes in PluginLoader, the replacement won’t affect kernel.getServiceAsync() (factory path still wins). Either constrain replaceService to instance-registered services only, or update the loader semantics so a replacement overrides factory resolution too.
| replaceService<T>(name: string, implementation: T): void { | ||
| // Check permission before replacing service | ||
| this.permissionEnforcer.enforceServiceAccess(this.pluginName, name); | ||
| this.baseContext.replaceService(name, implementation); | ||
| } |
There was a problem hiding this comment.
SecurePluginContext.replaceService uses enforceServiceAccess, which appears to represent read/access permission. Replacing a service is a much higher-privilege operation (it can hijack kernel internals even if a plugin can only “access” the service). Consider introducing and enforcing a distinct capability for service replacement (or restrict replacement to trusted/core plugins) rather than reusing the read-access check.
Implements upstream spec change §5.1.2 from objectql DESIGN_CORE_REFACTOR.md. The downstream
OptimizationsPluginneeds to replace/enhance kernel internals (metadata registry, connection pooling, hook manager), butPluginContexthas no service replacement API.§5.1.1 (Query Profiling) required no changes —
OperationContextandEngineMiddlewareare already exported.Changes
PluginContextinterface (types.ts): NewreplaceService<T>(name, implementation)method — throws if service doesn't existObjectKernelBase(kernel-base.ts): Implementation forLiteKernelpath (Map + IServiceRegistry)ObjectKernel(kernel.ts): Implementation with dual-write to services Map + PluginLoader (consistent with existingregisterServicepattern)PluginLoader(plugin-loader.ts):replaceServicewith existence validationSecurePluginContext(plugin-permission-enforcer.ts): Delegates through permission enforcement viaenforceServiceAccesskernel.test.tsandlite-kernel.test.ts(basic replace, missing service error, decorator pattern)Usage — decorator pattern for optimization plugins
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.