Non atom group names

[tompave]

Non atom group names

[tompave]

Hey, hi,

I think you're right, AFAIR there isn't really a reason to force group names to be atoms. I could lift the restriction, but that's a breaking change and I need to think a bit about an upgrade path.

In the meantime, to get you unblocked, I'd suggest to use actor gates to simulate dynamic groups.
For example:

defmodule User do
  defstruct [:id, :email]
end

defmodule UserBucket do
  defstruct [:user]
end

defimpl FunWithFlags.Actor, for: UserBucket do
  def id(bucket) do
    "users_bucket:" <> bucket_id(bucket.user)
  end

  def bucket_id(user) do
    rem(user.id, 10) |> to_string
  end
end


user = %User{id: 42, email: "[email protected]"}

FunWithFlags.enable :my_feature, for_actor: %UserBucket{user: user}

if FunWithFlags.enabled?(:my_feature, for: %UserBucket{user: user}) do
  # ... feature-flagged stuff...
end

iex> FunWithFlags.all_flags
{:ok,
 [%FunWithFlags.Flag{gates: [%FunWithFlags.Gate{enabled: true,
     for: "users_bucket:2", type: :actor}], name: :my_feature}]}

[tompave]

I came up with the following solution:

Oh, yes, that would work too. As long as you don't end up dynamically creating un unchecked number of atoms, you should be fine (as you said, the VM wouldn't like it because atoms are GC'd).

The main concern about lifting the restriction is that the keys are converted to/from strings when they are inserted in Redis, so if the restriction is lifted there will be no way to know if a key was an atom or string, right?

Well, both the Redis and Ecto adapters will ultimately store the group name as a binary (redis, ecto) and then deserialize it as an atom. Off the top of my head the library could just always convert group names to binaries, including the user-provided ones (so that calls to the public functions wouldn't need to be changed). In that case, there wouldn't really be any need to distinguish between originally atom or binary group names.

[costaraphael]

Well, both the Redis and Ecto adapters will ultimately store the group name as a binary (redis, ecto) and then deserialize it as an atom. Off the top of my head the library could just always convert group names to binaries, including the user-provided ones (so that calls to the public functions wouldn't need to be changed). In that case, there wouldn't really be any need to distinguish between originally atom or binary group names.

Treating all group names as binaries implies that everything that implements the String.Chars protocol could be used as group name. That's awesome! The end user will not feel the difference, as he/she could continue using atom names, but it will be way safer, as no string -> atom conversion would be performed.

BTW, thanks for giving FunWithFlags a try :-)
I hope it's working well.

It's working pretty well so far, we just completed setting up the web UI with some basic auth, pretty awesome lib ^^ Thank you for taking the time and effort needed to create it 😄

[tompave]

Hi @costaraphael, that PR 👆 adds support for binaries as group names.

[tompave]

Of course, you'd enable users_bucket:2 in the web GUI instead of using a user directly. That was just a quick proof of concept.

[costaraphael]

I came up with the following solution:

defimpl FunWithFlags.Group, for: MyApp.User do
  
  # Atom based groups go here

  def in?(user, group) when is_atom(group) do
    in?(user, to_string(group))
  end

  def in?(user, "users_bucket:" <> bucket) do
    # bucket checking logic
  end

  # Other string based groups go here
end

Then I just enable the buckets via: FunWithFlags.enable(:some_flag, for_group: :"users_bucket:2").

The main concern about lifting the restriction is that the keys are converted to/from strings when they are inserted in Redis, so if the restriction is lifted there will be no way to know if a key was an atom or string, right?

[tompave]

BTW, thanks for giving FunWithFlags a try :-)
I hope it's working well.

[tompave]

implies that everything that implements the String.Chars protocol could be used as group name.

I wouldn't recommend going that far, because group names still need to be displayed in the web GUI. I guess that it would be like you say as a side effect of supporting binaries (if coded in a certain way), but I wouldn't officially support it.

[costaraphael]

Thanks a lot man! This will increase a lot the safety of the lib.

Also dynamic group names allow for some crazy (but useful) stuff. What do you think of adding some examples of this to the README? I could write them and send a PR next week, if you are okay with it ^^

[tompave]

Sure, go for it, thanks!
Maybe the Readme is a bit long already, and was thinking of moving some stuff to the wiki.

What if you add to the readme a link to the wiki, and add your examples to the wiki?

[tompave]

Sorry, gave it another thought and it would actually be easier to have it in the readme for now. I can sort out the wiki later. 👍

[costaraphael]

Or, it could be a page in the hexdocs, like the new phoenix documentation. What do you think?

[tompave]

The readme is currently included in the hexeocs, so it would end up there. As I said, I've been planning to move the examples somewhere else, and the hexeocs are def a good choice.