-
Notifications
You must be signed in to change notification settings - Fork 204
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
Conversation
You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/506/ |
fa497b6
to
5d2b0a5
Compare
0ac9695
to
ff199ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
@@ -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?: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
e2123c2
to
1e4c114
Compare
There was a problem hiding this 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.
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:
Before
After
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!