Add flexibility in subject matching for nats_jetstream

[evankanderson]

I was attempting to migrate some existing code which uses longer topic names like ".." to NATS Jetstream. Unfortunately, the current code does not work for this stype of topic/subject, because:

1. Stream names cannot contain ..
2. The .* format matches a single .-delimited segment.

Added some logic to support both the .> multi-segment matcher, and using an empty string along with Consumer-side filtering.

[embano1]

Is this a fundamental limitation in NATS or just with the JetStream version we're using? If the latter, @stephen-totty-hpe is working on updating the package to the latest NATS JS SDK.

[evankanderson]

I'm not sure what you mean -- the NATS semantics are that foo.* matches foo.123 but not foo.clients.123, while foo.> matches both.

The stream naming limitations are part of core NATS: https://docs.nats.io/running-a-nats-service/nats_admin/jetstream_admin/naming

The subject matching rules are here: https://docs.nats.io/nats-concepts/subjects

[evankanderson]

(our subject names look like internal.minder.subsystem.event in some cases. We're likely to change that naming at some point, but probably to something like internal.subsystem.type.<project-id>, so this patch would still be useful then.)

[embano1]

The stream naming limitations are part of core NATS

That's what I wanted to understand, thx

[embano1]

Question (minor): is there a case where both conditions could be true? If not, this code could be simplified by removing the else and just having two if statements to increase readability without if/else indentation?

[embano1]

Also, once #1083 lands (depending on the implementation by @stephen-totty-hpe), we need to make sure to also apply this logic to the new JetStream implementation.

[evankanderson]

This code could also be written as:

if strings.HasPrefix(subject, stream) {
  if strings.Count(strings.TrimPrefix(subject, stream), ".") > 1 {
    // Change .* to .>
  }
} else {
  // Change to ""
}

I think if stream is foo and "subject" is bar.baz.12345, then the reformulated code would hit the second condition.

We could rewrite this as:

if strings.HasPrefix(subject, stream) && strings.Count(strings.TrimPrefix(subject, stream), ".") > 1 {
  // Change .* to .>
}
if !strings.HasPrefix(subject, stream) {
  subjectMatch = ""
}

But, we need the `HasPrefix` check on the `strings.Count`.  I could switch to using `TrimPrefix` (or some other tricks, like getting the prefix-trimmed string, and comparing its length to the original to determine `HasPrefix`), but none of the above seemed better (in particular, `TrimPrefix` in the non-prefixed case returns the original string, which seemed wrong.

Thinking further, I suppose I could remove the `HasPrefix` from the first clause, as the second will completely overwrite if `HasPrefix` is false.

[evankanderson]

Note - if we didn't have the current implementation that someone may already be relying on, I'd have simply changed the .* to a .>`, but I didn't want to break any existing users.

[embano1]

Nice and clean! Thx!

Yup, keeping existing by default is something I would have called out anyways 😀

Wanna squash the commits bc we don't have a fancy bot doing this for us ,)

[evankanderson]

Squashed -- it wasn't clear if you were using the "squash merge" option here.

[embano1]

Thx, we don't because Github doesn't support proper commit messages when using squash merge. IIRC, it should be doc-ed in our CONTRIBUTING guide. But all good :)

[embano1]

Thank you!