Add a subject type #222

MattesWhite · 2021-09-17T09:02:14Z

This PR adds a Subject type to the NATS client. It also includes some convenience stuff like a mutable, owned version SubjectBuf, Tokens as an own type, the possibility to iterate a subjects tokens and a matching algorithm.

For the validation algorithm I tried to stick to the docs from nats.io.

The subject type is for now nowhere integrated, i.e. it would only be useful for users. However, piece by piece substituting str with Subject(Buf) should proof beneficial.

This could also be used to solve #221 .

derekcollison

Will take a look, most clients do not vigorously test subjects since the server protects itself etc.

MattesWhite · 2021-10-06T14:46:21Z

Yes, in my application I don't validate subjects that much either. But frequently use the Subjects to parse messages I receive from subscription with wildcards to check out where it actually came from and subject.tokens().nth(4) is just way simpler than writting some custum parser every time.

caspervonb · 2022-01-06T03:23:44Z

Interesting but I'm unsure.
Any thoughts on this addition @Jarema?

derekcollison · 2022-01-06T03:47:15Z

I think the ability to pull tokens and such is a good addition, but would not want to force changes to normal publish and subscribe.

Jarema

Overall, I think this is nice to have such an API.

Though I'm wondering - how it would look without incorporating it into publish/subscribe?

Without that, it looks a little like we couldn't decide if we want subject validation or not, so we have a compromise that might raise eyebrows why it's there, but not part of pub/sub.

Incorporating it into publish and subscribe doesn't sound bad, especially if it implements nicely some kind of Into trait that still allows passing &str or String. Also, the performance culprit of such an addition should be marginal.

An alternative (but still a compromise) would be to have it exposed under the subject module and treat it as a utility.

Jarema · 2022-01-06T10:26:14Z

src/subject.rs

