Use custom struct implementation for filter values

[iffyio]

Filter values is currently exposed as a hashmap https://github.com/googleforgames/quilkin/blob/master/src/extensions/filter_registry.rs#L36 which could make it difficult to extend or change later on. Since a user only needs to set and get values with it we can expose a wrapper with around the map rather than the map itself - which would give us control over what methods to expose

[markmandel]

Curious - how do you think we'll change a Map later on?

[iffyio]

I was initially thinking while looking at the PR it would be nice to easily avoid cloning the same keys each time we replace a value in the map - in this case, the cloning will happen for every packet.
Also thought if we wanted to e.g do things like TTL on keys later on that won't be possible.

I figured since a HashMap doesn't currently have an upsert method we could have a wrapper with this method as well as only the essential get/delete methods. Though now I'm leaning towards just keeping the map to keep things simpler since I'm afraid we might end up duplicating a lot of the map's functionality on the wrapper in any case in the future - if we ever decide to do something like TTL then that could be worthy of a breaking change or another api

For the cloning issue, we can only clone once on the first insert and update the value otherwise with e.g

match ctx.values.get_mut(&self.context_key) {
  Some(prev_value) => std::mem::replace(prev_value, token),
  None => ctx.value.insert(self.context_key.clone(), token)
}

would you be willing to make this change in the PR?

[markmandel]

I'm trying to work out the value of TTL - the context only lives as far as the Filter invocation for each packet. So it's not persistent. So how would Time to Live work here?

[iffyio]

Oh, I was under the impression that we had a single map for the entire process lifetime but looking at the code now its created per packet as you mention 😛 Did we have a particular reason for doing it this way per packet?

[markmandel]

Yeah absolutely - otherwise it's a race condition waiting to happen if multiple Filters can access each other's context maps while many packets flow through the system.

[iffyio]

That makes sense, it can be nice to be able to say that the context is bound to processing a single packet, on the other hand having filter state that live beyond a single packet might be a thing but I think if that's needed later on we can do something about it then.
It'll still be nice to avoid re-cloning keys for each packet I'm not sure what that could look like now though I'll close this in any case then