Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ergonomics (accessing environment in rate limit instances) #6

Closed
hasufell opened this issue Jun 3, 2022 · 10 comments · Fixed by #10
Closed

Ergonomics (accessing environment in rate limit instances) #6

hasufell opened this issue Jun 3, 2022 · 10 comments · Fixed by #10
Assignees

Comments

@hasufell
Copy link
Contributor

hasufell commented Jun 3, 2022

I'm trying to access application environment specifically in the HasRateLimitStrategy instance (but also possibly in the HasRateLimitPolicy) one.

To access application environment there are two ways in servant afais:

  1. your custom monad
  2. the Context which is specifically for the combinators

I tried to shoehorn both into the instances, but I think it's impossible. The 3rd possibility (which is what sadly the README even suggests) is using unsafePerformIO.

In the end I went with overlapping the HasServer (RateLimit foo bar :> api) ctx instance, copy pasting and adjusting the internal code, then adding my own HasContextEntry constraint on top. This works ok-ish, but overall... this is rather unergonomic and hard to figure out.

If you have better suggestions, please share. Otherwise I suggest to add a more complicated example to the readme that does this.

@mbg mbg self-assigned this Jun 3, 2022
@mbg
Copy link
Owner

mbg commented Jun 3, 2022

Hi @hasufell, thanks for the feedback about this! I can see how being able to access the Servant context would be useful and it is a pretty easy change.

I have pushed 81a3fd5 and 8dd878b which implement this for HasRateLimitPolicy and HasRateLimitStrategy respectively.

I have also extended the README and the tests with an example that consumes a value from the context in a HasRateLimitPolicy policy.

Let me know if these changes work for you.

@hasufell
Copy link
Contributor Author

hasufell commented Jun 3, 2022

@mbg I tried something similar I think and I got stuck with that because now in order to make useful use of the Context, I need to specialize the key in HasRateLimitStrategy. But it doesn't have knowledge of the key type.

@hasufell
Copy link
Contributor Author

hasufell commented Jun 3, 2022

Maybe I should expand a little more:

  1. I use JWT auth
  2. Rate limiting information is embedded in the json token
  3. strategyValue is supposed to pattern match on the verified token and then decide about rate limiting

@mbg
Copy link
Owner

mbg commented Jun 3, 2022

The key type is essentially dictated by the Backend in use. Assuming that you are using wai-rate-limit-redis for example, the type of the backend returned by redisBackend is Backend ByteString. This corresponds to the type of key used by the backend to associate with the usage. So in that case you would always want type RateLimitPolicyKey ctx YourPolicy = ByteString and then convert your more concrete type (e.g. ApiKey in the case of my example) into a ByteString that the backend can then store.

In your case, it seems that you wouldn't really need a Backend at all, since the rate limiting information is stored in the JWT? In that case, I suspect that a workaround here might be to just build a dummy Backend that is parameterised over whatever more concrete type you want to use.

Instances of the HasRateLimitStrategy class aren't really intended to do anything key-specific, other than pass the Request -> IO key function from the Backend on to the underlying rate-limiting strategy.

@hasufell
Copy link
Contributor Author

hasufell commented Jun 3, 2022

Yes, my key type is ByteString and I'm using redis. Redis will store this key normally.

What I mean is that my Strategy building function needs knowledge of the key. strategyValue calls that function, but that doesn't typecheck, because one doesn't know about the key, while the other does.

@hasufell
Copy link
Contributor Author

hasufell commented Jun 3, 2022

In your case, it seems that you wouldn't really need a Backend at all, since the rate limiting information is stored in the JWT?

I do need a backend. There's just additional logic that will return False in strategyOnRequest on certain tokens.

The only workaround I see is moving that logic into the Backend itself and abusing BackendError for "rate limited"... but that feels wrong (and requires me to overwrite the existing redis backend).

strategyOnRequest is really the function that decides what is rate limited and what isn't. So it should be able to inspect the key as well.

@mbg
Copy link
Owner

mbg commented Jun 3, 2022

It's a bit tricky for me to visualise what exactly your code looks like at the moment, sorry. My current understanding is that you have something along the lines of this:

instance HasContextEntry ctx JWTData => HasRateLimitStrategy ctx CustomStrategy where
    strategyValue ctx backend getKey = case getContextEntry ctx of
        JWTDataCtr0 k0 -> ...
        JWTDataCtr1 k1 -> ...

Where k0 :: key0 and k1 :: key1 for some key0 and key1? And then you want to use some Strategy-building function in each of the branches involving different types of key, but also relating to the key type variable, which then doesn't work because key is universally quantified while key0 and key1 are (potentially) other / more concrete types?

Would parameterising HasRateLimitStrategy over key help you?

I would definitely try to avoid moving things into a Backend, especially since that type will change soon due #7.

@hasufell
Copy link
Contributor Author

hasufell commented Jun 7, 2022

Yes, this does work:

class HasRateLimitStrategy (ctx :: [Type]) strategy key where
    strategyValue ::
        Context ctx -> Backend key -> (Request -> IO key) -> Strategy

hasufell added a commit to hasufell/wai-rate-limit that referenced this issue Jun 7, 2022
hasufell added a commit to hasufell/wai-rate-limit that referenced this issue Jun 7, 2022
@mbg mbg closed this as completed in #10 Jun 7, 2022
mbg pushed a commit that referenced this issue Jun 7, 2022
@hasufell
Copy link
Contributor Author

hasufell commented Jun 7, 2022

Thanks, I guess this needs a major PVP version bump, because we broken the class API.

@mbg
Copy link
Owner

mbg commented Jun 7, 2022

Yes indeed. I am hoping to include the changes to Backend in the next release, which can then combine a bunch of breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants