Conversation
- Fix packageId access in getDriver method by using SchemaRegistry.getObjectOwner() - ServiceObject type does not have packageId, it's in ObjectContributor - Resolves build errors: "Property 'packageId' does not exist on type ServiceObject" Agent-Logs-Url: https://github.com/objectstack-ai/framework/sessions/93e8e1d7-431c-457a-b084-4e5cc358b097 Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Agent-Logs-Url: https://github.com/objectstack-ai/framework/sessions/93e8e1d7-431c-457a-b084-4e5cc358b097 Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes TypeScript type errors in ObjectQL datasource resolution by switching from ServiceObject.packageId access to looking up the owning contributor in SchemaRegistry, aiming to unblock CI builds.
Changes:
- Updated ObjectQL engine datasource resolution to use
SchemaRegistry.getObjectOwner()for package manifest lookup. - Tightened typing/default handling in
AuthManager.getPublicConfig()by using explicit config types and nullish coalescing defaults. - Expanded auth config type imports from
@objectstack/spec/system.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/plugins/plugin-auth/src/auth-manager.ts | Refines getPublicConfig() typing/defaults and updates imported auth config types. |
| packages/objectql/src/engine.ts | Fixes TS2339 by resolving packageId from the object’s owner contributor for manifest-based datasource routing. |
| const emailPasswordConfig: Partial<EmailAndPasswordConfig> = this.config.emailAndPassword ?? {}; | ||
| const emailPassword = { | ||
| enabled: emailPasswordConfig.enabled !== false, // Default to true | ||
| disableSignUp: emailPasswordConfig.disableSignUp, | ||
| requireEmailVerification: emailPasswordConfig.requireEmailVerification, | ||
| disableSignUp: emailPasswordConfig.disableSignUp ?? false, | ||
| requireEmailVerification: emailPasswordConfig.requireEmailVerification ?? false, |
There was a problem hiding this comment.
EmailAndPasswordConfig is inferred from a Zod .optional() schema (T | undefined). Using Partial<EmailAndPasswordConfig> collapses to {} (because keyof (T | undefined) becomes never), so emailPasswordConfig.enabled/disableSignUp/... will not type-check. Use Partial<NonNullable<EmailAndPasswordConfig>> (or Partial<NonNullable<AuthConfig['emailAndPassword']>>) for the local variable typing.
| // 3. Check package's defaultDatasource | ||
| if (object?.packageId) { | ||
| const manifest = this.manifests.get(object.packageId); | ||
| const owner = SchemaRegistry.getObjectOwner(objectName); |
There was a problem hiding this comment.
SchemaRegistry.getObjectOwner() only looks up by FQN key (it does not resolve short names or physical tableName). In getDriver(), objectName is often the resolved physical table name (via resolveObjectName()), so getObjectOwner(objectName) will usually return undefined and skip package defaultDatasource routing. Use the resolved schema's FQN (object?.name) when calling getObjectOwner, or add a registry helper that resolves owner from any object identifier.
| const owner = SchemaRegistry.getObjectOwner(objectName); | |
| const owner = SchemaRegistry.getObjectOwner(object?.name ?? objectName); |
| const owner = SchemaRegistry.getObjectOwner(objectName); | ||
| if (owner?.packageId) { | ||
| const manifest = this.manifests.get(owner.packageId); | ||
| if (manifest?.defaultDatasource && manifest.defaultDatasource !== 'default') { | ||
| if (this.drivers.has(manifest.defaultDatasource)) { | ||
| this.logger.debug('Resolved datasource from package manifest', { | ||
| object: objectName, | ||
| package: object.packageId, | ||
| package: owner.packageId, | ||
| datasource: manifest.defaultDatasource | ||
| }); | ||
| return this.drivers.get(manifest.defaultDatasource)!; |
There was a problem hiding this comment.
This change alters datasource routing logic but there are no tests asserting that package defaultDatasource resolution works when callers pass a short object name or a physical tableName (both are used elsewhere via resolveObjectName). Adding a focused unit test around getDriver()/find() would prevent regressions in datasource mapping.
Summary
Fixes TypeScript compilation errors (TS2339) in
packages/objectql/src/engine.tsthat were blocking CI builds.Problem
The code was attempting to access
object.packageIdwhereobjectis of typeServiceObject, butpackageIdis not a property ofServiceObject- it's a property ofObjectContributor.Error:
Solution
Changed from accessing
object.packageId(incorrect) to usingSchemaRegistry.getObjectOwner(objectName)?.packageId(correct), which properly retrieves the packageId from the object's owner contributor.Changes
resolveDatasource()method (lines 620-634)object?.packageIdtoowner?.packageIdwhereowner = SchemaRegistry.getObjectOwner(objectName)Testing