Let Flag get state for a given user, not only HTTP request

[tfiechowski]

Currently, the Flag has a get_state_for_request which accepts HTTP request. This is perfect for usage in views or very close to the API layer, but get's troublesome when the feature flag affects some logic deeper in the code.

I'd like to suggest implementing a similar method to one mentioned above, but accepting User instance instead of request. For example: get_state_for_user.

[Kos]

Hi, thanks for reporting!

Unfortunately the API you suggested doesn't fit the library's design: one of the main design goals was to avoid enforcing that rollouts are per-user, so that we could just as well have per-account flags, per-group flags... This is the main differentiator compared to django-waffle and the main reason why I decided to roll my own library.

That said!

The problem of using feature flags deeper in the code is very valid. I didn't anticipate that it would surface so quickly, but I agree that it needs to be addressed.

I've been thinking how the Subject interface should look like. Whatever we change here, it will bubble up to Flag interface too - these are kind of symmetric. This was one of my ideas:

Two methods (request or object)

class Subject(ABC):
    @abstractmethod
    def get_identifier_for_request(self, request: HttpRequest) -> Optional[str]:
        ...

    @abstractmethod
    def get_identifier_for_object(self, object: Any) -> Optional[str]:
        ...

Usage in client code would effectively be:

flag.get_state_for_request(request)
flag.get_state_for_object(user)

And custom subject implementations would look like:

class UserSubject(Subject):
    def get_identifier_for_request(self, request: HttpRequest) -> Optional[str]:
        return self.get_identifier_for_object(request.user)

    def get_identifier_for_object(self, user: Any) -> Optional[str]:
        if not isinstance(user, User):
            return None
        return str(user.pk) if user.is_authenticated else None

This looks easy to generalise to other models. Another example:

class IpAddressSubject(Subject):
    def get_identifier_for_request(self, request: HttpRequest) -> Optional[str]:
        return request.META.get("REMOTE_ADDR")

    def get_identifier_for_object(self, request: Any) -> Optional[str]:
        return None

Now I'm kind of confused - should get_identifier_for_object also work for requests? Or should it mean "all objects except requests"? If you call flag.get_state_for_object with a request, should it work or not?

So this API design leaves some gray zone & potential for mistakes. I'm hesitant to take this route, even though I liked the idea at first.

Another idea is to drop typing altogether and just go with:

One method (object)

class Subject(ABC):
    @abstractmethod
    def get_identifier_for_object(self, obj: Any) -> Optional[str]:
        ...

Usage in client code:

flag.get_state_for_object(request)
flag.get_state_for_object(user)

And here's an example how you could go around implementing your Subjects:

class UserSubject(Subject):
    def get_identifier_for_object(self, obj: Any) -> Optional[str]:
        if isinstance(obj, HttpRequest):
            request = obj
            return self.get_identifier_for_object(request)
        if isinstance(obj, User):
            user = obj
            return str(user.pk) if user.is_authenticated else None
        return None

class IpAddressSubject(Subject):
    def get_identifier_for_object(self, obj: Any) -> Optional[str]:
        if isinstance(obj, HttpRequest):
            return obj.META.get("REMOTE_ADDR")
        return None

(This can be further prettified using methoddispatch, but that's non-vanilla Python, so let's not require that.)

So we got much cleaner API for using flags, but at the price of more complexity in writing Subject subclasses. I'd like to guarantee that every Subject, built-in or custom, knows what to do with a request, so that the library can provide convenience request-oriented utils like view decorators, template tags, etc. This API can be problematic because it's so easy to write a Subject subclass that handles domain objects but doesn't handle requests.

Hybrid approach?

What if we kept the Subject interface just like in "two methods", but without propagating this change all the way to Flag interface? This means that you'd still write Subject subclasses with two separate methods (this is the easier way), but Flag would only have one get_state_for_object method that Does The Right Thing (tm) depending on the parameter type passed. Best of two worlds?

