The Refactor That Stopped Halfway

Unfinished refactors do not look like unfinished refactors. They look like working software with a few extra classes in it, because the half that was migrated works and the half that was not still works the way it always did. The only evidence is a registration nobody resolves and a handler nobody reaches. Part 3 showed a bug hidden by exactly one such unreachable handler. This part reads the commit that made it unreachable.

What the last commit actually did

Trill's Stories repository has three commits on master: 5b23614 init, 29557cb decorators, and 4d96ab8 net5.0 update. The last one is dated 2021-04-22 and the message is a lie of omission — it touched forty-five files and is mostly a domain refactor. The additions tell the story:

A  src/…Core/Events/IDomainEvent.cs
A  src/…Core/Events/IDomainEventDispatcher.cs
A  src/…Core/Events/IDomainEventHandler.cs
A  src/…Core/Events/StoryCreated.cs
A  src/…Core/Events/StoryRatingChanged.cs
A  src/…Core/Services/IStoryRatingService.cs
A  src/…Core/Services/StoryRatingService.cs
A  src/…Infrastructure/Kernel/DomainEventDispatcher.cs
A  src/…Application/Services/EventMapper.cs
A  src/…Application/Events/Domain/Handlers/StoryCreatedHandler.cs
A  src/…Application/Events/Domain/Handlers/StoryRatingChangedHandler.cs
M  src/…Application/Commands/Handlers/SendStoryHandler.cs

That is a complete in-process domain-event pipeline: an event marker, a dispatcher contract, a handler contract, two concrete events, a dispatcher implementation, a domain-to-integration event mapper, and two handlers. It also rewrote SendStoryHandler to use all of it. AggregateRoot<T> gained an _events list and an AddEvent method in the same commit.

There is one file that the commit does not touch, and it is the other command handler in the service: RateStoryHandler.cs.

The two halves

SendStoryHandler is the migrated half. Its tail is textbook:

var story = Story.Create(storyId, author, command.Title, text, command.Tags, now, visibility);
await _storyRepository.AddAsync(story);
var domainEvents = story.Events.ToArray();
await _domainEventDispatcher.DispatchAsync(domainEvents);
var integrationEvents = _eventMapper.Map(domainEvents).ToArray();
_storyRequestStorage.SetStoryId(command.Id, story.Id);
await _messageBroker.PublishAsync(integrationEvents);

Create through the static factory, collect the events the aggregate raised, dispatch them in-process, map them to integration events, publish to the bus. Story.Create is the only place StoryCreated is raised, and it raises it.

RateStoryHandler is the unmigrated half. Its tail, lines 46 to 50:

await _storyRatingRepository.SetAsync(new StoryRating(new StoryRatingId(command.StoryId, command.UserId),
    command.Rate));
var totalRating = await _storyRatingRepository.GetTotalRatingAsync(story.Id);
await _messageBroker.PublishAsync(new StoryRated(command.StoryId, command.UserId,
    command.Rate, totalRating));

It constructs StoryRating with new. The aggregate does have a factory method, added by the same commit:

public static StoryRating Create(StoryId storyId, UserId userId, int rate, int totalRate)
{
    var rating = new StoryRating(new StoryRatingId(storyId, userId), new Rate(rate));
    rating.AddEvent(new StoryRatingChanged(rating, totalRate));

    return rating;
}

StoryRating.Create is the only place StoryRatingChanged is ever raised. RateStoryHandler does not call it. Nothing calls it.

The chain of consequences

Follow it link by link, because each one is a grep away and every one holds.

StoryRating.Create is never called, so StoryRatingChanged is never added to an aggregate's event list.

StoryRatingChangedHandler is therefore unreachable. Its body credits the author's reputation:

public async Task HandleAsync(StoryRatingChanged domainEvent)
{
    var story = await _storyRepository.GetAsync(domainEvent.Rating.Id.StoryId);
    var user = await _userRepository.GetAsync(story.Author.Id);
    user.AddRating(domainEvent.Rating.Rate);
    await _userRepository.UpdateAsync(user);
}

So User.Rating in the Stories service is permanently zero. Nothing else in the service calls AddRating, and the UserDocument created by the UserCreated external-event handler starts at zero.

Which means the authorship gate can never fire. Core/Policies/StoryAuthorPolicy.cs is one line:

public bool CanCreate(User user) => !user.Locked && user.Rating >= -10;

SendStoryHandler calls _storyAuthorPolicy.CanCreate(user) and throws CannotCreateStoryException when it returns false. Half of that predicate is live — Locked is maintained by the UserLocked/UserUnlocked handlers. The other half is dead: Rating is always 0, and 0 is always >= -10. The reputation floor, the only piece of moderation logic in the entire Stories service, is unreachable by construction.

And IStoryRatingService, the service class the commit added to hold this logic properly, appears in exactly one place outside its own two files:

.AddScoped<IStoryRatingService, StoryRatingService>()

A registration and no injection. Its RateAsync method does the correct thing — checks user.Locked, reads the current total, adds the new rate, calls StoryRating.Create. It is the intended body of RateStoryHandler, sitting in Core/Services, waiting.

The version counter that came with it

The same commit modified AggregateRoot<T> to support all this, and the machinery it added is worth reading because it is dead in a second, quieter way:

private bool _versionIncremented;

protected void AddEvent(IDomainEvent @event)
{
    if (!_events.Any() && !_versionIncremented)
    {
        Version++;
        _versionIncremented = true;
    }

    _events.Add(@event);
}

protected void IncrementVersion()
{
    if (_versionIncremented)
    {
        return;
    }

    Version++;
    _versionIncremented = true;
}

A temporary field guarding a once-per-aggregate increment — the classic OO-abuser smell, named almost literally. And Story.Create interacts with it in a way that is correct only by accident:

var story = new Story(id, author, title, text, tags, createdAt, visibility) {Version = 0};
story.AddEvent(new StoryCreated(story));

The constructor calls SetTags, which calls IncrementVersion(), leaving Version = 1 and _versionIncremented = true. The object initializer runs after the constructor, and stomps Version back to 0 — reachable because Version's setter is protected and this is a static method on the same class. AddEvent then sees the flag already set and does not re-increment. The result is Version = 0 on a freshly created aggregate, which is what you want, produced entirely by C# evaluation order. Swap the two statements and the semantics change silently.

None of which matters, because Version is never persisted. StoryDocument has no version field, IStoryRepository has no UpdateAsync, and nothing anywhere reads Version back. An optimistic-concurrency apparatus was built, guarded, and wired to a column that does not exist. It is the same failure as the rating pipeline in miniature: the interesting half was written and the half that connects it to reality was not.

Three more things the pipeline does that it should not

Since we are inside the machinery, the dispatcher is worth reading. Infrastructure/Kernel/DomainEventDispatcher.cs, the whole dispatch loop:

using var scope = _serviceProvider.CreateScope();
foreach (var @event in events)
{
    var handlerType = typeof(IDomainEventHandler<>).MakeGenericType(@event.GetType());
    var handlers = scope.ServiceProvider.GetServices(handlerType);

    var tasks = handlers.Select(x => (Task) handlerType
        .GetMethod(nameof(IDomainEventHandler<IDomainEvent>.HandleAsync))
        ?.Invoke(x, new[] {@event}));

    await Task.WhenAll(tasks);
}

• It is a singleton holding the root IServiceProvider and creating its own scope. So a domain-event handler resolves its repositories in a different DI scope from the command handler that raised the event. There is no shared unit of work; StoryRatingChangedHandler would read the user through a second Mongo context and write it back independently of whatever the command handler did. That is the difference between “the rating and the reputation change together” and “they change near each other”.
• MakeGenericType plus GetMethod plus Invoke, uncached, per event, per dispatch. Reflection in the request path, where a cached Func<object, IDomainEvent, Task> per handler type costs one dictionary lookup.
• ?.Invoke produces a null element if GetMethod ever returns null, and Task.WhenAll will dereference it. The null-conditional operator here converts a clear NullReferenceException at the call site into a less clear one inside the framework.

Two smaller tells complete the picture. StoryCreatedHandler, added by the same commit, is this in full:

public async Task HandleAsync(StoryCreated domainEvent)
{
    await Task.CompletedTask;
}

It injects IUserRepository and never uses it — a placeholder that was going to do something. And EventMapper.Map falls back to _ => null for any unmapped domain event, which would put a null into the array handed to IMessageBroker.PublishAsync. That only survives because the broker has if (@event is null) continue; in its publish loop. Two files, one silent swallow, spread across an assembly boundary.

The service next door never got the commit at all

For contrast, the Users repository's net5.0 update commit touched six files, every one of them a .csproj or an appsettings.json. Users never received the refactor. And it shows in a single file: Domain/Entities/IDomainEvent.cs exists, is public, and nothing in the repository implements it. No dispatcher, no handler interface, no event mapper, no concrete event. One interface, zero implementations, kept alive by nothing but the compiler's indifference.

That is the same shape as IStoryRatingService and the same shape as StoryRatingChangedHandler — an abstraction that documents an intention rather than a behaviour. Across the two services, the estate accumulates a consistent list of these: IUsersApiClient and its HTTP implementation, registered AddScoped and injected nowhere; VisibilityDocument, referenced by nothing and shaped so Mongo could not deserialise it if it were; ContractAttribute, applied to no type in either service while UsePublicContracts<ContractAttribute>() is called in both. Roughly four hundred lines of registered-but-unreachable abstraction between the two repositories.

The pattern to notice is that every one of them is discoverable with the same grep: find the type, then count the constructor parameters that ask for it. When that count is zero and the registration count is one, the abstraction is a comment with a build step.

The lesson, and the fair reading

To be fair to the estate: this is a teaching repository, the pipeline it added is a good one, and SendStoryHandler demonstrates the full pattern correctly end to end. A reader learning domain events from this codebase learns the right shape. The commit is 90% of a genuinely useful refactor.

But the 10% is instructive in a way the 90% is not. A half-migrated pattern is worse than no pattern, because the abstraction's presence is read as a guarantee that it is in use. Anyone auditing this service for “do we raise domain events?” finds IDomainEventDispatcher, StoryRatingChanged, and a handler that credits reputation, and concludes yes. The grep that says otherwise is three greps deep.

Two cheap defences would have caught it. A composition-root test that resolves every registered service and fails on any interface with no consumer would have flagged IStoryRatingService immediately. And code coverage over a single integration run would have shown StoryRating.Create and StoryRatingChangedHandler at zero — which, for code added in the current commit, is a question worth asking out loud.

Next, the one piece of cross-cutting machinery in this service that is fully wired, and the single attribute that keeps it from registering itself: six decorators and one marker attribute.