UpdateableProperties<T> does not include properties recursively or in base types

[daviwil]

This is a pretty big limitation in how we model update operations right now in @cadl-lang/rest, @azure-tools/cadl-azure-core and @azure-tools/cadl-azure-resource-manager.

We need to be able to find all updateable properties in a model like this:

model BaseModel {
  @visibility("read", "update")
  baseProp: string;
}

model SecondaryModel {
  @visibility("read", "update")
  frob: string;
}

model RealModel extends BaseModel {
  @visibility("read", "update")
  name: string;

  @visibility("read", "update")
  other: SecondaryModel;

  @visibility("read")
  cantChangeMe: string;
}

Some things to consider:

• Should we take a more "traditional" approach and use cloneType to rebuild the graph of models starting from T?
• Or should we use projections instead since the former approach is basically the same thing?
• Possibly exclude discriminator properties from updateable properties set (or mark it as not updatable)

/cc @johanste

[nguerrera]

The visibility applied by #687 during is actually recursive, but that is also the source of the huge challenge that has made it drag on for so long. :(

Also, there's the whole issue we discussed of needing to push this down into another layer than during the Cadl -> Open API schema conversion for use in other emitters, that #687 doesn't solve.

[bterlson]

@nguerrera do you have a link to that discussion?

[nguerrera]

Also, neither #687 nor the metadata proposal deal with making properties optional when applying update visibility.

I was contemplating attempting to do the transformation on the Cadl graph to push it down but I recall that we said projections weren't right for this but I forget the details.

@bterlson I don't have a link, but it was a long portion of the review meeting of my notes on the draft PR and you were there. There was brainstorming about Cadl->Cadl vs. applying visibility as part of Cadl->OpenAPI but that is all.

[bterlson]

I recall the meeting but was hoping something was more documented. Was just talking with @johanste about this issue and we kinda came to the conclusion that "just take T and let the emitter sort it out" was the path of least resistance that is also easiest for API authors, but it makes me a little nervous because it breaks layering - to the extent possible downstream emitters should see the result of the visibility/patchable/etc. in types rather than that logic being emitter specific...

Do you recall any possible solutions proposed in the meeting? E.g. you could imagine an emitter-time equivalent of a mutating decorator where the emitter can just cram new computed types into the graph and any emitters that run later see that, but that approach has a lot of problems and limitations...

[nguerrera]

The layering breaking is keeping me up at night, to be honest. There's a sunk cost fallacy keeping me biased but I'm terrified that we'll end up throwing it away to get something that works cross-emitter.

[johanste]

Was just talking with @johanste about this issue and we kinda came to the conclusion that "just take T and let the emitter sort it out" was the path of least resistance that is also easiest for API authors, but it makes me a little nervous because it breaks layering

Yeah, it is not ideal. But in the short term, it should unblock emitters. Some will completely ignore the updatability, some will just update type annotations etc. And what we really need in the short term is a json-merge-patch representation of (the transitive closure of) a given model. Which @bterlson was helpful enough to call out that it is not just Optional<Updatable> but, in fact Nullable<Optional<Updatable>> with special understanding of arrays (e.g. Updatable<T[]> === T[])

[nguerrera]

I think different emitters will want different graph transformations. For example, in openapi visibility("read") is readonly: true and can be shared with other visibilities, but another emitter may not want this sharing. I think we want an emitter to express the deep transformation it wants to something lower that carries it out.

The deep transformation is not a straight deep-clone-and-filter. You want to share types that don't get modified or that get modified the same way for multiple visibilities.

We already have issues when we use spread-and-decorate to create mutated types where you get unwanted dupes. KeysOf<ParentOfT> !== ParentKeysOf<T>.

I think for something like Updateable you want #521 so you can return existing types.

I recall now that it was when we discussed that issue that we said projections were not able to fill this need.

[nguerrera]

And what we really need in the short term is a json-merge-patch representation of (the transitive closure of) a given model

For this short term need is it acceptable to:

1. fail if there is a cycle?
2. inline the whole closure in OpenAPI the way the top-level is currently inlined, IIRC? Edit: no I think we did XxxUpdate friendly name now, right?

[nguerrera]

I ask because I think the short term need can be easier than some of the things I'm talking about if it's yes and yes.

[bterlson]

