Source Generator? - Githubissues

christiannagel commented 2 years ago

I think the MapInstantAPIs method could be implemented with a source generator. This might also make it easier to create customizations with partial methods.

What do you think?

csharpfritz commented 2 years ago

I think you're on to something.. tagging @jasonbock who expressed some interest in exploring this as well

JasonBock commented 2 years ago

I definitely think source generators is an avenue to explore.

JasonBock commented 2 years ago

Definitely had some success tonight :)

https://twitter.com/jasonbock/status/1494527162585649153

csharpfritz commented 2 years ago

GREAT progress... thank you @JasonBock !

What if WebApplication.MapMyContextToAPIs() is a concrete extension method that takes the configuration and knows how to call into the generated code? Something like:

public static WebApplication MapMyContextToAPIs<D>(this WebApplication app) where D : DbContext {

  var generatedMapper = new GeneratedContextMapper<D>(MapConfiguration config);
  generatedMapper.MapTheAPIs();

}

public partial class GeneratedContextMapper<D> where D : DbContext {

  private MapConfiguration _Config;

  public GeneratedContextMapper(MapConfiguration config) {
    _Config = config;
  }

  public partial void MapTheAPIs();

}

We could in the generated code check the generic type and only emit mapping statements for that context. With a configuration passed in, the output code could decide which methods to map and more configuration goodness.

Then... we could also provide partial method signatures for OnBefore and OnAfter methods folks may want to call before and after the database interaction in an API

JasonBock commented 2 years ago

Hmmm....I'm sort of following what you're saying here...but I'm also kind of confused as well. I don't know what you mean by "concrete extension method". Also, if it "takes the configuration"....would that be a parameter to the extension method?

BTW I don't think that D generic parameter is necessary for the generated code - it already knows what the context type is.

JasonBock commented 2 years ago

Maybe try a different approach. Tell me what you want the configuration to do without necessarily thinking about how the SG will accomplish that, and then we can go from there.

I definitely see the need for configuration and customization, so I want to make sure the initial approach is going down the right path. (I also get a little leery with "I need to customize all the things!" because that can quickly end up in a quagmire. I mean....you're not saying that, but as soon as you add in a bit of configuration customization, a whole bunch of folks can come in and ask for all sorts of switches and it spirals out of control :) ).

JasonBock commented 2 years ago

I looked at what you're currently doing with InstantAPIsConfig, and I think a very similar approach can be done with the SG. For example, we could have something like this:

public static partial class WebApplicationExtensions
{
  public static WebApplication MapMyContextToAPIs(this WebApplication app, InstanceAPIConfig configuration = null)
  {
    if(configuration is null) { configuration = InstanceAPIConfig.Default; }

    if(configuration.ShouldGetAll)
    {
      app.MapGet(configuration.GetRoute("Contacts"), ([FromServices] MyContext db) =>
        db.Set<Contact>());    
    }

    if(configuration.ShouldGetById)
    {
      app.MapGet(configuration.GetRouteById("Contacts"), async ([FromServices] MyContext db, [FromRoute] string id) =>
        await db.Set<Contact>().FindAsync(int.Parse(id)));
    }

    return app;
  }
}

public class InstanceAPIConfig
{
  public static InstanceAPIConfig Default => new();

  public virtual string GetRoute(string tableName) => $"/api/{tableName}";

  public virtual string GetRouteById(string tableName) => $"/api/{tableName}/{id}";

  public virtual bool ShouldGetAll => true;

  public virtual bool ShouldGetById => true;
}

Developers could create their own configuration class for a given context:

app.MapMyContextToAPIs(new MyContextConfig());

Or, they can just go with the defaults:

app.MapMyContextToAPIs();

BTW that configuration class I have defined isn't completely fleshed out, and needs more thought around the best way to handle that.

csharpfritz commented 2 years ago

The reason to pass the type of the DbContext as generic parameter D is to ensure the correct DbContext is inspected and APIs are generated.

Yes, the If..then..else approach you outline is exactly the type of thing that I was thinking to embed in the generated code.

csharpfritz commented 2 years ago

By 'concrete extension method' I mean an extension method that is already written and compiled, and not one that we generate

JasonBock commented 2 years ago

The reason to pass the type of the DbContext as generic parameter D is to ensure the correct DbContext is inspected and APIs are generated.

I still don't think that's necessary though when this is source generated. We've already made a determination in the SG that the type is DbContext-based.

Yes, the If..then..else approach you outline is exactly the type of thing that I was thinking to embed in the generated code.

OK, lemme spend some time adding this idea to the SG and I'll make another commit, see if that's going in the direction you're thinking.

csharpfritz commented 2 years ago

Right.. what if there are multiple DbContext's in the assembly or referenced assemblies? A developer would want to be able to focus the code generation to just the context that they want to expose APIs for

JasonBock commented 2 years ago

Look at the GenerateWhenMultipleDbContextsExists() test ;). I already have it implemented such that a Map...ToAPIs() method is made for each DbContext. If the developer doesn't want to map a specific DbContext, they just don't call the extension method.

