davidMcneil / rants

An async NATS client library for the Rust programming language.
Apache License 2.0
81 stars 11 forks source link

Allow Subjects to be constructed without parsing a String #17

Closed sphqxe closed 4 years ago

sphqxe commented 4 years ago

I want to construct subjects programmatically by selecting branches from a subject tree for a given NATS server. I see that Subject's internal representation uses Vec and a flag for wildcards and for a subject hierarchy already represented as a tree of structs in Rust it would make more sense to construct this Vec directly.

davidMcneil commented 4 years ago

Thanks for opening the issue and the PR! This has been an issue I have been wanting to address. My concern with this initial approach is that it becomes possible to construct invalid subjects. Whereas we can do some validation when parsing from a string.

I think there are a few possible paths forward:

  1. Allow creating any subject like the PR does and documenting that it is possible to create invalid subjects. However, there is something really nice about knowing a Subject is valid regardless of where it came from and we would lose that.
  2. Make Subject::new return a Result and have similar validation. A really crude method would be to build up a string from the Vec<String> and try and parse it.
  3. Add some a Token enum type and make some sort of SubjectBuilder that guarantees valid subject creation.

3 is probably the "correct" approach but requires the most work.

What are your thoughts? How are your representing the subject hierarchy in your code?

sphqxe commented 4 years ago

Hi! Currently in my code I just have a very naive implementation comprising of a single struct:

struct SubjectTreeNode {
    subject_str: String,
    subjects: Vec<SubjectTreeNode>
}

where subject_str is the token and subjects contains child subjects. This is of course, meant to represent a whole subject tree rather than a single subject for subscription.

I don't mind taking a shot at 3.

sphqxe commented 4 years ago

Just a draft, what do you think?

pub struct SubjectBuilder {
    tokens: Vec<String>
}

impl SubjectBuilder {
    pub fn new() -> Self {
        SubjectBuilder {
            tokens: Vec::new()
        }
    }

    pub fn add(mut self, subject: String) -> Self {
        // Need to add some checks here to check for illegal characters
        self.tokens.push(subject);
        self
    }

    pub fn add_wildcard(mut self) -> Self {
        self.tokens.push("*".to_string());
        self
    }

    pub fn add_full_wildcard(self) -> FullWildcardSubjectBuilder {
        FullWildcardSubjectBuilder (self)
    }

    pub fn build(self) -> Subject {
        let fwc = self.tokens.is_empty();
        Subject {
            tokens: self.tokens,
            full_wildcard: fwc
        }
    }
}

pub struct FullWildcardSubjectBuilder (SubjectBuilder);

impl FullWildcardSubjectBuilder {
    pub fn build(self) -> Subject {
        Subject {
            tokens: self.0.tokens,
            full_wildcard: true
        }
    }
}

So you can build subjects like:

let subject = SubjectBuilder::new()
   .add("foo")
   .add("bar")
   .add_wildcard()
   .add_full_wildcard()
   .build();

assert_eq!(format!("{}", subject), "foo.bar.*.>");

I didn't go with the Token enum because it seemed unnecessary.

davidMcneil commented 4 years ago

This looks great thanks! I will review your PR and will try and fix the CI errors you are seeing.