Delivery API: Generate typed OpenAPI schemas per content type#22666
Conversation
4f0931a to
f97db75
Compare
|
Claude finished @lauraneto's task in 10m 26s —— View job PR ReviewTarget: Adds opt-in typed OpenAPI schema generation to the Delivery API: when
Important
Suggestions
Approved with Suggestions for improvementGood to go, but please carefully consider the importance of the suggestions. The implementation is well-structured, the two-phase schema+document transformer design is sound, the circular-reference placeholder mechanism is clever and well-explained, and the test coverage (contract snapshots + unit tests for both transformer phases) is thorough. |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in Delivery API OpenAPI mode that emits typed schemas per document, element, and media type instead of only the generic interface models.
Changes:
- Adds a new
GenerateContentTypeSchemasDelivery API OpenAPI setting and wires a new schema/document transformer behind it. - Generates typed
*ResponseModel,*Model, and*PropertiesModelschemas, includingoneOf/discriminatorunions and compositionallOfchains. - Expands unit/integration coverage with scenario-specific contract tests and updated OpenAPI snapshots for disabled/enabled modes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Umbraco.Cms.Api.Delivery/Configuration/ConfigureUmbracoDeliveryApiOpenApiOptions.cs |
Registers the new transformer only when the OpenAPI flag is enabled. |
src/Umbraco.Cms.Api.Delivery/OpenApi/Transformers/ContentTypeSchemaTransformer.cs |
Core implementation for typed schema generation and reference cleanup. |
src/Umbraco.Core/Configuration/Models/DeliveryApiSettings.cs |
Adds the new nested OpenAPI configuration flag. |
src/Umbraco.Core/Models/DeliveryApi/IApiElement.cs |
Marks element responses as a polymorphic Delivery API base type. |
src/Umbraco.Infrastructure/Serialization/ContentJsonTypeResolverBase.cs |
Adds media derived-type resolution used during schema generation. |
tests/Umbraco.Tests.UnitTests/Umbraco.Cms.Api.Delivery/OpenApi/ContentTypeSchemaTransformerTests.cs |
Unit tests for placeholder replacement and typed union generation. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/OpenApiContractTestBase.cs |
Shared OpenAPI assertions plus sample content/media type setup. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/OpenApiContractTest.Default.cs |
Verifies the disabled-flag baseline contract. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/OpenApiContractTest.GenericSchemasWithSampleTypes.cs |
Verifies generic schemas remain when sample types exist but the flag is off. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/OpenApiContractTest.TypedSchemasEmptyProject.cs |
Verifies enabled mode for built-in schemas in an empty project. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/OpenApiContractTest.TypedSchemasWithSampleTypes.cs |
Verifies enabled mode with sample document, element, media, and composition types. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/ExpectedContracts/default.json |
Snapshot for the default OpenAPI contract. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/ExpectedContracts/generic-schemas-with-sample-types.json |
Snapshot for generic mode with seeded types. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/ExpectedContracts/typed-schemas-empty-project.json |
Snapshot for typed mode without custom sample types. |
tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/ExpectedContracts/typed-schemas-with-sample-types.json |
Snapshot for typed mode with seeded sample types. |
ContentTypeSchemaTransformer now filters DocumentTypes through DeliveryApiSettings.IsAllowedContentType so document types blocked by AllowedContentTypeAliases / DisallowedContentTypeAliases no longer leak into the polymorphic union or discriminator mapping.
ContentJsonTypeResolverBase.GetDerivedTypes goes back to returning empty. Previously it registered ApiMediaWithCrops and ApiMediaWithCropsResponse as derived types of their interfaces, which made every consumer of the resolver (the Delivery API and webhooks) emit a $type discriminator on media payloads, even when the typed schema feature was disabled. The Delivery API still needs a base schema for the typed media schemas to extend via allOf. Since the concrete media classes are internal to Umbraco.Infrastructure and cannot be referenced from [JsonDerivedType] in Core, ContentTypeSchemaTransformer now builds that base from the interface's own properties when the interface has no [JsonDerivedType] entries. Content/element interfaces are unaffected and keep using their declared concrete derived types. Snapshots regenerated.
Removes the [JsonDerivedType] attributes from IApiContent, IApiContentResponse, and IApiElement. Without them System.Text.Json configures no polymorphism by default, so wire payloads stop carrying $type fields and the OpenAPI spec stops emitting a discriminator on the generic schemas - matching v17 Delivery API behaviour. Consumers that need polymorphic serialization can still register derived types via ContentJsonTypeResolverBase. Snapshots regenerated.
…ry-api-document-type-schema-generation-clean
Spotted an auto-review comment or two to check out first.
|
Just to clear up any confusion @lauraneto - I've added comments for #22710 on this PR by accident. I haven't done anything yet to review this one. |
…hema-generation-clean # Conflicts: # tests/Umbraco.Tests.Integration/Umbraco.Api.Delivery/OpenApi/ExpectedContracts/default.json
…ry-api-document-type-schema-generation-clean
…component schema. Avoid unnecessary re-get of the JsonTypeInfo for the default case.
There was a problem hiding this comment.
Implementation and tests all look great here @lauraneto - you've done a very thorough job.
I think if you've done the testing you've indicated in your companion branch that's a good sign that the output is going to work as expected - so I don't think I need to repeat that.
What I've done though is generate the output of /umbraco/openapi/delivery.json and compared with the setting on and off. Found a few things - as you'll see, with Claude's help - to consider:
- I see
IApiElementModelas:
"IApiElementModel": {
"required": [
"contentType"
],
"type": [
"null",
"object"
],But IApiContentModel as:
"IApiContentModel": {
"required": [
"contentType"
],
"type": "object",
So the former nullable, but the latter not (meaning I believe block.content and block.settings would appear nullable when they aren't). Same issue for IRichTextElementModel. It seems to happen due to the order of updates, and having a nullable property pollute the referenced model to be nullable as well.
The transformer's
CreateContentTypePropertiespassedschema => schema.Type |= JsonSchemaType.Nullas a callback intoCreateSchemato mark each property
nullable, butCreateSchemainvoked that callback against the sameOpenApiSchemainstance it then registered as a shared component (e.g.IApiElementModel) — so the null flag was OR-ed into the shared definition rather than the property-reference site, and every other consumer of that component inherited the nullability. Made worse by being order-dependent: whichever property happened to reach the type first decided whether its component got polluted.
- Most models have
addtionalProperties: false, e.g.:
"CategoryContentResponseModel": {
"type": "object",
"allOf": [
{
"$ref": "#/components/schemas/IApiContentResponseBaseModel"
},
{
"$ref": "#/components/schemas/CategoryContentModel"
}
],
"additionalProperties": false
},
Maybe we drop setting that? Here's Claude's view - you can determine better than me if it makes sense!
Under JSON Schema 2020-12 (which OpenAPI 3.1 mandates), here's what a strict validator does, in order:
- Validate against IApiContentResponseBaseModel (the allOf parent). ✓ — every required field is there.
- Validate against ArticleContentModel (the second allOf parent). ✓ — properties matches.
- Validate against the local schema. The local schema has properties: {} (empty) and additionalProperties: false.
- additionalProperties only sees properties declared in this schema's own properties keyword. It does not look through allOf. So id, createDate,
updateDate, route, properties, cultures — every property contributed by an allOf parent — counts as "additional" → REJECTED.
This is the classic JSON Schema gotcha —
additionalPropertiespredatesallOfhaving well-defined merge semantics, so it never learned to look through composition. JSON Schema 2019-09 introducedunevaluatedPropertiesspecifically to fix it.
Why your clients still work: Code generators don't validate documents — they read them. Every generator you tested (orval, hey-api, kiota) treats
allOfas inheritance: it merges the parent's properties into the child's effective shape before applyingadditionalProperties. So in TypeScript-land, the article instance has all the inherited fields and additional-properties checking is a no-op against the merged shape. The constraint is silently ignored.
What you give up: the documentation no longer claims "no other properties allowed". What you actually had: a constraint that rejected every conforming response under strict validation, ignored by lenient consumers. So you give up nothing real and gain consistency.
What you gain: the document validates strictly, every consumer interprets it identically, and it accurately describes the contract Umbraco actually offers — Umbraco's API can grow new properties in non-major releases, and a closed schema would be a lie even if it worked.
- I get a few odd named models when we have runs of capital letters - e.g.
SEocontrolsPropertiesModel,XMlsitemapContentModel. Looks like you've followed what models builder does here, which is consistent. But maybe this is more a problem for typed clients where the consumer (and consuming developers) are likely "further away" from Umbraco so will just wonder why this is.
Looks like this could be done in ContentTypeSchemaService, modifying:
// Currently uses the same transformation as ModelsBuilder (UmbracoServices.GetClrName)
private string GetContentTypeSchemaId(string contentTypeAlias) =>
contentTypeAlias.ToCleanString(_shortStringHelper, CleanStringType.ConvertCase | CleanStringType.PascalCase);To:
// Content type aliases are already valid identifiers (camelCase or PascalCase) per Umbraco
// validation, so we only need to uppercase the first character. This avoids
// conversion tokenised by case boundaries, which mangles uppercase runs (e.g. "xMLSitemap"
// would become "XMlsitemap"). Deliberately deviates from ModelsBuilder for OpenAPI readability.
private static string GetContentTypeSchemaId(string contentTypeAlias)
=> contentTypeAlias.Length == 0
? contentTypeAlias
: char.ToUpperInvariant(contentTypeAlias[0]) + contentTypeAlias[1..];Note that I've pushed a fix for 1. and for an inline comment (if you see any issues, of course please revert; but it was easier this way so I could actually test out a resolution and propose the update). I imagine this will break your integration tests, so you will have to adjust for that if you do keep the update.
I added the needs-docs label as we should make sure to document the new setting.
JSON Schema 2020-12 (mandated by OpenAPI 3.1) does not let additionalProperties look through allOf, so a strict validator rejects every inherited field on the composed *ResponseModel/*Model/*PropertiesModel schemas. Most code generators silently ignore it, but the document is technically invalid and the constraint would be a lie anyway since Umbraco can grow new properties in non-major releases. Removed from all four schema construction sites (response, content type, properties, and the interface-based fallback) and regenerated the affected snapshots.
Replaces the legacy ModelsBuilder-style ToCleanString tokenizer with ToFirstUpperInvariant. The tokenizer split aliases on case boundaries and mangled capital-letter runs (e.g. "xMLSitemap" -> "XMlsitemap"), making the typed schema names harder to read for OpenAPI consumers. Since content type aliases are already valid identifiers, only the first character needs uppercasing. Also adds an "xMLSitemap" sample type to the integration tests to cover the casing-preservation behavior.
Document, element, and media types share the same alias namespace
across content/media (a doc-type and a media-type can use the same
alias), so a "{Schema}PropertiesModel" naming scheme could collide.
Properties model schemas now follow the same Content/Element/Media
suffix as their parent *Model schema:
- Document type: ArticlePageContentPropertiesModel
- Element type: TestElementElementPropertiesModel
- Media type: VideoMediaPropertiesModel
Composition references look up each composition's own IsElement so
that a doc-type composing an element-type (allowed in the UI) still
references the correct ElementPropertiesModel schema.
AndyButland
left a comment
There was a problem hiding this comment.
The updates all look good to me @lauraneto and I don't see anything further to comment. Just note my mention from earlier about preparing a docs update for the new configuration setting please.
If you are happy, fine with me to merge in.
Summary
Adds optional content-type-aware schema generation to the Delivery API OpenAPI document. When enabled, the spec emits per-document-type, per-element-type, and per-media-type schemas wired into the existing
IApi*polymorphic schemas viaoneOf+discriminator. Composition relationships are reflected throughallOfreferences on the corresponding*PropertiesModel.The feature is opt-in via
Umbraco:CMS:DeliveryApi:OpenApi:GenerateContentTypeSchemas(defaultfalse).Schema shape
For each non-element document type, the spec gains
{Alias}ContentResponseModel(top-level response withcultures),{Alias}ContentModel(nested form, used in property references), and{Alias}PropertiesModel(the property bag). Element types get{Alias}ElementModel+{Alias}PropertiesModel, and media types get{Alias}MediaWithCropsResponseModel,{Alias}MediaWithCropsModel, and{Alias}PropertiesModel.The interface schemas (
IApiContentResponseModel,IApiContentModel,IApiMediaWithCropsResponseModel,IApiMediaWithCropsModel) become unions over their typed variants with the discriminator oncontentType/mediaType.Compositions
A composing type's
*PropertiesModelreferences each composition's*PropertiesModelviaallOf. Composition types themselves appear as full content types in the polymorphic union.Testing
For end-to-end verification, check out the throwaway companion branch
v18/task/delivery-api-openapi-sample-content. It commits a pre-seeded SQLite database plus four TypeScript / .NET console projects undertests/clients(orval,hey-api,kiota,nswag) that generate clients from the running spec and exercise the typed shape. Seetests/clients/README.mdfor the full setup.Once on that branch (and after applying the connection-string + Delivery API config snippets from the README to your local
appsettings.json):dotnet run --project src/Umbraco.Web.UIto start Umbraco athttps://localhost:44339. The committed DB ships with sample document, element, media, and composition types so the spec is non-trivial./umbraco/openapi/delivery.jsonand confirm each document type produces its{Alias}ContentResponseModel,{Alias}ContentModel, and{Alias}PropertiesModel(or the media equivalents), thatIApiContentResponseModellists every type in itsoneOfanddiscriminator.mapping, and that the composing type's*PropertiesModelreferences the composition's*PropertiesModelviaallOf.tests/clients/orval, runnpm install && npm start. The script regenerates the client from the live spec, builds it, and prints the data for the seeded test page. Successful compilation + typed property access is the primary signal — discriminating oncontentTypeshould narrow the union toTestPageContentResponseModeland expose the composition fields (sharedToggle,sharedString,sharedRadiobox,sharedRichText).tests/clients/hey-api, runnpm install && npm start. Same expectations as orval; this client uses@hey-api/openapi-tswith the bundled fetch client.tests/clients/kiota, rundotnet run. The build downloads the spec, restores the kiota tool, and generates a C# client. Compiles and runs, but property bags collapse toAdditionalData(kiota loses field typing through our*PropertiesModelindirection — see the project README). This is a known kiota limitation, not a spec issue.tests/clients/nswag, rundotnet run. Build is expected to fail with ~17 compile errors referencingAnonymous,MultinodeTreepicker,MediaPicker, etc. NSwag's generator does not handle our OpenAPI 3.1 polymorphic shape. The project documents the failure mode rather than ships a working client.Finally, flip
Umbraco:CMS:DeliveryApi:OpenApi:GenerateContentTypeSchemastofalse, restart the web app, and confirm/umbraco/openapi/delivery.jsonfalls back to the genericApiContent[Response]Model/ApiMediaWithCrops[Response]Modelshape with no per-type schemas.