Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for optional fields on union member types in the API Builder spec and updates the API JSON ingestion path to parse those fields and (optionally) synthesize missing models for union member types.
Changes:
- Extend
union_typein the spec with an optionalfields: [field]property and regenerate Scala models/clients accordingly. - Parse
fieldsfor union member entries inInternalApiJsonFormand introduce auto-model creation for unknown union member types. - Add repo config for API Builder org under
.devops/config.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/apibuilder-spec.json | Adds fields to union_type in the spec (plus minor whitespace cleanup). |
| lib/src/main/scala/generated/ApicollectiveApibuilderSpecV0Models.scala | Regenerated spec models: UnionType now includes optional fields and JSON codecs updated. |
| generated/app/ApicollectiveApibuilderSpecV0Client.scala | Regenerated client: UnionType includes optional fields and version bump. |
| generated/app/ApicollectiveApibuilderSpecV0MockClient.scala | Regenerated mock client: includes fields = None for UnionType factory. |
| core/app/builder/api_json/InternalApiJsonForm.scala | Parses fields on union member entries and appends auto-created models into models. |
| core/app/builder/api_json/AutoCreateUnionTypeModels.scala | New helper to create InternalModelForms for unknown union member types. |
| .devops/config | Adds apibuilder_organization configuration value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def models: Seq[InternalModelForm] = { | ||
| val knownModels = declaredModels ++ internalDatatypeBuilder.modelForms | ||
|
|
||
| val datatypeResolver: DatatypeResolver = DatatypeResolver( | ||
| enumNames = enums.map(_.name), | ||
| interfaceNames = interfaces.map(_.name), | ||
| unionNames = unions.map(_.name), | ||
| modelNames = knownModels.map(_.name) | ||
| ) | ||
|
|
||
| knownModels ++ AutoCreateUnionTypeModels.createModelsForUnionTypes(datatypeResolver, unions) | ||
| } |
There was a problem hiding this comment.
models is now doing non-trivial work (building a resolver and scanning unions) and it’s called multiple times during validation/build (e.g., validateModels references form.models repeatedly). Consider making this a lazy val (or caching knownModels/autoCreatedModels) to avoid recomputation and to ensure a single, consistent set of auto-created models is used across all validation steps.
| @@ -461,7 +461,7 @@ object InternalUnionForm { | |||
| optionalStrings = Seq("description", "discriminator_value"), | |||
| optionalBooleans = Seq("default"), | |||
| optionalObjects = Seq("deprecation"), | |||
| optionalArraysOfObjects = Seq("attributes"), | |||
| optionalArraysOfObjects = Seq("fields", "attributes"), | |||
| prefix = Some(s"Union[$name] type[${datatypeName.getOrElse("")}]") | |||
There was a problem hiding this comment.
Test coverage: this introduces a new union member syntax ({ "type": "X", "fields": [...] }) plus auto-model creation behavior. There are existing specs for inline union models, but there’s no new/updated test here to assert (a) models are created with the provided fields, and (b) when fields is absent, unknown/typoed types still fail (per the PR description). Adding a focused spec in core/test/core would help prevent regressions.
| def createModelsForUnionTypes(resolver: DatatypeResolver, unions: Seq[InternalUnionForm]): Seq[InternalModelForm] = { | ||
| unions.flatMap { u => | ||
| u.types.flatMap { t => | ||
| t.datatype.toOption.map(_.name).flatMap { typ => | ||
| if (isImported(typ)) { | ||
| None | ||
| } else { | ||
| resolver.parse(typ) match { | ||
| case None => Some(createModelForUnionType(typ, t.fields)) | ||
| case Some(_) => None | ||
| } |
There was a problem hiding this comment.
createModelsForUnionTypes currently auto-creates a model for any unknown union member type, even when the union member has no fields. That changes (and can mask) the previous behavior for typos/unknown types, and it also conflicts with the PR description that says existing behavior is preserved when fields is absent. Consider only auto-creating when fields is explicitly provided (or at least when t.fields.nonEmpty), or track fields presence separately (e.g., store it as an Option[Seq[...]] in InternalUnionTypeForm) so missing/typoed types still fail validation.
| unions.flatMap { u => | ||
| u.types.flatMap { t => | ||
| t.datatype.toOption.map(_.name).flatMap { typ => | ||
| if (isImported(typ)) { | ||
| None | ||
| } else { | ||
| resolver.parse(typ) match { | ||
| case None => Some(createModelForUnionType(typ, t.fields)) | ||
| case Some(_) => None | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This can generate duplicate InternalModelForms when the same union member type appears in multiple unions (or multiple times), because the result isn’t deduped by model name. Downstream builders/validators appear to treat models as a simple Seq, so duplicate names could leak into the final Service. Consider distinctBy(_.name) (or a Map keyed by name) and, if two auto-created definitions disagree on fields, emit a validation error rather than picking one silently.
| unions.flatMap { u => | |
| u.types.flatMap { t => | |
| t.datatype.toOption.map(_.name).flatMap { typ => | |
| if (isImported(typ)) { | |
| None | |
| } else { | |
| resolver.parse(typ) match { | |
| case None => Some(createModelForUnionType(typ, t.fields)) | |
| case Some(_) => None | |
| } | |
| } | |
| } | |
| } | |
| } | |
| unions | |
| .flatMap { u => | |
| u.types.flatMap { t => | |
| t.datatype.toOption.map(_.name).flatMap { typ => | |
| if (isImported(typ)) { | |
| None | |
| } else { | |
| resolver.parse(typ) match { | |
| case None => Some(createModelForUnionType(typ, t.fields)) | |
| case Some(_) => None | |
| } | |
| } | |
| } | |
| } | |
| } | |
| .distinctBy(_.name) |
- Fix attributes parsing to use json instead of value for consistency - Change InternalUnionTypeForm.fields to Option[Seq] to distinguish absent fields (no auto-creation) from empty fields (create empty model) - Only auto-create models when fields is explicitly provided, preserving existing behavior for unknown/typoed types (they still fail validation) - Add validation rejecting fields on primitive, list, and map types - Add distinctBy to prevent duplicate auto-created models - Split match arms in AutoCreateUnionTypeModels for clarity - Pass fields through to service spec UnionType - Add comprehensive test coverage (9 new tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
fieldspropertyfieldsis present, the type will support the specified fieldsfieldsis absent, existing behavior is preservedExample
{ "unions": { "task_type": { "discriminator": "discriminator", "types": [ { "type": "game" }, { "type": "merge_person", "fields": [ { "name": "user_id", "type": "string" }, { "name": "person_id", "type": "string" } ]} ] } } }