feat(flags): Do token validation and extract distinct id

[neilkakkar]

towards PostHog/posthog#22131

Some tests and much nicer handling of redis errors, although I want to double check if there's a better way.

For an example of usage, see #42

[neilkakkar]

TODO: because rust exposes all string handling, double check if something very bad will happen if I have a utf-16 character in the team name

[neilkakkar]

eh will come back to this, will remove team name from consideration for now, will handle this on string matching properties.

[neilkakkar]

looking for suggestions here

[xvello]

Probably not worth adding too much complexity for the time being. Once we have enough helpers that are generic enough, they could be moved to a separate common crate.

[neilkakkar]

works, sounds good!

[neilkakkar]

looking for suggestions here

[thiagovarela]

Maybe you could downcast from anyhow? something like

impl From<anyhow::Error> for FlagError {
    fn from(error: anyhow::Error) -> Self {
        match error.downcast::<CustomRedisError>() {
            Ok(CustomRedisError::NotFound) => FlagError::NotFound,
            Err(_) => FlagError::InternalServerError,           
        }        
    }
}

from what I could understand you would like to error when redis returns empty for a key and the other errors you bubble up to the api handler without the need to customise each one, and then let the "FlagError into response impl" assign the http status based on what you logic implies.

what I did in the past was something like this https://github.com/thiagovarela/rust-minijinja-htmx/blob/main/server/src/error.rs#L16-L47, without thiserror, and manually implementing whatever specialised error I'd get.

[neilkakkar]

ooh nice, thank you @thiagovarela , this is what I was missing! - implementing the From trait here, and downcasting (what I meant by reverse-coerce lol).

I think I want to handle these errors a little differently, so I'll keep the CustomRedisError, but good to know this is how I'd do it implicitly.

[xvello]

• Thinking of the redis module as an isolated thing, it makes sense for it to return it's own error type like you did. You can then implement impl From<CustomRedisError> for FlagError for the ? operator to work transparently. This would indeed be the cleanest way. Make sure that the match in your From impl does not use the _ fallback, so that you remember to update it when new enum variants are added
• A shortcut would be for RedisClient to directly return FlagErrors, like we do for capture sinks returning CatpureErrors. Not the best module isolation, but can be refactored later on

As we probably will want to extract the RedisClient in a common crate, I'd go with a separate error type though.

[neilkakkar]

Nice, thanks!

Yeah, agree, I didn't want to just return FlagErrors from redis. I feel a lot more comfortable doing this now that the type system will protect me from unknown thrown errors.

[xvello]

nit: since that dataset is relatively small, an in-process LRU cache would be very useful. We do have it in ingestion's TeamManager for example. Can be added later for sure, but happy to help there.

[neilkakkar]

oof tricky, how do you handle invalidations in this case? Team config options / api tokens would get updated out of sync via django. Or is the TTL here so low that it doesn't make a difference in practice 👀 .

Either way, good idea with the low ttl, will look into this!

[xvello]

The plugin-server cache has a 2 minute TTL, so tokens are still accepted 2 minutes after rotation, which is good enough in my book. Negative lookups (token is not valid) are cached for 5 minutes too, as it's less probable.

[xvello]

nit for another PR: this map_err could be removed if you implement From<CustomRedisError> for FlagError