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

Proposal for sidecar files #218

Closed
bterlson opened this issue Feb 2, 2022 · 26 comments
Closed

Proposal for sidecar files #218

bterlson opened this issue Feb 2, 2022 · 26 comments
Assignees
Labels
design:needed A design request has been raised that needs a proposal
Milestone

Comments

@bterlson
Copy link
Member

bterlson commented Feb 2, 2022

Generating high-quality clients across multiple languages can require significant amounts of metadata. One approach to this problem is just to say hey, add all your metadata to the service definition itself. The drawback is, I fear if you're generating code across say a dozen languages, the core API specification will be buried under a sea of metadata. Additionally, it requires coordination between the service authors and the client generators to share metadata definitions which may not be possible or desirable (e.g. because the API definition is provided by company A and company B is generating clients for their own use).

I propose we handle this scenario using side-car files which are able to add metadata to existing API specifications. Consider the following sidecar using the experimental projections feature:

// service.cadl
namespace SomeService;
model Pet {
  name: string;
}
@get op getPet(): Pet;

// typescript.cadl

// import metadata used by my custom TS emitter
import "myTypescriptMetadata.cadl";
using TSMeta;

projection SomeService.Pet#typescript {
  to {
    // emit an interface rather than a class
    @useInterface(Pet);
  }
}

Using the --include command line option we can then include this transform. Then, our custom emitter can enable the typescript projection when emitting a typescript client.

This approach has some nice benefits:

  1. The service metadata remains simple and focused on the general API shapes. Handling specific scenarios can move into a sidecar.
  2. The sidecars can apply metadata that the service knows nothing about. This would e.g. allow a team that owns the typescript emitter to annotate any services with additional metadata without sending a PR upstream.
  3. Conventions can be established to make this easier/automatic, e.g. dropping a file with a particular name next to a cadl service definition makes it get picked up automatically.

The nice part is I don't think this needs any new language support, although it might be nice to allow projections to omit the explicit to { } block if this becomes a common scenario.

/cc @allenjzhang

@timotheeguerin
Copy link
Member

Does --import not work for that already?

@bterlson
Copy link
Member Author

bterlson commented Feb 3, 2022

Yep, everything outlined here works already. This issue is more about, does this pattern seem good, should we bless it as a recommended pattern, should we work on establishing conventions/add tests/etc.

@markcowl markcowl added the design:needed A design request has been raised that needs a proposal label Feb 3, 2022
@markcowl markcowl added this to the [2022] March milestone Feb 3, 2022
@markcowl
Copy link
Contributor

markcowl commented Feb 24, 2022

  • Potential Use Cases
    • richer content for docs
    • Richer examples
    • client side 'higher level operations'
    • generated tests
    • multi-protocol specs
  • Additional considerations
    • verbosity of formatting may be a usability concern for some use cases [needs a proposal for an alternate format for documentation cases] {Allen}
    • versioning for sidecar metadata
      • refrerring to versioned models
      • doing different things for different versions

@allenjzhang
Copy link
Member

Here is an example using yaml to represent doc and examples injections. Behind the scenes will still be processed by projection, but this is more compact without the projection artifacts. The files can even be separated into doc.yaml, example.yaml to provide a concentrated view of the additional data.

This does not exclude using projections for more advanced or targeted projections.

SomeService
  Employee:
	@doc: |
	  This is very long doc string
	  2nd line
	  3rd line
	id:
		@doc: >
		  This is some description
		  that will be folded
  Employee.Get:
	@doc: |
	  This is very long doc string
	  2nd line
	  3rd line
	@example:
		- "GetExample1": ./examples/getEmployee1.json
		- "GetExample1": ./examples/getEmployee1.json

@allenjzhang

This comment was marked as duplicate.

@nguerrera
Copy link
Contributor

nguerrera commented Feb 25, 2022

I don't think we should do something with yaml. If we want less verbosity, more declarative or what not, then we should consider language features that we can tool, etc. For example, C# partial classes and methods have their attributes merged together, IIRC.

My fear here is that doing it with YAML won't be any easier but will have a lot of consequences. Years ago, for example FxCop suppressions were carried in a sidecar XML file and the end-to-end experience was pretty brutal IMHO.

