Behavior of op test(...Model)

[timotheeguerin]

Dpg emitters have been confused by what we mean and what we want the generated code to look like

model User {
  @key id: string;
  @header lifetime: string;
  ...UserContent;
}

model UserContent {
  name: string;
  favoriteColor: string;
}

op test(...User): void

Here what we expected this too look like

Response test(User user);

however they have interpreted that as spreading properties but still combining the body together(which is another question of its own)

Response test(String id, String lifetime, UserContent userContent);

Problem

Now if we decide to keep our original meaning ...User means User is the input then we cannot represent the case where we actually want to spread the properties and this is something DPG teams have said is a behavior wanted

Basically how do I represent those 2 signature from the User model

Response userAsSpread(String id, String lifetime, String name, String favoriteColor);
Response userAsInput(User user);

Option 1: ...User still means input is User

That means we would need to create some new core type SpreadProps which would remove the reference to the User

op userAsInput(...User): void;
op userAsSpread(...SpreadProps<User>): void;

Option 2: ...User means spread

That means we would need a new syntax to specify the input (related to #1140)

op userAsSpread(...User): void;
op userAsInput(User): void;

cc: @m-nash

[johanste]

The introduction of a User model in the conceptual model of the service means that it is something that has a meaning beyond just being a grab-bag of various properties.

Thus, a generic op doSomething(...User): void operation is best represented in a client library as something that takes a User as input. If User had been an alias (and thus just a named bag of things that is convenient to spread around in the Cadl spec to avoid repetition), then it should have spread parameters.

Under what circumstances would it be preferable for a client library to not take a User as an argument if the input cadl had a named User model?

(please note that a given emitter may map things differently - e.g. an emitter that distinguishes between inputs and outputs would be free to manufacture an output type based on the input. But since different emitters will have different mappings of the service model into their design patterns, that is really an emitter concern and not something that goes into the conceptual description of the service)

[bterlson]

Without syntax for #1140 we can't really disambiguate the two cases clearly in Cadl syntax. But even with that syntax, the behavior is still not entirely clear as it moves the question into the model you spread (i.e., is UserContent a model we want to represent, or is it just a convenient way to compose the User model).

Going back to the thought we had in mind when we started using ...User in places, we thought that should be the same as the hypothetical op test(User) signature. And I think I mostly agree with Johan's analysis that this is fair. Since User exists as a model, then it seems fair to interpret that what you want is the userAsInput signature. But even then it also seems reasonable that an emitter might want to always pull out the key field and pass it as a positional - for some targets, this is a faithful interpretation of what it means to "take a User as input". This I think is just a special case of the general situation Johan points out, where an emitter understanding that the intent is the operation takes a user, may translate that intent into various representations that may or may not involve a User input type, and if it does, it may or may not have all the same members as what is declared in the Cadl.

Which leaves the question of, how would I override this behavior? One possible way, assuming the service has a concept of a user but for this method for whatever reason wants to be clear that the operation takes the same data a user holds but does not take a nominal User, might be op test(... { ...User }): void (or the equivalent involving an alias to avoid the annoying nested spread). A decorator like @preferPositional might also make sense. I'm not sure I like the template instantiation approach but it could have benefits.

All that said, I think a reasonable alternative approach is to say that ...User means you don't want to take a user but a user's properties, and if you want to take a user, you have to explicitly do op test(user: User): void. This feels cleaner for the simple case but it still requires some smarts to avoid using pointless envelopes on the wire - probably the user did not intend for the wire to contain an envelope, but maybe they did? It would need some override. And it would also be a bit mysterious what happens when you add another positional parameter - in that case we have to have an envelope on the wire, unless the target is http and the additional parameter is a header, query string, or path parameter. And I think it makes it harder to justify doing simple transformations like "to take a user means to take the id as a positional and the rest in some kind of options bag", perhaps requiring you to be explicit about that behavior in the cadl, adding verbosity removing the flexibility for some targets to use that convention and for others to avoid it. Even with these caveats, this is maybe a viable alternative if folks think it would make more sense.

[m-nash]

@johanste if I understand you correctly would you lean towards using the following to differentiate?

If we want an option bag
userAsInput(...User): void;
And then if we want them spread out
userAsSpread(@query id: User.Id, @header lifetime: User.lifetime, @body content: UserContent): void
And then if we want even the body spread out they would need to write
userAsSpread(@query id: User.Id, @header lifetime: User.lifetime, @body name: User.name, @body favoriteColor: User.favoiteColor): void

In other words there is no short hand for spread into parameters unless User happened to be an alias then the behavior changes?

[m-nash]

If we do want to change what ...User means we might consider changing what the compiler sends to the emitters.

Today if I set

I get the following from TypeGraph

Which indicates to me that I should have 4 parameters which doesn't match what I hear above which is it should be one parameter of type User.

If instead I set this

I now get the single param in the TypeGraph

This one indicates to me that I should use a single param of type User

[timotheeguerin]

I started a doc showing different Cadl operation styles, what it means in http context and what we'd want the client to be https://cadlwebsite.z1.web.core.windows.net/prs/1396/standard-library/rest/examples.html

[m-nash]

It appears you are leaning towards this as well then?

Spread operation means OptionsBag and if you don't want an options bag you must be explicit on your op definition?

Can you add an example that shows the case where I want to spread the query / header params but use a single param for the entire body?

[timotheeguerin]

Sorry, I entered those examples on how we envisioned things before this issue to represent the current state and make sure we cover all cases . It is still open for discussion.

will add that example

[bterlson]

If we do want to change what ...User means we might consider changing what the compiler sends to the emitters.

FWIW what we send to emitters is very strongly implied by the source code, I don't think we can change that. The behavior you want requires syntax like #1140. But it would be a fairly simple matter for a helper to construct the view you want in the meantime.

[m-nash]

I don't think I would advocate to change the meaning of "spread operator" to mean single parameter if its a model we are spreading and enumerated properties if its an alias we are spreading.

I was just trying to repeat back what I am hearing being suggested to make sure I understand what is being suggested.

[johanste]

In other words there is no short hand for spread into parameters unless User happened to be an alias then the behavior changes?

Correct. In general, it is my belief (experience?) you will find fewer compelling reasons to "spread" into individual parameters for any model types that are ever used as outputs. And if you do spread, it is more likely going to be a client-only overload rather than a separate service operation. Models that are used as input-only in multiple operations are less common and we should review their existence when we review the service API description (and make them into aliases if necessary).

For specific operations, a given emitter may have an established design pattern - e.g. python prefers spreading properties to keyword-only parameters over creating separate named models for json-merge-patch PATCH operations. The python emitter is responsible for said transformation of the signature.

It feels like what we are talking about is (at least partially) an override in how a given emitter generates code for a specific operation or model. Which feels like an augment decorator in an emitter-specific client side-car (i.e. cadl-dpg for our DPG emitters).

[nguerrera]

Regarding #1392 (comment)

There is a REST-layer API built on a similarly named compiler API that is used by the openapi emitters to recover a named model from an anonymous model consisting entirely of properties spread/sourced from something named. It handles more than just this top-level spread body parameter case. It also deals with things like op Foo(): NamedThing & { @header example: string }, which will appear anonymized without this step. With this step we determine that once we remove the header from the return type and put it elsewhere the property bag that we're left with is equivalent to NamedThing.

Calling this API is part of the documentation I am writing for #1083. I tried to move this call into the REST layer API that enumerates routes but that turned out to be too difficult in general because you have to do it recursively, but it is not currently tractable for the rest layer to recursively transform the core Cadl type graph before passing it along to emitters. It is also not feasible to change the core semantics so that these names are recovered automatically.

The API is here, next to other API that are useful to reason about metadata and visibility:

https://github.com/microsoft/cadl/blob/03a9438c1a64f04d92554b979ec6b94911093212/packages/rest/src/http/metadata.ts#L304-L308

What is happening when an alias changes the openapi behavior is that it makes the name recovery done by this API impossible since aliases are erased in the type graph.

My document will walk through a sample emitter much smaller than the openapi emitter that calls all of the relevant API.

[johanste]

Tagging @lmazuel since he showed an interest in this topic as well...

[timotheeguerin]

Summary of meeting

• op test(...Pet): void means the operation is taking a "anonymous" param of type Pet
  • use helper getEffectiveModelType to find that model
  • have to find a way to override the parameter name
• Discuss new decorators specific to csharp or dpg:
  • operationParamAsOptionBag: Way to say all the parameters should actually be grouped under a single operation
  • operationParamsPositional: Way to ignore the getEffectiveModelType and treat each parameter individually

[nguerrera]

I'm still working on the doc mentioned, but I have written a sample and some summary notes for the text that should help in the meantime. See #1083 (comment)

[nguerrera]

I believe the meeting summary above is enough to close this at this point. I have gathered that DPG emitters have already started using getEffectiveModelType effectively (couldn't resist the pun, sorry).

We also settled on https://github.com/Azure/cadl-azure/issues/2360 that using explicit @body is suffiicent to override the parameter name.

I think DPG decorators like @operationParamsPositional and @operationParamAsOptionBag can be discussed independently, and also DPG team should feel empowered to add them to cadl-dpg and consume them appropriately in emitters.

Let me know if you disagree and I can re-open, or even better, it might help to split off new issues that are more targeted at work that is still needed.