Side note: right now, I have this looking for a ClassDeclarationSyntax - that is, the class deriving from DbContext has to be defined in the assembly that is referencing the SG. For the MVP in place, that's fine, because it's all in one project, but realistically, the context will be in another assembly that has no references to the web-related assemblies. One way to handle that is to have an assembly-level attribute that lets the developer specify which context they want the extension method generated for, like this:

[assembly: InstanceAPIsFor(typeof(CustomerContext))]

JasonBock commented 2 years ago

I think I have a prototype working for configuration, I'll push my latest changes later tonight when I clean up a couple of things.

csharpfritz commented 2 years ago

I had some content building in the feature_Config branch in an effort to declare a configuration strategy

Jeff

On Feb 18, 2022, at 16:15, Jason Bock @.***> wrote:

I think I have a prototype working for configuration, I'll push my latest changes later tonight when I clean up a couple of things.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

JasonBock commented 2 years ago

I committed this....and then I saw the comment :). I'll look at that branch and see what you did there.

That said, the latest commit adds InstanceAPIGeneratorConfig. You can see in Program.cs how you can create custom configuration to change the routes or disable a mapping.

csharpfritz commented 2 years ago

I bet there’s a way to merge these ideas. We’re throwing a bunch of prototypes right now… and we can pull together to a unified API definition

Jeff

On Feb 18, 2022, at 17:53, Jason Bock @.***> wrote:

I committed this....and then I saw the comment :). I'll look at that branch and see what you did there.

That said, the latest commit adds InstanceAPIGeneratorConfig. You can see in Program.cs how you can create custom configuration to change the routes or disable a mapping.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

JasonBock commented 2 years ago

It looks like what I'd need to my branch is the idea that you can include/exclude tables via configuration. Which makes sense, but I'm not getting my head around why there are explicit "Include" and "Exclude" lists. I get that you're not allowing a table name to be in both lists at the same time (at least I think that's what you're doing in your code), but....what happens if I have two tables, Product and Customer, and I include Customer but I never explicitly say anything about Product. Does that get excluded or included? If I'm following what I see in that branch, it seems like once I either include or exclude a table, I now have to specify all tables to be either excluded or included. It seems like you'd want to either opt-in all the tables and let users exclude specific ones, or opt-out all tables and let users include specific ones (and I'd personally go with the first one).

Either way, I'll try to get something in my branch to reflect this idea. I'll also add in other API verbs as I noticed you had added others as well.

csharpfritz commented 2 years ago

Maybe we’re thinking about this wrong…

Maybe this shouldn’t be a source generator… maybe it should be a parameterized template run at the command line like the other EF tools?

On Feb 19, 2022, at 10:01, Jason Bock @.***> wrote:

It looks like what I'd need to my branch is the idea that you can include/exclude tables via configuration. Which makes sense, but I'm not getting my head around why there are explicit "Include" and "Exclude" lists. I get that you're not allowing a table name to be in both lists at the same time (at least I think that's what you're doing in your code), but....what happens if I have two tables, Product and Customer, and I include Customer but I never explicitly say anything about Product. Does that get excluded or included? If I'm following what I see in that branch, it seems like once I either include or exclude a table, I now have to specify all tables to be either excluded or included. It seems like you'd want to either opt-in all the tables and let users exclude specific ones, or opt-out all tables and let users include specific ones (and I'd personally go with the first one).

Either way, I'll try to get something in my branch to reflect this idea. I'll also add in other API verbs as I noticed you had added others as well.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

JasonBock commented 2 years ago

Actually, let's not nix this just yet :). I just got back from running errands and I think I have a way to do this. I have a self-defense lesson around lunch, but I have lots of time this afternoon to try it out ;).

csharpfritz commented 2 years ago

Understood... I suggested the template tool in an effort to better understand and control the creation / regeneration and management of the code this generates.

Looking forward to your ideas.

Also: I'm proposing a flags enum in the feature_Config branch to allow definition of the methods to generate. I'll wrap that branch and share to main so that we can start basing some of the other proposed features on that

JasonBock commented 2 years ago

OK! My latest updates now have something in place. If you look in Program.cs you should see how you can do configuration. It's not quite the same as what you have, but it does work (and thanks for adding in Swagger :) ).

