Add none generic service middleware registration methods in Autofac

[idiotsky]

These changes introduce new methods to register service middleware in the Autofac library. Also, a new test has been added to Autofac.Specification.Test to ensure that the middleware is getting invoked correctly when a service is registered. These methods allow for more precise control of the pipeline during the resolve request process.

[alistairjevans]

Hi @idiotsky, can you please elaborate on the purpose of these new methods? What issue are you trying to address?

Typically we'd raise an issue to discuss a feature request or bug before we open a PR.

[alistairjevans]

Ah, ok, this is for #1416.

[idiotsky]

does my change broken the build, on my side all the test passed? looks the unit test failed not related to my code

[tillig]

I mean, this isn't a question I can answer off the top of my head, it's just what we see:

• The build has been stable for all other PRs.
• The build failed on this PR.

Unclear if it's due to a problem in the test or something in this PR or what. But that's what I see - the same thing as you.

I kicked off the build again to see if it consistently fails.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.51%. Comparing base (4547d16) to head (2cddfd3).
Report is 36 commits behind head on develop.

Files with missing lines	Patch %	Lines
...Autofac/ServiceMiddlewareRegistrationExtensions.cs	66.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1417      +/-   ##
===========================================
- Coverage    78.52%   78.51%   -0.02%     
===========================================
  Files          200      200              
  Lines         5713     5719       +6     
  Branches      1168     1168              
===========================================
+ Hits          4486     4490       +4     
- Misses         714      716       +2     
  Partials       513      513              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tillig]

Build seems to be passing now, but it appears the coverage is unhappy.

I haven't gotten around to reviewing this yet (but it looks like it's reasonable); I did see there was one unit test added to the RegistrationOnlyIfTests which seemed an odd place to add tests for a middleware registration method. After doing some searching, it appears we actually don't have specific tests about RegisterServiceMiddleware, so that seems like a shortcoming.

Here's what I'd say:

• Add a class Autofac.Specification.Test.Registration.ServiceMiddlewareTests.
• Put your tests for your new registrations in there. They should be named like RegisterServiceMiddleware_ScenarioNameHere since that's the method they're testing.
• Add at least one test for each of the new overloads so the overloads get exercised. The test should verify that the registered middleware ran as expected.

That should take care of the coverage issue and give us a place where we can put additional tests as needed.

[tillig]

It looks like this has been sitting for a while. My understanding of the status:

• We need some new tests to validate the methods, as indicated by the missing coverage numbers.
• The title on the PR should be updated a little - I don't know what "none generic service middleware" is (is it "non-generic?" is it a typo for "more generic?"). That's important so we can quantify it in release notes.

We'd love to get this into a new release, but it does need a little more work.

If we don't hear back in, say, a month? By end of October 2024? Then we'll close it and not integrate it.

[tillig]

Since we haven't heard anything and we're in December 2024 now, I'm going to close this.

[idiotsky]

I simply copied the three old RegisterServiceMiddleware overload methods and made the generic TService a parameter called serviceType. I ran the code coverage locally because this method doesn't have any existing tests or code coverage, and as a result, the related methods also lack code coverage. Could you please add code coverage for this method so I can add tests as well?
and can we reopen this?

public static void RegisterServiceMiddleware<TService>(this ContainerBuilder builder, string descriptor, PipelinePhase phase, Action<ResolveRequestContext, Action<ResolveRequestContext>> callback)
{
    builder.RegisterServiceMiddleware<TService>(descriptor, phase, MiddlewareInsertionMode.EndOfPhase, callback);
}

[tillig]

I get that, but we have a couple of problems:

1. This sat since April waiting for feedback, and while I'm not one to hurry folks, it does seem like eight months is plenty of time to wait for feedback.
2. There aren't a ton of people working on Autofac. It's basically one and a half people chipping in free time of late. We need to rely on the community to help fill in coverage/testing gaps. That might mean if you add something new, you also need to add tests for it; and if there weren't tests before, maybe that's something we need help filling out. If it's going to wait for one of us to back-fill stuff, you will likely be waiting a long time.

So, no, I can't really commit to going in here and adding coverage for existing stuff. I'm sorry, I don't mean that to be harsh, but I have to set expectations. I'm not going to get around to it. There are already a few other PRs I'm trying to get through on other integrations and given the slow feedback here it's not something I can sink time into. If you want these extensions, submit a new PR with the coverage satisfied. Again, sorry, there just aren't enough people to help do it for you.