Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce primitive to filter model properties and get effective type #506

Merged
merged 13 commits into from
Jun 3, 2022

Conversation

nguerrera
Copy link
Contributor

@nguerrera nguerrera commented May 4, 2022

This change adds two new primitives to the checker:

filterModelProperties(model: ModelType, filter: (property: ModelTypeProperty) => boolean): ModelType

Applies a given filter to a model. If no properties are filtered out, then it returns the input unchanged. If any are filtered out then it returns a new anonymous model that is equivalent to { ...model } with the filtered out properties removed.

getEffectiveModelType(model: ModelType): ModelType

For anonymous models, attempts to find a non-anonymous model with the same set of properties. This allows recovering a "good name" when the result of filtering out properties effectively leaves you with another well-named model. If the model is not anonymous or no such named equivalent can be found, then the input model is returned unchanged.

For example, { ...X, @remove prop: Y} where you use filterModelProperties to remove things marked @remove, then you are left with an "effective model type" of X.

EDIT This was updated to take a filter, which it applies first, and then ignores filtered properties when determining if the result matches a named model. This addresses a case that was discussed in the PR.

Using this for current OpenAPI emit

The OpenAPI emitter is changed here to use these to get better names for what's left after it filters out@statusCode, @header and friends. It only does this to anonymous models as we currently expect to use the Cadl name for the thing that has schema and non-schema properties as the OpenAPI name for the thing with the non-schema properties removed. But if it doesn't have a name, we can use these to try to find one instead of inlining it.

You can see the effect by comparing this example in the playground before and after this change:

Notice how we are able to recover the Widget name.

Next steps

The hope is that we can also build on these primitives to implement the metadata proposal. If that doesn't work out, there is still some value as seen above.

This change was not huge in the end, but I got it wrong enough times that I'm not super confident that there aren't more issues lurking. There were a lot of traps. I'm putting this up as a draft now and will work on related design proposals and we'll discuss at an upcoming design meeting. In the meantime, all feedback is welcome, and I especially would love to hear about any test cases you think I might have missed or any strangeness you can find in the playground!

@azure-pipelines
Copy link

You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/506/

@nguerrera nguerrera changed the title *NO MERGE* WIP Introduce primitive to filter model properties and get effective type May 4, 2022
@nguerrera nguerrera force-pushed the get-effective-type branch 2 times, most recently from 0ac9695 to ff199ac Compare May 4, 2022 21:46
Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

packages/openapi3/src/openapi.ts Outdated Show resolved Hide resolved
@@ -144,7 +144,7 @@ export type IntrinsicModel<T extends IntrinsicModelName = IntrinsicModelName> =
export interface ModelType extends BaseType, DecoratedType, TemplatedType {
kind: "Model";
name: IntrinsicModelName | string;
node:
node?:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when do you not have a node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I create a new type in filterModelProperties. I wasn't sure about this. I could maybe copy the node from the source model, but I recently found issues with places where we do that elsewhere. See #462. This will be a good thing to cover in design discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right I guess that make sense. I think then we might want to think of a sourceMap system so you can figure out where a type like that was introduced from

Copy link
Member

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

I think it adds some useful building blocks that we can use in any approach we finally decide on for metadata.

I did leave one small suggestion that I think will improve the handling for cases where the model includes a @Header.

packages/openapi3/src/openapi.ts Show resolved Hide resolved
@nguerrera nguerrera marked this pull request as ready for review May 16, 2022 15:58
Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I love that we'll be able to intersect metadata and model types now without polluting the OpenAPI schemas. This should make extensible library work a lot easier.

packages/compiler/core/checker.ts Show resolved Hide resolved
@nguerrera nguerrera merged commit 706e1c0 into microsoft:main Jun 3, 2022
@nguerrera nguerrera deleted the get-effective-type branch June 3, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants