EventStore / EventStore-Client-Dotnet

Dotnet Client SDK for the Event Store gRPC Client API written in C#
Other
145 stars 38 forks source link

Breaking change "SubscribeToAllAsync is no longer supported. Use SubscribeToAll instead" #290

Closed stefer closed 7 months ago

stefer commented 7 months ago

Describe the bug

Breaking change introduced in Grpc.Streams 23.2.0: "SubscribeToAllAsync is no longer supported. Use SubscribeToAll instead"

This require a major rewrite of our code as we hook into Azure WebJob triggers to handle the events.

It's a bit harsh to introduce a breaking change like this without a grace period as it would be with a Obsolete message as warning. Why is the obsolete message set to Error=true?

            [Obsolete("SubscribeToAllAsync is no longer supported. Use SubscribeToAll instead.", true)]
        public Task<StreamSubscription> SubscribeToAllAsync(

While I am here, I wonder what exceptions we will get if the subscription is dropped when using the new SubscribeToAll method. The example code on https://developers.eventstore.com/clients/grpc/subscriptions.html#handling-subscription-drops handles the drop like this:

} catch (Exception ex) {
    Console.WriteLine($"Subscription was dropped: {ex}");
    goto Subscribe;
}

But an untyped exception could come from anywhere, so how do we know that it is an actual drop of the subscription?

To Reproduce Steps to reproduce the behavior:

  1. Compile code that worked using Grpc.Streams 23.1.0.

Expected behavior It should still be possible to compile the code successfully, with a deprecation warning that SubscribeToAllAsync is obsolete.

Actual behavior Compile error (not warning) "'SubscribeToAllAsync is no longer supported. Use SubscribeToAll instead.'"

Severity    Code    Description Project File    Line    Suppression State
Error   CS0619  'EventStoreClient.SubscribeToAllAsync(FromAll, Func<StreamSubscription, ResolvedEvent, CancellationToken, Task>, bool, Action<StreamSubscription, SubscriptionDroppedReason, Exception?>?, SubscriptionFilterOptions?, UserCredentials?, CancellationToken)' is obsolete: 'SubscribeToAllAsync is no longer supported. Use SubscribeToAll instead.' Collector.CreditEvaluation.Infrastructure   C:\Development\CollectorCreditEvaluation\src\Collector.CreditEvaluation.Infrastructure\Messaging\EventStore\GrpcEventStoreSubscriber.cs 58  Active

Config/Logs/Screenshots

EventStore details

Additional context

We are using dependabot to update packages and it created a PR for bumping EventStore.Client.Grpc.Streams from 23.1.0 to 23.2.0.

stefer commented 7 months ago

As a temporary work-around, we have marked our own functions calling SubscribeToAllAsync as [Obsolete("", false)] It seems as obsoleted method are allowed to call other obsoleted methods.

w1am commented 7 months ago

@stefer Appreciate you bringing this to our attention. It was indeed an oversight. We'll make sure to correct it in the upcoming patch release shortly

w1am commented 7 months ago

I wonder what exceptions we will get if the subscription is dropped when using the new SubscribeToAll method

The subscription could have been dropped for all kinds of reasons, so it's up to the user to handle it.

stefer commented 7 months ago

I wonder what exceptions we will get if the subscription is dropped when using the new SubscribeToAll method

The subscription could have been dropped for all kinds of reasons, so it's up to the user to handle it.

But the exception could also be produced by any kind of error in HandleEvent, so handling it like in the example would be misleading. But you are saying that there are no wrapped exceptions for dropped subscription from EventStore?

w1am commented 7 months ago

That's correct. It has been like this the entire time for regular subscriptions. The code is now much cleaner :)