+    #[test_case("abc.def", &["*", "fed"], "abc.def.*.fed"        ; "single wildcard and more")]
+    #[test_case("abc", &[">"], "abc.>"                           ; "multi wildcard")]
+    #[test_case("abc", &[">", "cba"], "" => panics               ; "multi wildcard and more")]
+    fn join_subject(base: &str, appends: &[&str], expect: &str) {


This can be useful for internals.
I like it.
It's pretty easy to mess up final subject while concating tokens of the subject (or specifically - prefixes with user provided subjects, JS api prefix with KV prefix etc).

Having correctness check of the subject ensured by some tested function has a value.

Jarema · 2022-01-06T10:30:04Z

src/subject.rs

+    #[test_case("probe" => true    ; "valid name")]
+    #[test_case("pröbe" => false   ; "non alphanumeric")]
+    #[test_case("PrObE007" => true ; "wild stuff")]
+    fn validate_token(token: &str) -> bool {


This part is nice and can help both users and contributors, but is IMHO too strict.

Yes, NATS docs specifies that subject tokens should contain only ASCII alphanumeric chars, but that is said only to ensure as much compatibility across NATS Clients as possible. NATS Server actually accepts any UTF-8 charts as part of a subject token as long as they're not . and space, as those are part of NATS proto.

I found myself (as a user) useful to be able to use non alphanumeric characters and it would be shame to artifically remove such option from the client.

After using NATS more I recognized this as well. $SYS subjects wouldn't be valid as well. So I'll relax the constraints

So everything is a token as long as !( token.is_empty() || token.chars().any(|c| c == '.' || c == ' ') ), right?

Jarema · 2022-01-06T10:41:06Z

src/subject.rs

+    #[test_case("cba", ">" => true    ; "multi wildcard reverse")]
+    #[test_case("*", ">" => true      ; "mixed wildcards")]
+    #[test_case("cba", "abc" => false ; "unequal tokens")]
+    fn match_tokens(l: &str, r: &str) -> bool {


I'm not sure about that.
What are the use cases of that?

@MattesWhite was that added for testing capabilities?

If you subscribe - you will get what you asked for and matching won't help.
Checking if you will get what you asked for before subbing looks IMHO a little too defensive.
For publishing - matching beforehand sounds almost like coupling with consumers of the subject.
But maybe I'm too picky here :).

This originats from the current implementation of the async client. For each subscription a thread is spawn, epsecially when you have lots of request handling this produces a lot of overhead.
As a workaround for our PoC we have one > subscription and distribute the messages to the async parts via channels. This is not good and will probably become a bottleneck but for now works ok. It would be okay for me to remove this once the async client is no longer depending on the blocking crate.

MattesWhite · 2022-01-06T12:32:18Z

Yes, an integration of the Subject API into the crate was my long time goal. However, I was not sure if the work would be accepted so I waited for a positive response. When I'm done with my other PR I'll continue here.

MattesWhite · 2022-01-09T12:47:53Z

As predicted, incorporating Subject and SubjectBuf into the whole crate is a lot of work. At the moment I'm not done but the work in the crate itself is mostly done.

I reworked Subject and Token into dynamically sized types, i.e. they behave like str or Path now. Sadly, this includes a little bit of unsafe. I'd like to have your feedback on this matter.

ToDo:

Fix benches, examples and tests
Reevaluate the uses of Subject::new_unchecked() and SubjectBuf::new_unchecked()
Write proper documentation for Subject and SubjectBuf
DISCUSS: Public API could take impl TryInto<&Subject> or a bound to another converter trait. This would probably lessen the migration effort for users.

SubjectBuf implements Deref

derekcollison · 2022-01-15T17:52:35Z

FYI we need two types. One for publish which can not have wildcards (must be literal) and one for subscriptions which can include wildcards.

MattesWhite · 2022-01-19T20:10:02Z

The current implementation has a contains_wildcards() -> bool isn't that enough?

derekcollison · 2022-01-20T01:01:58Z

Possibly, depends on the contract and when they are valid or not and whether or not we want that correctness enforced in a zero abstractions scenario.

caspervonb · 2022-01-25T12:09:43Z

Discussed this a little bit offline last week with @Jarema.
I think a type for this can be useful, in that we provide a guarantee that once constructed the type is valid but this feels a bit heavy handed with both Subject and SubjectBuf's.

I feel like the sweet spot could be to have a Subject type, that is a validated type. Then we can implement AsSubject/Into traits for conversions from &str, String, etc. Basically somewhere in-between this PR and #295

I think I might take a quick pass at this soon, see if my vague idea is actually any better in practice 🙂

MattesWhite · 2022-03-07T07:40:43Z

So after long time I have again some time to work on this. So far I got everything to compile again. Next I'll marge main to catch up with recent development (good work by the way +1). Then I'll fix tests to work again. And after all this is done we can start reviewing this giant of a PR...

It didn't add enough benefits

…t-type

MattesWhite

Self review

MattesWhite · 2022-04-05T14:28:59Z

@caspervonb
Finally, after months of delay this PR is ready for review, CI is passing, and Subject is introduced to the whole crate.

I slimed the added features a bit down:

Token as an own type is gone
Subject::sub_subject() is removed

The AsSubject trait was introduced to allow the use of &str in most places of the public API so the doc tests are mostly left unchanged, and the change for users should be minimal.

Jarema · 2022-04-28T10:43:17Z

@MattesWhite hey! We will look into this as soon as we will tackle this topic in async-nats client to have it in both, consistent.

MattesWhite · 2023-08-30T07:16:34Z

Closing this PR as it is way outdated and superseded by #596 and #952

Add a subject type

54ef7ca

derekcollison requested a review from spacejam September 17, 2021 18:15

mfarnbauer-schmidt added 3 commits September 20, 2021 15:56

Implement FromStr were appropriate

3406fc9

Add contains_wildcard()

7d9ae28

Allow '-' and '_' in subjects

f484b14

derekcollison reviewed Oct 6, 2021

View reviewed changes

mfarnbauer-schmidt added 2 commits October 8, 2021 08:30

Fix incorrect matching of subjects

7f32cfb

Improve match algorithm

094a694

caspervonb self-assigned this Nov 8, 2021

Jarema reviewed Jan 6, 2022

View reviewed changes

MattesWhite added 3 commits January 6, 2022 15:52

Merge branch 'main' of github.com:MattesWhite/nats.rs into subject-type

86c48ce

Relax token validation

f1c56d8

Add subject indexing

fc89e3b

derekcollison mentioned this pull request Jan 8, 2022

Subject trait #295

Closed

Introduce Subject/-Buf to the crate

35a7df9

MattesWhite changed the title ~~Add a subject type~~ [WIP] Add a subject type Jan 9, 2022

Remove redundant methods

6f68d39

SubjectBuf implements Deref

MattesWhite and others added 2 commits March 7, 2022 07:23

Integrate Subject into rest of crate

70e82cc

Finish inital integration of Subject

d9a5d9b

Optimize Subject validation

8df6741

mfarnbauer-schmidt added 12 commits March 22, 2022 08:11

Add bench for subject validation

ecd09a3

Remove Token type

f1ab632

It didn't add enough benefits

Apply rustfmt

d68af07

Fix doc tests

a95da53

Merge branch 'main' of https://github.com/nats-io/nats.rs into subjec…

2c45bf9

…t-type

Introduce Subject to new code after merge

fd27940

Apply clippy and fmt

4ea7c8b

Fix linter errors

eba48ab

Remove IDE specific settings

867ce85

Remove unused function

d73f415

Apply all clippy lints

ea0fa3c

Fix doc test (again)

089d6a9

MattesWhite commented Apr 5, 2022

View reviewed changes

mfarnbauer-schmidt added 2 commits April 5, 2022 16:06

Apply findings from own review

348a491

cargo fmt

6ce5b19

MattesWhite changed the title ~~[WIP] Add a subject type~~ Add a subject type Apr 6, 2022

MattesWhite mentioned this pull request Apr 26, 2022

add queue subscriber and unit test #388

Merged

MattesWhite mentioned this pull request Aug 5, 2022

Introduce a Subject type to async-nats #596

Closed

MattesWhite closed this Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a subject type #222

Add a subject type #222

MattesWhite commented Sep 17, 2021

derekcollison left a comment

MattesWhite commented Oct 6, 2021

caspervonb commented Jan 6, 2022

derekcollison commented Jan 6, 2022

Jarema left a comment

Jarema Jan 6, 2022

Jarema Jan 6, 2022

MattesWhite Jan 6, 2022

MattesWhite Jan 6, 2022

Jarema Jan 6, 2022

MattesWhite Jan 6, 2022

MattesWhite commented Jan 6, 2022

MattesWhite commented Jan 9, 2022 •

edited

Loading

derekcollison commented Jan 15, 2022

MattesWhite commented Jan 19, 2022

derekcollison commented Jan 20, 2022

caspervonb commented Jan 25, 2022 •

edited

Loading

MattesWhite commented Mar 7, 2022

MattesWhite left a comment

MattesWhite commented Apr 5, 2022

Jarema commented Apr 28, 2022

MattesWhite commented Aug 30, 2023

Add a subject type #222

Add a subject type #222

Conversation

MattesWhite commented Sep 17, 2021

derekcollison left a comment

Choose a reason for hiding this comment

MattesWhite commented Oct 6, 2021

caspervonb commented Jan 6, 2022

derekcollison commented Jan 6, 2022

Jarema left a comment

Choose a reason for hiding this comment

Jarema Jan 6, 2022

Choose a reason for hiding this comment

Jarema Jan 6, 2022

Choose a reason for hiding this comment

MattesWhite Jan 6, 2022

Choose a reason for hiding this comment

MattesWhite Jan 6, 2022

Choose a reason for hiding this comment

Jarema Jan 6, 2022

Choose a reason for hiding this comment

MattesWhite Jan 6, 2022

Choose a reason for hiding this comment

MattesWhite commented Jan 6, 2022

MattesWhite commented Jan 9, 2022 • edited Loading

derekcollison commented Jan 15, 2022

MattesWhite commented Jan 19, 2022

derekcollison commented Jan 20, 2022

caspervonb commented Jan 25, 2022 • edited Loading

MattesWhite commented Mar 7, 2022

MattesWhite left a comment

Choose a reason for hiding this comment

MattesWhite commented Apr 5, 2022

Jarema commented Apr 28, 2022

MattesWhite commented Aug 30, 2023

MattesWhite commented Jan 9, 2022 •

edited

Loading

caspervonb commented Jan 25, 2022 •

edited

Loading