Projections clones the entire graph because as soon as a modification occurs on the leaf, things which refer to it need to be modified to point to the new type, and this recurses all the way to the root. It also simplifies a fair amount of logic. You probably want a system where A referring to B when B is mutated, A remains the same. Early in the projections journey we considered a cursor-style approach which could conceivably handle this kind of thing (essentially looking each type you come across up in a map) but has a ton more complexity for everyone involved.

But let's say we had such a mechanism - would it even solve the layering violation? Wouldn't downstream emitters have to know to "ask for" the proper view of the world as constructed by some higher level abstractions?

[nguerrera]

A referring to B when B is mutated, A remains the same.

Did you mean "A referring to B, when A is mutated, B remains the same"?

This is what we want in the OpenAPI emit of metadata proposal. We want to bubble up, but we don't want to clone the whole graph and create more types than necessary.

I think I'm thinking about something different in terms of layering violation. I would want such a facility so that I could at least point others to it who need to do the same type of computation. That seems like it would be useful even if it not as a way to just transparently insert yourself into a chain and make magic happen.

[bterlson]

Is that not solved by exposing the utility functions used to compute the types you need?

[nguerrera]

Maybe. I don't know exactly what the utility functions should look like. For the issues I faced, it wouldn't be sufficient, for example, to make filterModelProperties recursive where it is smart enought to bubble changes up while sharing elsewhere. Because we would still create multiple types if two different filters applied to the same model produced the same result.

[nguerrera]

It could simplify things if we don't need that. Specifically for metadata, if we could say once something changes for visibility, it creates a new type for itself and everyone who refers to it. If another visibility produces the same set of properties, too bad, you get another copy of this and everything that refers to it. I think this would produce correct but suboptimal results. I'm now tempted to make a quick version like this just to see how much less code there is and how much more reusable that is and then compare it to the current draft in terms of how the output looks with various examples. If that was enough and we generalized filter to a callback that can either something that could remove or change its type, it could maybe be reused for this too. And then if we did #521 we could wire calls to this helper to template instantiation.

[nguerrera]

Hmm, no, I don't think what I was imagining to try "quickly" would work, lol. You still need to be able to reuse past results even if you don't do it for different visibilities. Because you'll end up asking for the transformation on the same subgraphs. But maybe if you had something like:

function createModelTransformer(propertyCallback): ModelTransformer;
interface ModelTransformer {
   getTransformedModel(model: ModelType): ModelType;
}

where propertyCallback takes a property and returns no-op or request to delete/make-optional/etc. (Didn't want to bother trying to express that as code in a comment)

And then ModelTransformer memoizes results so it goes deep down the graph and bubbles up, but if you ask it for a subgraph its already processed it returns that.

I've probably hijacked this thread and I'm sorry for that.

[johanste]

The memoizing mutator is the path that I was considering after (quickly) giving up on projections. Using evaluateCadl of course! (not really)

[nguerrera]

Trying something along these lines and if it works, it will simplify my code a lot and provide a path to do things like this. But it doesn't work yet, so no promises. :)

[daviwil]

@nguerrera Any update on this? @johanste was asking me about it today.

[nguerrera]

This morphed into the "realm" feature that @timotheeguerin, @bterlson and I discussed. Working on writing something up. My attempt at a quicker feature did not succeed.

[markcowl]

@markcowl Need current status update

[nguerrera]

In the end, the metadata proposal was implemented without this realms feature.

With this implemented, the openapi3 emitter will automatically and deeply filter "update" visibility for PUT and PATCH operations. See https://cadlwebsite.z1.web.core.windows.net/docs/standard-library/rest/operations/#automatic-visibility. A wrapper UpdateableProperties<T> is not needed for that and the intent is that you can use logical T directly.

This hasn't been ported to cadl-autorest, but a PR for that should be up soon. Tracking issue: #1070.

But one thing the metadata proposal did not address is making properties optional as OptionalProperties<T> does. It's possible but to work this into the automatic transformation that happens for visibility. That might be the most practical.

https://github.com/Azure/cadl-azure/issues/2144 is dicussing getting back to the logical resource type in an emitter, which may also be relevant.

[nguerrera]

@daviwil I think we can close this one, right? We did the thing where we make the properties optional recursively using the metadata layer, right?

[markcowl]

Closing @daviwil Please open a new issue for any lingering issues with PATCH