Don't run the tests just yet - most are failing because I need to change what the expected generated code is. But you can play with the same app if you want.

JasonBock commented 2 years ago

Oh....you now have the workflow running tests :). Well, that'll fail for a little while!

csharpfritz commented 2 years ago

Sorry about that... saw the message after I clicked run. 🤦

I just finished working through an InstantAPIsConfigBuilder that would resolve the list of tables so that the generated code can just iterate through the list of tables... it does use reflection to examine the list of tables available and de-dupe.

I'll switch to your table config definition and modify so that it returns just the resolved list of tables

JasonBock commented 2 years ago

OK, now all the tests should pass.

JasonBock commented 2 years ago

And now all the API verbs are being generated :)

JasonBock commented 2 years ago

Now that I've got things as "far" as I think I can go at the moment (I'm sure there's a lot more that can be done, but for the MVP, it's pretty complete), I wanted to discuss how viable this is.

Some advantages:

The ID parsing code is wired directly in. The Reflection approach has to do that lookup every time.
I can reference the properties on the DbContext directly, like this: db.Contacts, rather than db.Set<Contact>. I'm not an EF expect, so I don't know how much of a big deal that is, but...it seems more "direct".
There's no Reflection in setting up the Map...() calls, but to be fair, that's only done once at start-up.
It was mentioned before that partial methods here may make it easier to extend what the mapping is doing. I personally don't know what the specifics are with this, but this might be easier to do with a SG approach.

Some disadvantages:

Source generators are not trivial to author. Consuming them is just fine, but it can be a bit tricky to get the project/package done just right.
I'm using an IndentedTextWriter to generate the code. I'm fine with it, but there are other options to use (e.g. Scriban). That may make things easier.

What's unclear at this point as well is just how much perf a SG approach will make. The API wiring only happens once at startup, and there's also time that the SG takes to create code. If there's things within the APIs that require Reflection, then the SG may end up being the "winner" because it can (possibly) wire those things into the code.

JasonBock commented 2 years ago

So, the latest changes to the PR include having Results being returned as well as using IEndpointRouteBuilder instead of WebApplication. I'm not sure if there are any other features to get up to parity with, I think that's it's for now. If there are others, let me know.

I looked at the configuration stuff that is currently being used, and I think we need to have a shared abstraction, or at least use the same generalized principles. What you have right now works well for the Reflection approach, but for the SG one, I don't think it fits. I don't need to get an instance of the DbContext, and the TypeTable class uses a Type, which isn't a great fit for the SG code as well. I like the approach of using a builder with options, and that creates the configuration. I'll see if I can change what I'm doing to mimic that.

csharpfritz commented 2 years ago

Do you think we're at a place where its time to merge this branch?

Jeff

On Sat, Feb 26, 2022 at 10:08 AM Jason Bock @.***> wrote:

So, the latest changes to the PR include having Results being returned as well as using IEndpointRouteBuilder instead of WebApplication. I'm not sure if there are any other features to get up to parity with, I think that's it's for now. If there are others, let me know.

I looked at the configuration stuff that is currently being used, and I think we need to have a shared abstraction, or at least use the same generalized principles. What you have right now works well for the Reflection approach, but for the SG one, I don't think it fits. I don't need to get an instance of the DbContext, and the TypeTable class uses a Type, which isn't a great fit for the SG code as well. I like the approach of using a builder with options, and that creates the configuration. I'll see if I can change what I'm doing to mimic that.

— Reply to this email directly, view it on GitHub https://github.com/csharpfritz/InstantAPIs/issues/12#issuecomment-1052178141, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAATF4KP7ZWYACJHQZ3HZ2DU5DUGFANCNFSM5OVZVVBQ . You are receiving this because you commented.Message ID: @.***>

JasonBock commented 2 years ago

I think we're close. Here's what I think is remaining:

Change the SG approach to use a builder that eventually create the configuration.
Have a separate "WorkingAPI" project for the SG. Every time I update my fork, my Program.cs file complains because of merge conflicts, and I think it'll be easier if there's a separate project there.
Add tests for Fritz.InstantAPIs.Generators.Helpers. This won't be hard.

Part of me wants to do this for my stream next Thursday, but I also don't want to keep this in a separate PR in perpetuity either :). So I may just do this over the weekend.

JasonBock commented 2 years ago

So, at this point, @csharpfritz I think it's ready to have the PR merged in, if you want. If there's anything else you want done, just let me know.

csharpfritz commented 2 years ago

Source generator merged in! I will add some docs for this

csharpfritz / InstantAPIs

Source Generator? #12