refactor: unify discovery protocol — remove redundant capabilities, fix route inconsistencies#622
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…nts to routes, fix route inconsistencies - Remove `features` (ApiCapabilitiesSchema) from DiscoverySchema — redundant with services - Remove `capabilities` from GetDiscoveryResponseSchema — redundant with services - Rename `endpoints` → `routes` in GetDiscoveryResponseSchema for consistency with DiscoverySchema - Make `services` required in DiscoverySchema (was optional) - Fix route inconsistencies: core services now use /api/v1/ prefix consistently - Deprecate ApiCapabilitiesSchema (kept for backward compat) - Update objectql protocol implementation - Update client to use routes instead of endpoints - Update REST server to use routes - Update all tests, examples, and documentation Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… convention, remove type assertion Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the discovery protocol to remove redundancy and establish consistency across the ObjectStack API. It addresses three key issues: (1) capabilities/features fields were fully derivable from the services map, (2) inconsistent naming between endpoints and routes across different schemas, and (3) core service routes using /api/ without the /v1/ version prefix while plugin routes used /api/v1/.
Changes:
- Unified route naming from
endpointstoroutesacross all schemas and implementations - Removed redundant
capabilities/featuresfields; deprecatedApiCapabilitiesSchemafor backward compatibility - Made
servicesrequired as the single source of truth for service availability - Standardized all API routes to use
/api/v1/prefix (except GraphQL which uses/graphqlby industry convention)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/spec/src/api/protocol.zod.ts | Removed capabilities field, renamed endpoints to routes in GetDiscoveryResponseSchema, updated documentation |
| packages/spec/src/api/discovery.zod.ts | Deprecated ApiCapabilitiesSchema, removed features from DiscoverySchema, made services required, added design rationale comments |
| packages/spec/src/api/discovery.test.ts | Updated tests to remove features field usage, added tests for required services field and capability derivation, updated test descriptions |
| packages/rest/src/rest-server.ts | Renamed discovery.endpoints to discovery.routes throughout route override logic |
| packages/objectql/src/protocol.ts | Updated SERVICE_CONFIG to remove capability field, changed core service routes to use /api/v1/ prefix, removed capabilities building logic, renamed endpoints to routes |
| packages/objectql/src/protocol-discovery.test.ts | Updated all tests to use routes instead of endpoints, removed capabilities assertions, added test for route prefix consistency and capabilities removal |
| packages/client/tests/integration/01-discovery.test.ts | Updated discovery response assertions to check routes and services instead of capabilities and endpoints |
| packages/client/src/index.ts | Changed logging and routing logic to use services and routes instead of capabilities and endpoints |
| examples/minimal-auth/src/test-discovery.ts | Updated console logging to use routes.auth instead of endpoints.auth |
| content/docs/references/api/discovery.mdx | Updated documentation to reflect removal of ApiCapabilities, focus on services as source of truth, and clarify routes as convenience map |
|
|
||
| expect(discovery.features.graphql).toBe(true); | ||
| expect(discovery.services['graphql'].enabled).toBe(true); | ||
| expect(discovery.routes.graphql).toBe('/api/v1/graphql'); |
There was a problem hiding this comment.
Test assertion expects GraphQL route to be /api/v1/graphql, but the convention established in this PR is that GraphQL should use /graphql (industry standard, not versioned REST). This assertion should expect /graphql instead to align with the route set on line 390 above, which should also be corrected to /graphql.
| capabilities: ApiCapabilitiesSchema.optional().describe('Supported features/capabilities'), | ||
| endpoints: ApiRoutesSchema.optional().describe('Available endpoint paths'), | ||
| routes: ApiRoutesSchema.optional().describe('Available endpoint paths'), | ||
| services: z.record(z.string(), ServiceInfoSchema).optional().describe('Per-service availability map'), |
There was a problem hiding this comment.
Inconsistency between DiscoverySchema and GetDiscoveryResponseSchema: In discovery.zod.ts, the services field is required (line 126), but in protocol.zod.ts, the same field is marked as optional (line 101).
Since the PR description states that "services is now the single source of truth" and the tests require it to be present, this field should be required in both schemas for consistency. The GetDiscoveryResponseSchema should be updated to make services required to match DiscoverySchema.
| services: z.record(z.string(), ServiceInfoSchema).optional().describe('Per-service availability map'), | |
| services: z.record(z.string(), ServiceInfoSchema).describe('Per-service availability map'), |
| graphql: '/api/v1/graphql', | ||
| }, | ||
| features: { | ||
| graphql: true, | ||
| search: true, | ||
| websockets: true, | ||
| files: true, | ||
| services: { | ||
| ...minimalServices, | ||
| graphql: { enabled: true, status: 'available' as const, route: '/api/v1/graphql', provider: 'plugin-graphql' }, |
There was a problem hiding this comment.
GraphQL route should use /graphql (industry convention), not /api/v1/graphql. According to the PR description and the code comment in packages/objectql/src/protocol.ts:46, GraphQL is intentionally excluded from the /api/v1/ prefix pattern because it follows industry convention. The route in this test should be changed from /api/v1/graphql to /graphql to be consistent with the established convention.
| graphql: '/api/v1/graphql', | ||
| }, | ||
| features: { | ||
| graphql: true, | ||
| search: true, | ||
| websockets: false, | ||
| files: true, | ||
| services: { | ||
| ...minimalServices, | ||
| graphql: { enabled: true, status: 'available' as const, route: '/api/v1/graphql', provider: 'plugin-graphql' }, |
There was a problem hiding this comment.
GraphQL route should use /graphql (industry convention), not /api/v1/graphql. According to the PR description and the code comment in packages/objectql/src/protocol.ts:46, GraphQL is intentionally excluded from the /api/v1/ prefix pattern because it follows industry convention. The route in this test should be changed from /api/v1/graphql to /graphql to be consistent with the established convention.
Discovery response had three problems:
capabilities/featureswas fully derivable fromservices[x].enabled,endpointsvsroutesnaming was inconsistent betweenDiscoverySchemaandGetDiscoveryResponseSchema, and core service routes used/api/datawhile plugin routes used/api/v1/data.Schema changes (
discovery.zod.ts,protocol.zod.ts)featuresfromDiscoverySchemaandcapabilitiesfromGetDiscoveryResponseSchema— redundant withservicesendpoints→routesinGetDiscoveryResponseSchemato align withDiscoverySchemaservicesrequired (was optional) — it's now the single source of truthApiCapabilitiesSchema(export kept for backward compat instack.zod.ts)Route consistency (
objectql/protocol.ts)data,metadata,analytics) now use/api/v1/prefix, matching plugin services/graphql(industry convention, not a versioned REST endpoint)Consumer updates
client/src/index.ts— readroutesinstead ofendpointsrest/src/rest-server.ts— usediscovery.routesBefore/After
💡 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.