You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(http): apply the merge patch transform to the base model - #12299
The merge patch mutator in packages/http/src/merge-patch.ts transforms the properties and indexer of a model, but the clone keeps baseModel pointing at the original base. So for model Widget extends Base, MergePatchUpdate<Widget> emitted allOf: [{ $ref: Base }], inherited required properties stayed required, and {} was rejected. Nested WidgetMergePatchUpdateOrCreate had the same allOf.
The Model mutator now runs baseModel through the same mutator and sets the result on the clone. The base gets the matching name, such as BaseMergePatchUpdate or BaseMergePatchUpdateOrCreate, and is shared with MergePatchUpdate<Base>. Inherited properties follow the model's own rules: a required id becomes optional but not nullable, optional ones also take null, and a discriminator on the base stays required. I kept the base instead of flattening so emitters still see the inheritance and discriminator. A patched base's discriminator mapping still lists the unpatched derived models, as MergePatchUpdate<Pet> does today.
Three changes to note:
A visible inherited @path or other metadata property now reports merge-patch-contains-metadata, as the model's own properties do, unless the derived model overrides it. On main it only got the metadata-ignored warning.
A transformed model or union with @friendlyName now gets a friendly name from the same name template, so a patched BaseDto base is emitted as BaseDtoMergePatchUpdate and the original keeps BaseDto. Without this, the patched base would reuse BaseDto and openapi3 would report duplicate-type-name, which root and nested models already hit on main. model WidgetPatch is MergePatchUpdate<Widget> still emits as WidgetPatch. When the original friendly named model is not emitted itself (for example it lives outside the service namespace, or omit-unreachable-types is on), its patch is now WidgetDtoMergePatchUpdate instead of WidgetDto.
A property that the transform makes optional drops @key, @madeOptional and @madeRequired in the patch, since it is optional in every version. Inherited properties need this now that the base is patched, and own properties get it too: on main an own @key reports no-optional-key, an own @madeRequired reports made-required-optional, and an own @madeOptional stays required in the patch schema of earlier versions. Properties that stay required, such as discriminators and the properties of an element in a replace only array, keep them.
One case still differs from main: a @resource model whose @key is inherited now reports resource-missing-key on the patch model, because the patch has no key. A @resource model with its own @key already reports this on main, so I left it for a separate change.
Smaller output changes: a Record<Thing> base now patches its values, and a read only inherited @key @path no longer warns.
I added 17 cases to packages/http/test/merge-patch.test.ts, 5 to packages/openapi3/test/merge-patch.test.ts, and a changeset.
Testing, built from this branch:
vitest run test/merge-patch.test.ts: 54 passed in packages/http and 11 in packages/openapi3. With src/merge-patch.ts from main, 21 of the 22 new cases fail; the other checks an error main already reports.
vitest run with two workers: 447 passed in packages/http, 2717 in packages/openapi3.
The issue's snippet as OpenAPI 3.1: {}, {"label":"x"} and {"label":null} now validate against WidgetPatch.
The merge patch mutator transformed the properties and indexer of a
model but left the clone's baseModel pointing at the original base. For
a model that uses extends, MergePatchUpdate emitted an allOf to the
unpatched base, so inherited required properties stayed required and an
empty patch was rejected.
The base model now goes through the same mutator, so it is emitted as
BaseMergePatchUpdate (or its OrCreate and ReplaceOnly variants) and the
inherited properties follow the same rules as the model's own.
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.
A model or union cloned by the merge patch mutator replays the
decorators of its source, so `@friendlyName` gave the clone the same
emitter name as the original. Now that the base model goes through the
mutator, a base with a friendly name hit `duplicate-type-name` in
OpenAPI whenever the original base was also emitted. Root and nested
models with a friendly name already did on main.
When the mutator renames a clone, it now also rewrites the clone's
friendly name with the same name template, so BaseDto becomes
BaseDtoMergePatchUpdate (or its OrCreate and ReplaceOnly variants). The
original keeps its friendly name, and `model X is MergePatchUpdate<T>`
still uses its own name.
I pushed a follow up commit for the @friendlyName case.
When the mutator renames a model or union that has a friendly name, it now builds the clone's friendly name from the same name template and leaves the original alone. The base keeps BaseDto and its patched copy becomes BaseDtoMergePatchUpdate. Nested models get ThingDtoMergePatchUpdateOrCreate and ThingDtoMergePatchUpdateReplaceOnly, and a base with @friendlyName("{name}Envelope", T) becomes InnerEnvelopeMergePatchUpdate. This also covers root and nested models with a friendly name, which already hit duplicate-type-name on main. model WidgetPatch is MergePatchUpdate<Widget> still emits as WidgetPatch.
One output change: when the original friendly named model is not emitted itself (for example it lives outside the service namespace, or omit-unreachable-types is on), its patch came out as WidgetDto on main and comes out as WidgetDtoMergePatchUpdate now.
I added two cases to packages/http/test/merge-patch.test.ts and two to packages/openapi3/test/merge-patch.test.ts, including a PATCH that takes MergePatchUpdate<Widget> and returns Widget with both schemas emitted. All four fail on the previous commit and pass now. Full runs: 435 passed in packages/http and 2715 in packages/openapi3.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Base traversal rejects metadata properties even when the derived model overrides them with non-metadata properties.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Avoid inherited metadata errors for overridden properties
packages/http/src/merge-patch.ts:420
For model Base { @path id: string; } model Widget extends Base { id: string; }, traversing the base now reports merge-patch-contains-metadata on Base.id and blocks emission, even though the effective Widget.id has no metadata (packages/compiler/src/core/checker.ts:8984–9002 excludes overridden properties). Validate metadata against each patch subject's effective properties, independently of cached base transformation, and add a regression that also verifies patching Base directly still reports the error.
Since the base model goes through the merge patch mutator, the base
reported `merge-patch-contains-metadata` for its own metadata
properties. For `model Base { @path id: string; }` and
`model Widget extends Base { id: string; }`, patching Widget failed even
though Widget.id has no metadata. On main it compiled with only the
`metadata-ignored` warning.
The metadata check now runs when a model is used as a patch subject, on
its own properties and the inherited ones it does not override
(`walkPropertiesInherited`). A model reached only as a base is not
checked on its own, and each property is reported once per mutator, so
patching Base directly, or through a derived model that does not
override id, still reports it in either order. The check is registered
before the model's properties are transformed, so a base that is patched
again through its own properties (`parent?: Base`) is still checked.
Record values now go through `cachedMutateSubgraph` like the other
nested types so their check runs too.
Metadata properties are also left out of the transformed model, so the
patched base does not keep the overridden property.
I pushed one more commit for a case the base model change broke: a derived model that overrides an inherited metadata property.
For model Base { @path id: string; name: string; } and model Widget extends Base { id: string; }, patching Widget reported merge-patch-contains-metadata on Base.id, although Widget.id has no metadata. On main this compiled with only the metadata-ignored warning.
The check now runs for each patch subject, on its own properties and the inherited ones it does not override. So MergePatchUpdate<Base>, a derived model that keeps the @path id, and a Base that is patched through its own parent?: Base property still get the error, whichever operation comes first. The patched base also leaves metadata out, so here BaseMergePatchUpdate has only name and WidgetMergePatchUpdate has an optional id.
I added eight cases to packages/http/test/merge-patch.test.ts (@path and @header overrides next to a plain size override, an override two levels down, Base patched directly after Widget, a sibling that does not override id, and three ways Base is patched again through its own properties) and one to packages/openapi3/test/merge-patch.test.ts. The four override cases fail on the previous commit and pass now. Full runs: 443 passed in packages/http and 2716 in packages/openapi3.
Apply the MergePatchUpdate and MergePatchCreateOrUpdate transform to the base model of a model that uses extends, so inherited properties become optional in the patch shape instead of coming from the unpatched base model. A visible inherited metadata property now reports merge-patch-contains-metadata, as the model's own properties do, unless the derived model overrides it. A transformed model or union with @friendlyName now gets a friendly name from the same name template, such as BaseDtoMergePatchUpdate for BaseDto, so OpenAPI no longer reports duplicate-type-name when the original is also emitted.
Since the base model goes through the merge patch mutator, an inherited
`@key` property becomes optional in the patched base but keeps `@key`.
Replaying it reports `no-optional-key`, so for
`model Base { @key id: string; }` and `model Widget extends Base {}`,
`MergePatchUpdate<Widget>` failed to compile. On main it compiled
because the base was not patched. An inherited `@madeRequired` property
failed the same way with `made-required-optional`, and an inherited
`@madeOptional` property stayed required in the patch schema of the
versions before it was made optional.
A property clone that the transform makes optional now leaves out
`@key`, `@madeOptional` and `@madeRequired`, since a patch property is
optional in every version. The source keeps them, and so do clones that
stay required, such as the properties of an element in a replace only
array. The same rule covers a model's own properties, which already hit
these problems on main.
I pushed one more commit for properties that the patch makes optional.
For model Base { @key id: string; } and model Widget extends Base {}, the patched base made id optional but kept @key, so MergePatchUpdate<Widget> failed with no-optional-key. Main compiled it because the base was not patched. The versioning decorators had similar problems: an inherited @madeRequired(Versions.v2) property failed with made-required-optional, and an inherited @madeOptional(Versions.v2) property stayed required in the v1 patch schema.
A property that the transform makes optional now drops @key, @madeOptional and @madeRequired in the patch, since it is optional in every version. The source keeps them, and so do properties that stay required, such as the properties of an element in a replace only array. The same rule covers a model's own properties, which have the same problems on main.
One case still differs from main: a @resource model whose @key is inherited now reports resource-missing-key on WidgetMergePatchUpdate, because the patch has no key. A @resource model with its own @key already reports this on main, so I left it for a separate change.
New tests: four cases in packages/http/test/merge-patch.test.ts (an inherited key, an own key, a key that stays required on an element of a replace only array, and a Record<Thing> base under both MergePatchUpdate and MergePatchCreateOrUpdate that leaves Record<Thing> and Thing unchanged) and one versioned case in packages/openapi3/test/merge-patch.test.ts. The two key cases that drop @key and the versioned case fail on the previous commit and pass now. Full runs: 447 passed in packages/http and 2717 in packages/openapi3. The changeset has one more sentence for this.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
microsoft-github-policy-service
Bot
added
lib:http
emitter:openapi3
Fixes #11534
The merge patch mutator in
packages/http/src/merge-patch.tstransforms the properties and indexer of a model, but the clone keepsbaseModelpointing at the original base. So formodel Widget extends Base,MergePatchUpdate<Widget>emittedallOf: [{ $ref: Base }], inherited required properties stayed required, and{}was rejected. NestedWidgetMergePatchUpdateOrCreatehad the sameallOf.The
Modelmutator now runsbaseModelthrough the same mutator and sets the result on the clone. The base gets the matching name, such asBaseMergePatchUpdateorBaseMergePatchUpdateOrCreate, and is shared withMergePatchUpdate<Base>. Inherited properties follow the model's own rules: a requiredidbecomes optional but not nullable, optional ones also takenull, and a discriminator on the base stays required. I kept the base instead of flattening so emitters still see the inheritance and discriminator. A patched base's discriminator mapping still lists the unpatched derived models, asMergePatchUpdate<Pet>does today.Three changes to note:
@pathor other metadata property now reportsmerge-patch-contains-metadata, as the model's own properties do, unless the derived model overrides it. On main it only got themetadata-ignoredwarning.@friendlyNamenow gets a friendly name from the same name template, so a patchedBaseDtobase is emitted asBaseDtoMergePatchUpdateand the original keepsBaseDto. Without this, the patched base would reuseBaseDtoand openapi3 would reportduplicate-type-name, which root and nested models already hit on main.model WidgetPatch is MergePatchUpdate<Widget>still emits asWidgetPatch. When the original friendly named model is not emitted itself (for example it lives outside the service namespace, oromit-unreachable-typesis on), its patch is nowWidgetDtoMergePatchUpdateinstead ofWidgetDto.@key,@madeOptionaland@madeRequiredin the patch, since it is optional in every version. Inherited properties need this now that the base is patched, and own properties get it too: on main an own@keyreportsno-optional-key, an own@madeRequiredreportsmade-required-optional, and an own@madeOptionalstays required in the patch schema of earlier versions. Properties that stay required, such as discriminators and the properties of an element in a replace only array, keep them.One case still differs from main: a
@resourcemodel whose@keyis inherited now reportsresource-missing-keyon the patch model, because the patch has no key. A@resourcemodel with its own@keyalready reports this on main, so I left it for a separate change.Smaller output changes: a
Record<Thing>base now patches its values, and a read only inherited@key @pathno longer warns.I added 17 cases to
packages/http/test/merge-patch.test.ts, 5 topackages/openapi3/test/merge-patch.test.ts, and a changeset.Testing, built from this branch:
vitest run test/merge-patch.test.ts: 54 passed inpackages/httpand 11 inpackages/openapi3. Withsrc/merge-patch.tsfrom main, 21 of the 22 new cases fail; the other checks an error main already reports.vitest runwith two workers: 447 passed inpackages/http, 2717 inpackages/openapi3.{},{"label":"x"}and{"label":null}now validate againstWidgetPatch.