diff --git a/packages/quicktype-core/src/Run.ts b/packages/quicktype-core/src/Run.ts index 898640aff5..2253823b9a 100644 --- a/packages/quicktype-core/src/Run.ts +++ b/packages/quicktype-core/src/Run.ts @@ -342,6 +342,7 @@ class Run implements RunContext { stringTypeMapping, conflateNumbers, true, + targetLanguage.supportsUnionsWithMultipleObjectTypes, debugPrintReconstitution, ); }); @@ -372,6 +373,7 @@ class Run implements RunContext { stringTypeMapping, conflateNumbers, false, + targetLanguage.supportsUnionsWithMultipleObjectTypes, debugPrintReconstitution, ); }); @@ -436,6 +438,7 @@ class Run implements RunContext { stringTypeMapping, conflateNumbers, false, + targetLanguage.supportsUnionsWithMultipleObjectTypes, debugPrintReconstitution, ); }); @@ -485,6 +488,7 @@ class Run implements RunContext { stringTypeMapping, conflateNumbers, false, + targetLanguage.supportsUnionsWithMultipleObjectTypes, debugPrintReconstitution, ); }); diff --git a/packages/quicktype-core/src/TargetLanguage.ts b/packages/quicktype-core/src/TargetLanguage.ts index f0617b7390..f916fa9d6b 100644 --- a/packages/quicktype-core/src/TargetLanguage.ts +++ b/packages/quicktype-core/src/TargetLanguage.ts @@ -115,6 +115,10 @@ export abstract class TargetLanguage< return false; } + public get supportsUnionsWithMultipleObjectTypes(): boolean { + return false; + } + public needsTransformerForType(_t: Type): boolean { return false; } diff --git a/packages/quicktype-core/src/Type/Type.ts b/packages/quicktype-core/src/Type/Type.ts index f888f4033f..5ef0daa81e 100644 --- a/packages/quicktype-core/src/Type/Type.ts +++ b/packages/quicktype-core/src/Type/Type.ts @@ -14,7 +14,6 @@ import { mapSortToArray, setFilter, setMap, - setSortBy, setUnionInto, toReadonlySet, } from "collection-utils"; @@ -861,8 +860,12 @@ export abstract class SetOperationType extends Type { } public getNonAttributeChildren(): Set { - // FIXME: We're assuming no two members of the same kind. - return setSortBy(this.members, (t) => t.kind); + // Opted-in targets can retain same-kind union members. + const members = Array.from(this.members).sort((a, b) => { + const kindOrder = a.kind.localeCompare(b.kind); + return kindOrder !== 0 ? kindOrder : a.index - b.index; + }); + return new Set(members); } public isPrimitive(): this is PrimitiveType { diff --git a/packages/quicktype-core/src/UnifyClasses.ts b/packages/quicktype-core/src/UnifyClasses.ts index b6a5a6efb9..e828a25e0b 100644 --- a/packages/quicktype-core/src/UnifyClasses.ts +++ b/packages/quicktype-core/src/UnifyClasses.ts @@ -1,5 +1,6 @@ import { iterableFirst, setUnionInto } from "collection-utils"; +import { explicitUnionMemberTypeAttributeKind } from "./attributes/ExplicitUnionMember.js"; import { type TypeAttributes, combineTypeAttributes, @@ -125,6 +126,7 @@ export class UnifyUnionBuilder extends UnionBuilder< typeBuilder: BaseGraphRewriteBuilder, private readonly _makeObjectTypes: boolean, private readonly _makeClassesFixed: boolean, + private readonly _supportsUnionsWithMultipleObjectTypes: boolean, private readonly _unifyTypes: (typesToUnify: TypeRef[]) => TypeRef, ) { super(typeBuilder); @@ -159,6 +161,20 @@ export class UnifyUnionBuilder extends UnionBuilder< const objectTypes = objectRefs.map((r) => assertIsObject(derefTypeRef(r, this.typeBuilder)), ); + if ( + this._supportsUnionsWithMultipleObjectTypes && + this.canKeepObjectTypesDistinct(objectTypes, typeAttributes) + ) { + const members = new Set( + objectRefs.map((r) => this.typeBuilder.reconstituteTypeRef(r)), + ); + return this.typeBuilder.getUnionType( + typeAttributes, + members, + forwardingRef, + ); + } + const { hasProperties, hasAdditionalProperties, @@ -225,6 +241,52 @@ export class UnifyUnionBuilder extends UnionBuilder< } } + private canKeepObjectTypesDistinct( + objectTypes: ObjectType[], + typeAttributes: TypeAttributes, + ): boolean { + if (objectTypes.length < 2) return false; + + // Every member must come from the same explicit union of direct schema + // references. Counting distinct members in each group prevents a marked + // type from changing an unrelated union in which that type is later reused. + const groups = + explicitUnionMemberTypeAttributeKind.tryGetInAttributes( + typeAttributes, + ); + if ( + groups === undefined || + !Array.from(groups.values()).some( + (members) => members.size === objectTypes.length, + ) + ) { + return false; + } + + const typeNames = new Set(); + const propertyNames = new Set(); + for (const objectType of objectTypes) { + if ( + !objectType.hasNames || + objectType.getProperties().size === 0 || + objectType.getAdditionalProperties() !== undefined + ) { + return false; + } + + const typeName = objectType.getCombinedName(); + if (typeNames.has(typeName)) return false; + typeNames.add(typeName); + + for (const propertyName of objectType.getProperties().keys()) { + if (propertyNames.has(propertyName)) return false; + propertyNames.add(propertyName); + } + } + + return true; + } + protected makeArray( arrays: TypeRef[], typeAttributes: TypeAttributes, @@ -249,6 +311,7 @@ export function unionBuilderForUnification( typeBuilder, makeObjectTypes, makeClassesFixed, + false, (trefs) => unifyTypes( new Set(trefs.map((tref) => derefTypeRef(tref, typeBuilder))), diff --git a/packages/quicktype-core/src/attributes/ExplicitUnionMember.ts b/packages/quicktype-core/src/attributes/ExplicitUnionMember.ts new file mode 100644 index 0000000000..7cadba3415 --- /dev/null +++ b/packages/quicktype-core/src/attributes/ExplicitUnionMember.ts @@ -0,0 +1,47 @@ +import { type TypeAttributes, TypeAttributeKind } from "./TypeAttributes.js"; + +export type ExplicitUnionMemberGroups = ReadonlyMap< + symbol, + ReadonlySet +>; + +class ExplicitUnionMemberTypeAttributeKind extends TypeAttributeKind { + public constructor() { + super("explicit-union-member"); + } + + public combine( + values: ExplicitUnionMemberGroups[], + ): ExplicitUnionMemberGroups { + const combined = new Map>(); + for (const groups of values) { + for (const [group, members] of groups) { + const combinedMembers = combined.get(group) ?? new Set(); + for (const member of members) combinedMembers.add(member); + combined.set(group, combinedMembers); + } + } + return combined; + } + + public makeInferred( + value: ExplicitUnionMemberGroups, + ): ExplicitUnionMemberGroups { + return value; + } + + public stringify(_value: ExplicitUnionMemberGroups): string { + return "explicit union member"; + } +} + +export const explicitUnionMemberTypeAttributeKind: TypeAttributeKind = + new ExplicitUnionMemberTypeAttributeKind(); + +export function makeExplicitUnionMemberAttributes( + group: symbol, +): TypeAttributes { + return explicitUnionMemberTypeAttributeKind.makeAttributes( + new Map([[group, new Set([Symbol()])]]), + ); +} diff --git a/packages/quicktype-core/src/input/JSONSchemaInput.ts b/packages/quicktype-core/src/input/JSONSchemaInput.ts index 66b1c4104b..5608fbacf0 100644 --- a/packages/quicktype-core/src/input/JSONSchemaInput.ts +++ b/packages/quicktype-core/src/input/JSONSchemaInput.ts @@ -32,6 +32,7 @@ import { import { defaultValueAttributeProducer } from "../attributes/DefaultValue.js"; import { descriptionAttributeProducer } from "../attributes/Description.js"; import { enumValuesAttributeProducer } from "../attributes/EnumValues.js"; +import { makeExplicitUnionMemberAttributes } from "../attributes/ExplicitUnionMember.js"; import { StringTypes } from "../attributes/StringTypes.js"; import { type TypeAttributes, @@ -1215,6 +1216,26 @@ async function addTypesInSchema( kind: string, ): Promise { const typeRefs = await makeTypesFromCases(cases, kind); + if ( + (kind === "oneOf" || kind === "anyOf") && + cases.length >= 2 && + cases.every( + (c) => + c !== null && + typeof c === "object" && + !Array.isArray(c) && + typeof (c as { $ref?: unknown }).$ref === "string", + ) + ) { + const explicitUnionGroup = Symbol(); + for (const typeRef of typeRefs) { + typeBuilder.addAttributes( + typeRef, + makeExplicitUnionMemberAttributes(explicitUnionGroup), + ); + } + } + let unionAttributes = makeTypeAttributesInferred(typeAttributes); if (kind === "oneOf") { forEachProducedAttribute( diff --git a/packages/quicktype-core/src/language/Python/language.ts b/packages/quicktype-core/src/language/Python/language.ts index d3e1ed48c9..b86d70e1fe 100644 --- a/packages/quicktype-core/src/language/Python/language.ts +++ b/packages/quicktype-core/src/language/Python/language.ts @@ -129,6 +129,10 @@ export class PythonTargetLanguage extends TargetLanguage< return false; } + public get supportsUnionsWithMultipleObjectTypes(): boolean { + return true; + } + public needsTransformerForType(t: Type): boolean { if (t instanceof UnionType) { return iterableSome(t.members, (m) => diff --git a/packages/quicktype-core/src/rewrites/FlattenUnions.ts b/packages/quicktype-core/src/rewrites/FlattenUnions.ts index aab4d85119..e927f73029 100644 --- a/packages/quicktype-core/src/rewrites/FlattenUnions.ts +++ b/packages/quicktype-core/src/rewrites/FlattenUnions.ts @@ -16,6 +16,7 @@ export function flattenUnions( stringTypeMapping: StringTypeMapping, conflateNumbers: boolean, makeObjectTypes: boolean, + supportsUnionsWithMultipleObjectTypes: boolean, debugPrintReconstitution: boolean, ): [TypeGraph, boolean] { let needsRepeat = false; @@ -140,6 +141,7 @@ export function flattenUnions( builder, makeObjectTypes, true, + supportsUnionsWithMultipleObjectTypes, unifyTypeRefs, ); return unifyTypes( diff --git a/test/inputs/schema/named-class-union.1.fail.union.json b/test/inputs/schema/named-class-union.1.fail.union.json new file mode 100644 index 0000000000..077cd796fd --- /dev/null +++ b/test/inputs/schema/named-class-union.1.fail.union.json @@ -0,0 +1,3 @@ +{ + "op": [] +} diff --git a/test/inputs/schema/named-class-union.1.json b/test/inputs/schema/named-class-union.1.json new file mode 100644 index 0000000000..42a64f57f4 --- /dev/null +++ b/test/inputs/schema/named-class-union.1.json @@ -0,0 +1,5 @@ +{ + "op": { + "foo": "foo value" + } +} diff --git a/test/inputs/schema/named-class-union.2.json b/test/inputs/schema/named-class-union.2.json new file mode 100644 index 0000000000..be3078dbdf --- /dev/null +++ b/test/inputs/schema/named-class-union.2.json @@ -0,0 +1,5 @@ +{ + "op": { + "bar": "bar value" + } +} diff --git a/test/inputs/schema/named-class-union.schema b/test/inputs/schema/named-class-union.schema new file mode 100644 index 0000000000..fed55880f0 --- /dev/null +++ b/test/inputs/schema/named-class-union.schema @@ -0,0 +1,33 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "title": "Container", + "type": "object", + "additionalProperties": false, + "properties": { + "op": { + "oneOf": [ + { "$ref": "#/definitions/Foo" }, + { "$ref": "#/definitions/Bar" } + ] + } + }, + "required": ["op"], + "definitions": { + "Foo": { + "type": "object", + "additionalProperties": false, + "properties": { + "foo": { "type": "string" } + }, + "required": ["foo"] + }, + "Bar": { + "type": "object", + "additionalProperties": false, + "properties": { + "bar": { "type": "string" } + }, + "required": ["bar"] + } + } +} diff --git a/test/languages.ts b/test/languages.ts index 120f3170b0..d768b07149 100644 --- a/test/languages.ts +++ b/test/languages.ts @@ -666,6 +666,8 @@ export const CJSONLanguage: Language = { * an integer is expected) are not checked either. */ ...skipsArrayElementValidation, "multi-type-enum.schema", + /* Named object unions merge into an optional-properties class, and wrong-shaped values are not rejected. */ + "named-class-union.schema", "nested-intersection-union.schema", "prefix-items.schema", /* Constraints (min/max and regex) are not supported (for the current implementation, can be added later, should abord parsing and return NULL) */ @@ -829,7 +831,9 @@ export const CPlusPlusLanguage: Language = { "integer-before-number.schema", // Python-specific union-order regression. // uses too much memory "keyword-unions.schema", - // The generated deserializer accepts non-object values when all class properties are optional. + // Named object unions merge into a class with all-optional properties, + // whose generated deserializer accepts non-object values. + "named-class-union.schema", "nested-intersection-union.schema", // Recursive top-level unions produce aliases that can refer to later aliases. "recursive-union-flattening.schema", @@ -935,8 +939,9 @@ export const ElmLanguage: Language = { // property decodes to the object's constructor function. "constructor.schema", "keyword-unions.schema", - // The generated decoder accepts invalid union members because all - // class properties decode via `Jpipe.optional`. + // Named object unions merge into a class whose properties all decode + // via `Jpipe.optional`, which accepts invalid union members. + "named-class-union.schema", "nested-intersection-union.schema", ], rendererOptions: {}, @@ -1439,8 +1444,9 @@ export const KotlinLanguage: Language = { // which is not represented in the types (implicit-class-array-union); // class-map-union: KlaxonException: Couldn't find a suitable constructor for class UnionValue to initialize with {} ...skipsUntypedUnions, - // Deserializes an array where a union of two classes is expected - // instead of rejecting it. + // Named object unions merge into one optional-properties class, which + // deserializes an array instead of rejecting it. + "named-class-union.schema", "nested-intersection-union.schema", "class-with-additional.schema", ...skipsMapValueValidation, @@ -1534,8 +1540,9 @@ export const KotlinJacksonLanguage: Language = { // which is not represented in the types (implicit-class-array-union); // class-map-union: KlaxonException: Couldn't find a suitable constructor for class UnionValue to initialize with {} ...skipsUntypedUnions, - // Deserializes an array where a union of two classes is expected - // instead of rejecting it. + // Named object unions merge into one optional-properties class, which + // deserializes an array instead of rejecting it. + "named-class-union.schema", "nested-intersection-union.schema", "class-with-additional.schema", ...skipsMapValueValidation, @@ -1902,6 +1909,10 @@ export const HaskellLanguage: Language = { skipSchema: [ "integer-before-number.schema", // Python-specific union-order regression. ...skipsUntypedUnions, + // Named object unions merge into an optional-properties class. The test + // driver encodes the Maybe result, so a failed decode prints "null" and + // exits 0 — expected-failure samples cannot be detected. + "named-class-union.schema", // The test driver encodes the Maybe result, so a failed decode prints // "null" and exits 0 — expected-failure samples cannot be detected. // (A top-level `[Int]` correctly fails to decode `[1, 2, "three"]`, diff --git a/test/unit/named-class-union.test.ts b/test/unit/named-class-union.test.ts new file mode 100644 index 0000000000..e9a7ba197d --- /dev/null +++ b/test/unit/named-class-union.test.ts @@ -0,0 +1,126 @@ +// Regression test for issue #1646: explicitly named object alternatives in a +// JSON Schema oneOf/anyOf must remain distinct when the target language supports +// unions with multiple object types. +// +// End-to-end round-trip coverage lives in +// `test/inputs/schema/named-class-union.schema`. The fixture cannot assert +// that no synthetic merged class was generated, so that structural detail is +// covered here. + +import { + FetchingJSONSchemaStore, + InputData, + JSONSchemaInput, + quicktype, +} from "quicktype-core"; +import { describe, expect, test } from "vitest"; + +function schemaWithUnion(operation: "oneOf" | "anyOf"): object { + const alternatives = [ + { $ref: "#/definitions/Foo" }, + { $ref: "#/definitions/Bar" }, + ]; + return { + type: "object", + additionalProperties: false, + properties: { + op: + operation === "oneOf" + ? { oneOf: alternatives } + : { anyOf: alternatives }, + }, + required: ["op"], + definitions: { + Foo: { + type: "object", + additionalProperties: false, + properties: { foo: { type: "string" } }, + required: ["foo"], + }, + Bar: { + type: "object", + additionalProperties: false, + properties: { bar: { type: "string" } }, + required: ["bar"], + }, + }, + }; +} + +function schemaWithReusedRef(): object { + const schema = schemaWithUnion("oneOf") as { + properties: Record; + }; + schema.properties.mixed = { + oneOf: [ + { $ref: "#/definitions/Foo" }, + { + title: "Baz", + type: "object", + additionalProperties: false, + properties: { baz: { type: "string" } }, + required: ["baz"], + }, + ], + }; + return schema; +} + +async function generate( + lang: "python" | "typescript", + operation: "oneOf" | "anyOf" = "oneOf", + schema: object = schemaWithUnion(operation), +): Promise { + const schemaInput = new JSONSchemaInput(new FetchingJSONSchemaStore()); + await schemaInput.addSource({ + name: "Container", + schema: JSON.stringify(schema), + }); + const inputData = new InputData(); + inputData.addInput(schemaInput); + + const result = await quicktype({ + inputData, + lang, + rendererOptions: lang === "python" ? { "python-version": "3.7" } : {}, + }); + return result.lines.join("\n"); +} + +describe("named class unions (issue #1646)", () => { + test("Python preserves the named alternatives", async () => { + const output = await generate("python"); + + expect(output).toContain("class Foo:"); + expect(output).toContain("class Bar:"); + expect(output).toContain("op: Union[Foo, Bar]"); + expect(output).not.toContain("class Op:"); + expect(output).toContain("from_union([Foo.from_dict, Bar.from_dict]"); + }); + + test("Python preserves anyOf alternatives produced by TypeScript input", async () => { + const output = await generate("python", "anyOf"); + + expect(output).toContain("op: Union[Foo, Bar]"); + expect(output).not.toContain("class Op:"); + }); + + test("the explicit-union marker does not leak through reused refs", async () => { + const output = await generate("python", "oneOf", schemaWithReusedRef()); + + expect(output).toContain("op: Union[Foo, Bar]"); + expect(output).toContain("mixed: Optional[MixedClass] = None"); + expect(output).toContain("foo: Optional[str] = None"); + expect(output).toContain("baz: Optional[str] = None"); + }); + + test("languages that do not opt in keep the existing behavior", async () => { + const output = await generate("typescript"); + + expect(output).toContain("op: Foo;"); + expect(output).toContain("export interface Foo {"); + expect(output).toContain("foo?: string;"); + expect(output).toContain("bar?: string;"); + expect(output).not.toContain("export interface Bar {"); + }); +});