search

nats-io / nats.net

Full Async C# / .NET client for NATS
https://nats-io.github.io/nats.net
Apache License 2.0
239 stars 49 forks source link

Add validation for stream name when contains illegal characters #357

Closed StKolev closed 7 months ago

StKolev commented 7 months ago

Proposed change

Based on jetStream Docs, Name cannot contain whitespace, ., *, >, path separators (forward or backwards slash), and non-printable characters. I think it will be easy to add such validation with more descriptive message. Thanks

Use case

When I'm creating a stream with name containing invalid characters like. I'm receiving the following stack trace which does not give much information about the problem. NATS.Client.JetStream.NatsJSApiNoResponseException: No API response received from the server at NATS.Client.JetStream.NatsJSContext.JSRequestAsync[TRequest,TResponse](String subject, TRequest request, CancellationToken cancellationToken) at NATS.Client.JetStream.NatsJSContext.JSRequestAsync[TRequest,TResponse](String subject, TRequest request, CancellationToken cancellationToken) at NATS.Client.JetStream.NatsJSContext.JSRequestResponseAsync[TRequest,TResponse](String subject, TRequest request, CancellationToken cancellationToken) at NATS.Client.JetStream.NatsJSContext.CreateStreamAsync(StreamConfig config, CancellationToken cancellationToken)

Contribution

yes I can try :)

mtmk commented 7 months ago

JetStream models were generated from schemas and we used to have a validation routine running against the model annotations. Unfortunately we had to remove that and rely on server validating the model because model validation did not work with AOT deployments: https://github.com/nats-io/nats.net.v2/blob/1f7010c7422e049faea6417031a9ea29eb7e9e54/src/NATS.Client.JetStream/NatsJSContext.cs#L198-L199 Not sure if that's the way to go but we can certainly do with validating request and we should look into this. Your assistance would be greatly appreciated 💯

niklasfp commented 7 months ago

Would it not be "enough" to at least do check for invalid stream names here: https://github.com/nats-io/nats.net.v2/blob/667b77ef83da72514240352525bb445270bf9813/src/NATS.Client.JetStream/NatsJSContext.Streams.cs#L20

Or maybe do that the validation on all stream names in that file ?

mtmk commented 7 months ago

@niklasfp I think you're right. we can just validate in CreateStreamAsync() method.

niklasfp commented 7 months ago

@mtmk I can give it a shot, should we consider validating the stream names in all public methods in jscontext where stream name is passed as a parameter?

I already have some code locally for my own usage (uses IndexOfAny and, when in.net 8 combines that with searchvalues) its kind of speedy, they improved it with simd.