Add support for Consumer Create Action #497
Conversation
|
Fixed problem with serialize .net8 and test on create publisher action |
| #else | ||
| [System.Text.Json.Serialization.JsonConverter(typeof(NatsJSJsonStringEnumConverter<ConsumerCreateRequestAction>))] | ||
| #endif | ||
| public ConsumerCreateRequestAction Action { get; set; } |
There was a problem hiding this comment.
Could we make this optional so that we don't have to deal with net8 serialization exception for an empty value?
There was a problem hiding this comment.
Yes, that was one of the ideas. In this case, the property will sample be ignored and NatsJSJsonStringEnumConverter will be less strict
| Create = 0, | ||
| Update = 1, | ||
| [EnumMember(Value = "")] | ||
| CreateOrUpdate = 2, |
There was a problem hiding this comment.
we can remove this option if action is optional.
| } | ||
| } | ||
| #else | ||
| internal class NatsJSJsonStringEnumConverter<TEnum> : JsonConverter<TEnum> |
There was a problem hiding this comment.
is this for net8? I guess we can leave it as camelCase and not need this class?
| @@ -33,31 +33,25 @@ public partial class NatsJSContext : INatsJSContext | |||
| CancellationToken cancellationToken = default) | |||
| { | |||
| ThrowIfInvalidStreamName(stream); | |||
| return await CreateOrUpdateConsumerInternalAsync(stream, config, ConsumerCreateRequestAction.CreateOrUpdate, cancellationToken); | |||
There was a problem hiding this comment.
we could make action nullable here.
|
|
||
| namespace NATS.Client.JetStream.Models; | ||
|
|
||
| public enum ConsumerCreateRequestAction |
There was a problem hiding this comment.
what do you think of ConsumerCreateAction as the name? more succinct?
There was a problem hiding this comment.
Yes, think it's good too. Renamed
… CreateOrUpdateAction
ConsumerCreateAction enum has been updated to include a default value. Previously nullable instances of this enum have been updated to leverage the default value instead. The necessary adjustments have been made in the related JSON serializer, consumer creation methods, and tests to reflect this change. When using nullable enum to accomodate for the absence of 'action' property in JSON, we are forced to remove the type constraints on the serializer class which is not ideal and leaves us to deal with null values. Instead, having the default value of the enum as the first element lets us using 'default' keyword instead of null in calls, making it less confusing.
|
@Ivandemidov00 thanks for the updates. Just made a change to enum default and removed the nullable marking. I think it's easier to reason with this way. |
There was a problem hiding this comment.
LGTM thanks @Ivandemidov00
(will wait for another review because of my changes)
|
|
||
| public enum ConsumerCreateAction | ||
| { | ||
| Default = 0, |
There was a problem hiding this comment.
Would it make sense to make the default descriptive? I.e. CreateOrUpdate = 0 instead of Default = 0?
Or is it best to leave it vague as the NATS Server can decide the default?
* Change ConsumerConfig AckPolicy default to Explicit (#490) * Add support for Consumer Create Action (#497) * Add `IsSuccess` extension method for PubAckResponse (#493) * Watch Multiple KV Filters (#485) * FIX : PurgeDeletesAsync hangs if KV Bucket is already empty (#489) * Fix NUID sequence (#488)
* Release 2.2.2 * Change ConsumerConfig AckPolicy default to Explicit (#490) * Add support for Consumer Create Action (#497) * Add `IsSuccess` extension method for PubAckResponse (#493) * Watch Multiple KV Filters (#485) * FIX : PurgeDeletesAsync hangs if KV Bucket is already empty (#489) * Fix NUID sequence (#488) * Fixed test
Resolves #475