diff --git a/src/parser/schemas.ts b/src/parser/schemas.ts index 28a0be5..5b76d8c 100644 --- a/src/parser/schemas.ts +++ b/src/parser/schemas.ts @@ -269,6 +269,16 @@ function extractNestedSchema(name: string, schema: SchemaObject): Model[] { // Handle oneOf: extract each object variant as its own model if (schema.oneOf) { const models: Model[] = []; + + // Collect the inline object variants up front. When every variant pins + // the same property to a const value, we can derive meaningful names + // from the const (e.g. `UserApiKeyOwner` instead of `ApiKeyOwner2`) + // — much friendlier for SDK consumers than numeric suffixes. + const inlineObjectVariants: SchemaObject[] = schema.oneOf.filter( + (v) => !v.$ref && v.properties && (v.type === 'object' || !v.type), + ); + const constNamingDiscriminator = deriveConstNamingDiscriminator(inlineObjectVariants); + for (const variant of schema.oneOf) { if (variant.$ref) continue; if (variant.properties && (variant.type === 'object' || !variant.type)) { @@ -278,8 +288,7 @@ function extractNestedSchema(name: string, schema: SchemaObject): Model[] { if (!fs) continue; fields.push(buildFieldFromSchema(fn, fs, name, requiredSet)); } - // Use the name for the first variant, suffix for subsequent - const variantName = models.length === 0 ? name : `${name}${models.length + 1}`; + const variantName = nameVariantModel(variant, name, models, constNamingDiscriminator); models.push({ name: variantName, description: variant.description, fields }); } } @@ -311,6 +320,80 @@ function extractNestedSchema(name: string, schema: SchemaObject): Model[] { return []; } +/** + * Derive a meaningful name for a oneOf object variant past the first, + * falling back to the numeric-suffix scheme (`Foo2`, `Foo3`) when no + * better signal is available. The "better signal" is a const-valued + * discriminator property that distinguishes one variant from the others. + * + * For a schema like `ApiKey.owner` whose oneOf has two variants — one + * with `type: { const: "organization" }` and one with `type: { const: + * "user" }` — this returns `ApiKeyOwner` for the first variant (the + * baseline name that the union TypeRef will reference, so consumer- + * visible typed fields stay backward-compatible) and `UserApiKeyOwner` + * for the second instead of `ApiKeyOwner2`. SDK consumers reading the + * generated types can tell the variants apart at a glance. + * + * The first variant always keeps the bare parent name because + * `schemaToTypeRef` builds union variants whose model refs collapse to + * `qualifyInlineModelName(field, parent)` — so the first model name + * must match for the union's degenerate-collapse to land on a valid + * type. Renaming variant 0 would orphan it. + * + * Falls back to numeric suffix when: + * - The variant has no discriminator property (no shared const + * property across all object variants), OR + * - The const value PascalCases to an empty/whitespace name, OR + * - The derived name collides with the parent name or an already- + * emitted variant name (numeric keeps uniqueness). + */ +function nameVariantModel( + variant: SchemaObject, + parentName: string, + alreadyEmitted: Model[], + discriminator: { property: string } | null, +): string { + // Variant 0 keeps the bare parent name so the union TypeRef's degenerate + // collapse target exists in the model registry. + if (alreadyEmitted.length === 0) return parentName; + + if (discriminator) { + const constValue = getConstPropertyValue(variant, discriminator.property); + if (constValue) { + const prefix = toPascalCase(constValue); + if (prefix) { + const candidate = parentName.startsWith(prefix) ? parentName : `${prefix}${parentName}`; + const collision = candidate === parentName || alreadyEmitted.some((m) => m.name === candidate); + if (!collision) return candidate; + } + } + } + return `${parentName}${alreadyEmitted.length + 1}`; +} + +/** + * Returns a discriminator descriptor when every inline object variant in a + * oneOf has a string-const value on the same property — the signature of a + * discriminated union without an explicit `discriminator:` keyword. Used by + * `nameVariantModel` to derive variant names from the const values. + * + * Distinct from `detectConstPropertyDiscriminator` (which requires every + * variant to carry a `$ref` or `title` so a name mapping can be built): + * this pass operates on inline anonymous variants, where naming is exactly + * what we're trying to derive. + */ +function deriveConstNamingDiscriminator(variants: SchemaObject[]): { property: string } | null { + if (variants.length < 2) return null; + const candidates = Object.keys(variants[0]?.properties ?? {}); + for (const propName of candidates) { + const values = variants.map((v) => getConstPropertyValue(v, propName)); + if (values.some((v) => v === null)) continue; + if (new Set(values).size !== values.length) continue; // collisions = no signal + return { property: propName }; + } + return null; +} + /** Build a single Field from a schema property entry. Shared across all extraction sites. */ export function buildFieldFromSchema( fieldName: string, diff --git a/test/parser/oneof-variant-naming.test.ts b/test/parser/oneof-variant-naming.test.ts new file mode 100644 index 0000000..0154995 --- /dev/null +++ b/test/parser/oneof-variant-naming.test.ts @@ -0,0 +1,234 @@ +import { describe, it, expect } from 'vitest'; +import { parseSpec } from '../../src/parser/parse.js'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +/** + * Helper: write a spec to a temp file, parse it, return the IR. Cleans up + * the temp dir on completion. + */ +async function parseInline(specYaml: string, fn: (ir: Awaited>) => T): Promise { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'oagen-oneof-naming-')); + const specFile = path.join(tmpDir, 'spec.yml'); + try { + await fs.writeFile(specFile, specYaml, 'utf-8'); + const ir = await parseSpec(specFile); + return fn(ir); + } finally { + await fs.rm(tmpDir, { recursive: true }); + } +} + +describe('oneOf variant naming', () => { + it('derives names from a const-property discriminator instead of using a numeric suffix', async () => { + // Mirrors the workos/openapi-spec ApiKey.owner shape: an inline oneOf + // whose object variants pin the same `type` property to distinct const + // values (`organization`, `user`). Without the const-naming pass the + // emitter would produce `ApiKeyOwner` and `ApiKeyOwner2` — informative + // for a robot, opaque for a human reading the generated SDK. With the + // pass it produces `ApiKeyOwner` (the first variant, kept as the bare + // parent name so the union TypeRef's degenerate-collapse target stays + // valid) and `UserApiKeyOwner` (the second variant, named after its + // discriminator value). + await parseInline( + ` +openapi: 3.1.0 +info: + title: Test + version: 1.0.0 +servers: + - url: https://api.example.com +paths: + /api_keys/{id}: + get: + operationId: getApiKey + parameters: + - name: id + in: path + required: true + schema: { type: string } + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/ApiKey' +components: + schemas: + ApiKey: + type: object + required: [object, id, owner] + properties: + object: { type: string, const: api_key } + id: { type: string } + owner: + oneOf: + - type: object + required: [type, id] + properties: + type: { type: string, const: organization } + id: { type: string } + - type: object + required: [type, id, organization_id] + properties: + type: { type: string, const: user } + id: { type: string } + organization_id: { type: string } +`, + (ir) => { + const ownerModels = ir.models.filter((m) => m.name.endsWith('ApiKeyOwner')); + const ownerNames = ir.models.map((m) => m.name).filter((n) => n.toLowerCase().includes('apikeyowner')); + + // First variant keeps the parent name so the union TypeRef has a + // valid collapse target. + expect(ownerNames).toContain('ApiKeyOwner'); + // Second variant gets a const-derived prefix instead of a `2` suffix. + expect(ownerNames).toContain('UserApiKeyOwner'); + expect(ownerNames).not.toContain('ApiKeyOwner2'); + expect(ownerModels.length).toBeGreaterThanOrEqual(1); + }, + ); + }); + + it('falls back to numeric suffix when no shared const property distinguishes the variants', async () => { + // No discriminator-style property — variants differ structurally + // (different field sets) but neither has a `const` value pinning a + // shared property. Numeric-suffix scheme remains the safe fallback. + await parseInline( + ` +openapi: 3.1.0 +info: { title: Test, version: 1.0.0 } +servers: [{ url: https://api.example.com }] +paths: + /thing: + get: + operationId: getThing + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/Thing' +components: + schemas: + Thing: + type: object + required: [data] + properties: + data: + oneOf: + - type: object + required: [a] + properties: + a: { type: string } + - type: object + required: [b] + properties: + b: { type: integer } +`, + (ir) => { + const dataModels = ir.models.filter((m) => m.name.startsWith('ThingData')); + const names = dataModels.map((m) => m.name); + expect(names).toContain('ThingData'); + // Second variant has no discriminator, so falls back to numeric suffix. + expect(names).toContain('ThingData2'); + }, + ); + }); + + it('falls back to numeric suffix when two variants have identical const values on the same property', async () => { + // The const-naming heuristic requires distinct values per variant — if + // two variants both pin `type: organization`, naming both + // `OrganizationApiKeyOwner` would collide. Numeric suffix is safer. + await parseInline( + ` +openapi: 3.1.0 +info: { title: Test, version: 1.0.0 } +servers: [{ url: https://api.example.com }] +paths: + /resource: + get: + operationId: getResource + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/Resource' +components: + schemas: + Resource: + type: object + required: [owner] + properties: + owner: + oneOf: + - type: object + required: [type, id] + properties: + type: { type: string, const: organization } + id: { type: string } + - type: object + required: [type, slug] + properties: + type: { type: string, const: organization } + slug: { type: string } +`, + (ir) => { + const ownerModels = ir.models.filter((m) => m.name.includes('ResourceOwner')); + const names = ownerModels.map((m) => m.name); + expect(names).toContain('ResourceOwner'); + // Same const value on both — we can't derive distinct names, so + // the fallback numeric suffix kicks in. + expect(names).toContain('ResourceOwner2'); + }, + ); + }); + + it('preserves single-variant inline schemas (one object, one null) as nullable, no naming work needed', async () => { + // Make sure the const-naming pass doesn't accidentally fire on the + // common `oneOf: [{ type: object … }, { type: null }]` nullable + // pattern — there's only one *object* variant so the multi-variant + // logic shouldn't run. + await parseInline( + ` +openapi: 3.1.0 +info: { title: Test, version: 1.0.0 } +servers: [{ url: https://api.example.com }] +paths: + /x: + get: + operationId: getX + responses: + '200': + description: ok + content: + application/json: + schema: + $ref: '#/components/schemas/X' +components: + schemas: + X: + type: object + required: [box] + properties: + box: + oneOf: + - type: object + required: [name] + properties: + name: { type: string } + - type: 'null' +`, + (ir) => { + const names = ir.models.map((m) => m.name); + expect(names).toContain('XBox'); + expect(names).not.toContain('XBox2'); + }, + ); + }); +});