@timotheeguerin
Copy link
Member

Thing good about projection is this is part of the language. Doesn't feel like some "hack" or patching job build after the fact(autorest directives).

Feel like there might be some simplification in the projection language we might want instead.

@bterlson
Copy link
Member Author

I had some conversations with Allen offline but will share my feeling here. YAML is extremely complex and combining with cadl would have a combinatorial complexity effect that I really want to avoid.

We should focus on goals and investigate native-cadl solutions to those. One example is making projections to implicit so you don't have to add that added indent. You could also make other projection elements optional/implicit, add a new syntax form for a more light-weight function that doesn't use self, or a "method-like" thing that has the members of what its attached to in scope, or even allow projection statements top-level. The solution space is rich so I think once we get crisp on goals we can find a way to leverage our existing machinery in a way that reduces verbosity without increasing complexity, sacrificing toolability, etc.

@allenjzhang
Copy link
Member

allenjzhang commented Feb 27, 2022

"The solution space is rich so I think once we get crisp on goals we can find a way to leverage our existing machinery in a way that reduces verbosity without increasing complexity, sacrificing toolability, etc."
💯

@mikekistler
Copy link
Member

Here's the proposed Overlay spec for OpenAPI (it's actually defined more broadly, but motivated by the OpenAPI use cases):

https://github.com/OAI/Overlay-Specification/blob/main/versions/1.0.0.md

@markcowl
Copy link
Contributor

markcowl commented Mar 2, 2022

Action items:

  • make 'to' block optional
  • Create specific design proposals for specific use cases that require additional / altered syntax [Brian]
  • Decide how projections correlate with emitters : what projection(s) are emitters expected to run? [Brian]
    • Universal projections [e.g. added metadata for new requirements]
    • Feature-specific projections (e.g. versioning, language translation)
    • Emitter-specific projections (e.g. user customization of emitters)

Future considerations

  • Add a lightweight function definition that could be called inside the projection
  • Merge decorators and functions
  • Allow sidecar files to simply specify decorator / function executions
  • Add a selector for namespace, to apply to all namespaces
  • For documentation, examples, it will be common to need to merge documentation into a versioned spec, will likely need a proposal/example for tersely describing docs and examples for versioned models.

Note on usage:

projection model#typescript {
 @doc(self.name, "My pet name")
 @doc(self.age, "My pet age")
}

will apply to all models. Similar selectors exist for:

  • model, interface, operation, union,
  • 'namespace' is a possible additional selector

@markcowl markcowl modified the milestones: [2022] March, [2022] May Mar 29, 2022
@timotheeguerin timotheeguerin changed the title Proposal for side-car files x Apr 28, 2022
@markcowl markcowl changed the title x Proposal for sidecar files Apr 28, 2022
@markcowl markcowl modified the milestones: [2022] May, [2022] July May 11, 2022
@markcowl markcowl removed this from the [2022] July milestone Jun 8, 2022
@markcowl
Copy link
Contributor

markcowl commented Jun 8, 2022

  • close for implementation issue

@markcowl markcowl added this to the Backlog milestone Jun 10, 2022
@bterlson
Copy link
Member Author

bterlson commented Jul 27, 2022

In the last few months I think we've learned more about how folks want to use this sidecar story and there are a few undesirable aspects to the "just use projections" approach:

  1. It brings in the additional complexity of projections and overloads the concept - the sidecar is really just using the projects as a function container (as evident by not needing the from projection)
  2. There is too much syntax
  3. Running the projection requires either an emitter to explicitly ask for it (necessitating some kind of convention), or some command line incantation to turn it on.
  4. The decorators run very late, meaning other parts of the Cadl program which might be impacted by the additional sidecar decorators won't see the decorated state.

Thus, I propose a new approach: augment decorators (name is horrible please suggest better names)

Syntax
The @@ sigil leads an augment decorator. It is a statement that can go anywhere other statements go. The augment decorator always takes at least one argument (the target) and so we can require this in the parser.

@@decorator(target, arguments)

For example, a sidecar file add-docs.cadl might look like:

@@doc(MyModel, "special documentation");

Combined with --include option on the command line, this enables factoring out use-case specific metadata into separate files that are included as needed.

