rebus-org / Rebus

:bus: Simple and lean service bus implementation for .NET
https://mookid.dk/category/rebus
Other
2.26k stars 353 forks source link

Introduces an IExceptionInfoFactory to customize the ExceptionInfo class [Refs #1122] #1124

Closed ladenedge closed 6 months ago

ladenedge commented 6 months ago

This PR introduces IExceptionInfoFactory to customize how ExceptionInfo classes are created from exceptions. The main points are:

  1. Classes converting exceptions into ExceptionInfos now have a dependency on IExceptionInfoFactory.
  2. The default exception info factory is just ExceptionInfo.FromException(), so out-of-the-box behavior should not change.
  3. We also provide an InMemExceptionInfo (and corresponding factory) such that interested users can revert to an ExceptionInfo that contains the raw exception. The new factory would have to be registered at bus configuration time to override the default.
  4. Custom info users may then make use of ExceptionInfo.ConvertTo<TInfo>() to get their custom info. (This is probably the kludgiest part of this approach. ☹️)

For the moment, this is just an idea to review. If you like it, I will of course add full unit test coverage, and perhaps some configuration helpers. If you don't like the idea, either whole or in part, please let me know how things (even tiny things) might be changed for the better. I'll keep an eye on #1122 for discussion.

Thanks for your time!


Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

CLAassistant commented 6 months ago

CLA assistant check
All committers have signed the CLA.

mookid8000 commented 6 months ago

This is IMO a rather elegant solution to your problem, and you have coded it like I would have 🤩 If you add a test or two, it would be even better, of course 😁 I'll merge it ASAP when you tell me it's ready

ladenedge commented 6 months ago

Thanks for the encouragement. I'll try to polish this up early next week.

ladenedge commented 6 months ago

These commits do a couple things:

870b1dd : Full unit test coverage (I think) for the new ExceptionInfo-related classes, including the original one. I tried not to be too smart -- for instance, they only check that Details is non-empty, not that it has to be from ToString(). I also added a configuration helper off of Errors(...) that looks like this IRL:

Configure.With(activator)
    .Errors(e => e.UseInMemExceptionInfos())

ad10b67 : From what I can tell, the unit tests require a good number of manually-created test classes (fakes, I guess) to test some of these systems with many dependencies. On the chance you haven't seen these glue libraries AutoFixture.NUnit3 and AutoFixture.AutoMoq -- or, more likely, just haven't had the time to integrate them -- this commit demonstrates how a class with lots of DI, DefaultRetryStep, might be unit tested pretty easily with these tools. You can see some of these tests are done with only a few lines and no manually-written fakes because we rely on AutoFixture/Moq to simply give us a sensible anonymous DefaultRetryStep that we can play with.

Even tests that do require enumerating the dependencies, such as CreatesInfoForSecondLevelRetryException, are still pretty straightforward. Unit tests like these can also encourage good IoC because it's easier to test!

If you like this strategy, perhaps it can be integrated more as time goes on. Without it, I feel like DefaultRetryStep is much less fun to test. 🙁 However, I realize it does add some new dependencies which you seem to have worked hard to avoid, so if you don't like it I'm happy to revert this commit, no harm done. Lemme know what you think!

mookid8000 commented 6 months ago

I don't have a problem with trying out some AutoFixture stuff in the test project – either it'll grow roots and be used in other places as well, or it'll be torn out some time in the future 😉

Thanks for taking the time to contribute with this – it will be out as Rebus 8.1.0 on NuGet.org in a few minutes if the tests are 🟢