Main issue here is a cosmetic one: We'd have to document that the get_state_for_object is overloaded and can accept both requests and domain objects. I don't think it's problematic, but it still smells like non-Pythonic API design (Union[Request, Any]?) and I'm worried it might confuse people.

Or we could simply revert to the original idea with two methods, but explicitly prohibit calling get_flag_for_object with Request. Improper usage would fail early. Maybe this is indeed the cleanest approach? WDYT?

[Kos]

Interesting edge case I just thought of:

If I always query a flag with a request, then I'm safe - each Subject knows how to interpret a request instance, and things will work as intended.

However, if I query the flag with a User instance, then it will work correctly only if the flag is enabled using User-aware rollouts.

Scenario

The programmer writes an Account subject like this:

class AccountSubject(Subject):
    def get_identifier_for_request(self, request: HttpRequest) -> Optional[str]:
        if request.user.is_authenticated:
            return self.get_identifier_for_object(request.user.account)
        return None

    def get_identifier_for_object(self, obj: Any) -> Optional[str]:
        if not isinstance(obj, Account):
            return None
        return str(obj.pk)

Some feature flag is queried somewhere using users:

flag.get_value_for(user)

Now a rollout is created:

Rollout.objects.create(flag_id=..., subject='AccountSubject', enable_percentage=50)

The user might expect that this rollout will affect 50% of existing accounts, and the flag will return True for each user in affected accounts. However, the flag will always return False. This happens because this subject will always say "False" when queried with a User instance, which it doesn't know.

This problem doesn't appear if we have the restriction that flags can only be queried with requests, because every Subject understands requests by definition.

The problem is addressable by writing Subjects in a robust way:

class AccountSubject(Subject):
    def get_identifier_for_request(self, request: HttpRequest) -> Optional[str]:
        if request.user.is_authenticated:
            return self.get_identifier_for_object(request.user.account)
        return None

    def get_identifier_for_object(self, obj: Any) -> Optional[str]:
        if isinstance(obj, Account):
            return str(account.pk)
        elif isinstance(obj, AbstractUser):
            return str(user.pk)
        else:
            return None

[Kos]

The most generic way of formulating this problem would be:

When a programmer is quering a Flag in the code, they must make sure to only call flag.get_value with arg types that are known to be supported by every Subject that could possibly be used in the future.

This is simply unrealistic, because there's always the danger of users querying the flag with type A and then trying to roll it out for subjects of type B and being super-surprised about the outcome. This is very non-obvious.

I feel it should be possible to address this with better API design. Typed flags, typed subjects?

[Kos]

Imagine:

• all flags by default can only be queried using a Request
• you can declare a special type of flag that can be additionally queried with User instances, BUT now the system won't allow you to make a rollout for this flag with a subject that doesn't explicitely say that it knows what a User is.

This would also prevent some mind-boggling edge cases where you:

1. query the same flag in two places, once with a request, once with a user
2. create two rollouts for this flag, one based on IP address, one based on users

I honestly don't have an idea how the system should behave in such a twisted situation. In the solution I'm proposing here, it would never occur: you could do either 1 or 2, but not both.

WDYT?

[Kos]

api mockup:

f1 = Flag(...)
f1.get_value_for_request(req)     # ok

f2 = TypedFlag[User](...)
f2.get_value_for_request(req)     # ok 
f2.get_value_for_object(user)     # ok
f2.get_value_for_object(account)  # error

and custom subjects (note the improved typing):

class AccountSubject(TypedSubject[User, Account]):
    def get_identifier_for_request(self, request: HttpRequest) -> Optional[str]:
        if request.user.is_authenticated:
            return self.get_identifier_for_object(request.user)
        return None

    def get_identifier_for_object(self, obj: Union[User, Account]) -> Optional[str]:
        if isinstance(obj, Account):
            return str(account.pk)
        elif isinstance(obj, User):
            return str(user.account.pk)