Semantics
Augment decorators are run when the type they target is instantiated, and after any decorators that are attached to the declaration. This means that there should be no observable difference when a decorator on some type is instead moved to an augment decorator. This makes the side-car story super easy to reason about.

Implementation
In order to run augment decorators during type instantiation, we need to collect all augments across the entire program prior to doing any checking. We can do this work in the binder by building a map of target symbol -> augments. A complexity here is how to deal with late-bound symbols - we often don't know whether a model will have a property until we've computed its type via checking. We will likely need another map of [ late-bound container, symbol name ] -> augments to handle this case.

Projections already exposes decorators where the first parameter is the target. The projection syntax should be changed to align (which will also enable the possibility of declaring decorated types inside a projection). This is technically a breaking change but we yell at you if you use projections so I feel this is morally justified.

/cc @nguerrera @timotheeguerin

@nguerrera
Copy link
Contributor

+1 I like this proposal. My only question is where are augment decorators allowed to occur. Wherever a statement can? Only at top level?

I also like the name "augment decorator" fine. 🙂

@bterlson
Copy link
Member Author

I think it's just a statement yeah, updated proposal to reflect. I don't think we gain much by limiting it further than that, and it might be frustrating if it has to go only at top-level given when you want to have types in the sidecar file as well (and thus likely want to use a blockless namespace).

@bterlson
Copy link
Member Author

Another update to the proposal: projections currently allows @doc(target, "contents"), this should be changed to @@doc(target, "contents") to align with this proposal.

@timotheeguerin
Copy link
Member

I like it as well, the @@ syntax does feel a little weird but don't know anything better either so probably just need some getting use to.

@nguerrera
Copy link
Contributor

nguerrera commented Jul 27, 2022

Just thinking about alternative syntax. Not sure if it's better, but might as well write down an idea that occurs to me.

@doc("This is special")
@yadaYadaYada
augment MyModel;

We add:

AugmentStatement :
   DecoratorList? `augment` ReferenceExpression

Possible advantages:

  • We continue to have a keyword at the front of every statement, which is relied upon in places.
  • You can add multiple decorators to something without repeating its name for each one.
  • We don't need to add @@ or change projection syntax

Could also make tooling easier since we wouldn't have to special case @@ vs @ in things like signature help. (EDIT: But I guess we will need to handle the leading argument difference in projections still.)

@timotheeguerin
Copy link
Member

IMO using the same syntax for augment and the projection is a good thing. In one case you augmenting all the time in the other with a specific projection.

if decorator worked after that would also not look too bad

augment MyModel @doc("foo")

@bterlson
Copy link
Member Author

I was thinking along those lines as I was pondering how you might augment by adding or wrapping parts of the type, e.g. to augment a member to an array you could do something like:

model Foo { x: string }

@added
augment model Foo {
  @added
  augment x: x[];
  newProp: int32;
}

would be equivalent to

@added
model Foo {
  @added x: string[];
  newProp: int32;
}

This would enable adding multiple decorators inside a model without constantly repeating the same model name.

But this is starting to look pretty complex!

@bterlson
Copy link
Member Author

I feel like the change from @ to @@ in projections is a good thing regardless of what we decide here as it enables creating decorated declarations inside projections.

@nguerrera
Copy link
Contributor

But this is starting to look pretty complex!

Yeah, it's starting to look an awful lot like partial in C# and I don't think we want to sign up for that.

@bterlson
Copy link
Member Author

  • Concern: tooling wouldn't reflect side-car state making debugging difficult. There will be some design work to make this easy, but for now users can develop the augment decorators as regular decorators and factor them out later.
  • Do we want to gate these behind "feature flags" (i.e. do we somehow gate a sidecar file behind a flag name?)
  • Should we drop the @@ sigil and say decorators are "just functions".
  • We will likely need to add model properties/interface members going forward, but will tackle augment syntax with more experience.

@markcowl
Copy link
Contributor

removing dpg imnpact, asn this is covered by the client design issues (which are labeled as DPG Impact)

@tjprescott
Copy link
Member

@markcowl if this has been decomposed to implementation issues, is there a reason this design issue is still open?

@tjprescott
Copy link
Member

This has been implemented, so closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal
Projects
None yet
Development

No branches or pull requests

8 participants