Add async support

[dennis-gr]

Add async method support via AsyncGenerator [https://github.com/maca88/AsyncGenerator], fixes 47.

Overall the generated code looks good, all tests are passing (since I'm working primarily with MySQL,
I've switched the provider used by the tests to MySqlConnector, as MySql.Data has issues with its async support).

Issues:

• The ResultsAsync methods for classes inheriting from AbstractRevisionsQuery aren't properly generated. I'm not sure if it's an issue with my AsyncGenerator configuration or a bug/limitation in AsyncGenerator. Maybe @maca88 can take a look at this?

• The async generation for the test project outputs same warnings, but it doesn't look like anything major.

[RogerKratz]

Hi
Dont know much about asyncgenerator. What is suppose to call the yml file?
Had a look at nhib-core now but I'm not sure if I fully understand.

[dennis-gr]

Hi Dont know much about asyncgenerator. What is suppose to call the yml file? Had a look at nhib-core now but I'm not sure if I fully understand.

I'm using the dotnet tool version: dotnet tool install -g CSharpAsyncGenerator
You can then run async-generator in the directory where the configuration file is located.

Later on it'll probably make sense to have the async generation run as part of the workflow like NHibernate does.

[maca88]

The ResultsAsync methods for classes inheriting from AbstractRevisionsQuery aren't properly generated. I'm not sure if it's an issue with my AsyncGenerator configuration or a bug/limitation in AsyncGenerator.

AsyncGenerator has a logic that skips converting method calls inside a LINQ query, which is fine for most cases (e.g. async is not allowed in where statement). In this particular case, BuildAndExecuteQuery method could be converted to async because it is part of the in keyword. In short, this is a limitation of AsyncGenerator that could be fixed. As a workaround, you can just move the BuildAndExecuteQuery method call outside the LINQ statement (file: AllEntitiesAtRevisionQuery.cs):

var query = BuildAndExecuteQuery<IDictionary>();

// the result of BuildAndExecuteQuery is always the name-value pair of EntityMode.Map
return from versionsEntity in query
			select (TEntity)EntityInstantiator.CreateInstanceFromVersionsEntity(EntityName, versionsEntity, _revision);

I'm using the dotnet tool version: dotnet tool install -g CSharpAsyncGenerator

Usually it is better to use csharpasyncgenerator.tool instead as it can be configured per project with a specific version. Example from NHibernate (the file has to be in the .config folder):
https://github.com/nhibernate/nhibernate-core/blob/master/.config/dotnet-tools.json

then you need to restore the tools by using dotnet tool restore (one time only) and after that you can generate the code by executing dotnet async-generator command in the Src folder.

[dennis-gr]

Thank you for your suggestions! I'll update the pull requests in the next few days.

[RogerKratz]

@maca88 In nhib core, what was the reason generated code by asyncgenerator was committed to the repo? In general, it makes more sense not commit that I would say. Maybe I miss something?

[hazzik]

@RogerKratz overall, it was committed to the repo to spot issues like this:

The ResultsAsync methods for classes inheriting from AbstractRevisionsQuery aren't properly generated.

[dennis-gr]

I've updated the PR with @maca88's suggestions and the methods are being generated correctly now.

[RogerKratz]

@RogerKratz overall, it was committed to the repo to spot issues like this:

@hazzik Sorry, I'm sure I miss something, but why would that easier be spotted easier if the code was committed? Wouldn't it be just as easy (or hard) to spot if async code was generated explicitly locally (and in build pipe of course)?

[hazzik]

I'm sure I miss something, but why would that easier be spotted easier if the code was committed?

Because you can see and argue about committed code here on GitHub. It is just easier to make a comment.

When code is not committed, it is really hard: one needs to create a snippet, post it to GitHub, and proof that the code not properly generated (if it is) not because of misconfiguration or other issues of the local environment, but because of a bug in AsyncGenerator itself, for expample.

Take a look at comments at this PR to better understand what I mean: nhibernate/nhibernate-core#3139

Also with committed code it is easier to see changes, for example, when version of async generator is updated. And sometimes the async code was/is unstable.

[RogerKratz]

@hazzik
Ok. Makes sense!
In there any check (in nhib core) to make sure asyncgenetor has been run on latest code?

[dennis-gr]

@hazzik Ok. Makes sense! In there any check (in nhib core) to make sure asyncgenetor has been run on latest code?

It is part of the workflow: https://github.com/nhibernate/nhibernate-core/blob/master/.github/workflows/GenerateAsyncCode.yml

[RogerKratz]

Thanks.

• This needs to be part of the build pipe (dotnet.yml) I assume
• When creating a release, which is explicit, local action, these files needs to be included (default.msbuild)

can you add this code generation to these steps please?

[dennis-gr]

Thanks.

* This needs to be part of the build pipe (dotnet.yml) I assume

* When creating a release, which is explicit, local action, these files needs to be included (default.msbuild)

can you add this code generation to these steps please?

Done. I've copied and modified the GenerateAsyncCode.yml from NH, not sure if the NHIBERNATE_BOT_TOKEN will work here though.

[hazzik]

not sure if the NHIBERNATE_BOT_TOKEN will work here though.

It would probably not. Someone (@RogerKratz) would need to generate PAT and put it into the secrets. It would not work with standard GITHUB_TOKEN as it does not have right permissions.

[RogerKratz]

Thanks.

* This needs to be part of the build pipe (dotnet.yml) I assume

* When creating a release, which is explicit, local action, these files needs to be included (default.msbuild)

can you add this code generation to these steps please?

Done. I've copied and modified the GenerateAsyncCode.yml from NH, not sure if the NHIBERNATE_BOT_TOKEN will work here though.

Thanks. Not still 100% of how the process of this code generation is supposed to be.

Some ideas...

• When doing a release (default.msbuild) stop procedure/throw if any code is generated?
• In dotnet.yml, generate code, stop precedure/throw if any code is gerenated?

Would force explicit commits of generated code. Then GenerateAsyncCode.yaml isn't needed.

Thoughts?

[dennis-gr]

My 2 cents:

Async code should be generated for each PR for the reasons @hazzik outlined above. Ideally, all code changes are done through PRs, so there should be no async code left to be generated when doing a release.
Maybe also consider switching from doing releases manually to a workflow using Git tags where async code generation can integrated too.

[RogerKratz]

Hi

Was thinking a bit last night about this... Maybe the easiest, at least to start with, is to simply fail build pipe/workflow (on all branches - both master, PRs etc)?

Historically, not all changes (far from) has been made in PR. Me personally has done a lot of trunk based development.

To get rid of doing manually releases would of course be good, but think we should leave that out of this PR.

[dennis-gr]

Was thinking a bit last night about this... Maybe the easiest, at least to start with, is to simply fail build pipe/workflow (on all branches - both master, PRs etc)?

Historically, not all changes (far from) has been made in PR. Me personally has done a lot of trunk based development.

Maybe we should take a step back and not overthink this: If you create a token for the workflow like @hazzik mentioned, the workflow will take care of generating code for PRs i.e. external contributions are covered. That seems to be working pretty well for NHibernate. If you're working on the master directly, AsyncGenerator will be invoked when a release build is created, which should be fine for the time being. I'd suggest creating an issue for further discussion on improvements for that.

To get rid of doing manually releases would of course be good, but think we should leave that out of this PR.

Agree, that's an issue for another day.

[RogerKratz]

I tried to create a personal accesss token for envers but... My account seems to have no access to that for this repo. Or I am looking at the wrong place. Not sure.

[hazzik]

I've checked the secrets. This repo should have access to NHIBERNATE_BOT_TOKEN so it should just work.

[RogerKratz]

ok, maybe this pr is ready to go now then?
dont know how/if i can validate the generateasynccode workflow file until after this has been merged though, but let's hope it works :).
A last question, now when there will be multiple workflow files, is there any predictable order of them? I might be wrong, but I thought it wasn't? And if so - isn't that an issue? We want generateasynccode.yml to run before dotnet.yml, right?

[hazzik]

A last question, now when there will be multiple workflow files, is there any predictable order of them? I might be wrong, but I thought it wasn't?

You are not wrong. The workflow are usually started simultaneously, or queued in a random order (basically on agent availability).

And if so - isn't that an issue? We want generateasynccode.yml to run before dotnet.yml, right?

Not an issue. What would happen is that if async generator would run the workflow would add a commit to the PR with the generated code changes. Then a new set of workflows will be triggered for this new commit.

The second run should not produce a new commit.

So worst case you’ll have a wasted workflow run. I don’t think that is an issue at all.

dont know how/if i can validate the generateasynccode workflow file until after this has been merged though, but let's hope it works

In theory you should be able to push this branch to the repo without merging the PR and run the workflow from that branch.

[RogerKratz]

Not an issue. What would happen is that if async generator would run the workflow would add a commit to the PR with the generated code changes. Then a new set of workflows will be triggered for this new commit.

Sure, make sense.

[RogerKratz]

Thank you!
Squashed and rebased.