servicestack-ratelimit-redis icon indicating copy to clipboard operation
servicestack-ratelimit-redis copied to clipboard

Update build

Open wwwlicious opened this issue 6 years ago • 17 comments

Update the build config

wwwlicious avatar Aug 13 '18 20:08 wwwlicious

Still to update Appveyor config

wwwlicious avatar Aug 14 '18 21:08 wwwlicious

Want a hand?

jezzsantos avatar Aug 14 '18 21:08 jezzsantos

Well I'm done for the night but you should see that the Feature tests now have an authenticated and unauthenticated client.

Rather than mocking everything, the goal is to create real services/dto's using both LimitProviders

(I've combined them so that both can be used but the redis script key should prob be refactoring out into a single instance away from limit providers.)

Then testing is just sending dto's via the clients to verify everything works and limits are hit, status codes returned as expected etc.

I'll pick it up tomorrow night but feel free to checkout dev and work on the above if you want to. 👍

wwwlicious avatar Aug 14 '18 21:08 wwwlicious

OK, will have a look. It a.m. here in ole antipodea, so will let you know something before you wake up. 👍

jezzsantos avatar Aug 14 '18 21:08 jezzsantos

Ideally, that pattern you would want to setup, is having a base test class that has all the tests in it that verifies that any LimitProvider works the way you (the interface designer) says it should work.

That way, a new LimitProvider can provide its own test class that inherits, and does specifically what it needs to provide the limits for the tests.

jezzsantos avatar Aug 14 '18 22:08 jezzsantos

Hey Scott, sorry was not able to help out much today. I think I will wait until you have things squared away to your own satisfaction first, before adding to it. I am happy to contribute the integration tests for LimitRateAttribute if you need me to. Or perhaps you want to do those, so you get to understand how it works better? your call?

I am however, very keen to use this from nuget now that we have the attribute. Can we soon get a new nuget release out there?

jezzsantos avatar Aug 14 '18 23:08 jezzsantos

@jezzsantos I'll put in an example of a attribute based integration test and leave you to flesh out more test cases. I'll also update the readme for config and leave a placeholder for describing attribute usage if you want a crask at that too.

btw, The plugin is called 'RateLimit', any objection to renaming the attribute to match so there is consistency when typing so both RateLimit and LimitRate are not both used?

Will release on nuget once the tests and docs have been updated and api has settled.

wwwlicious avatar Aug 15 '18 16:08 wwwlicious

Sweet, Ill have a crack today. Thanks.

As far as the name goes, if you look at it from the consumers point fo view. I.e the person who just installed the Plugin and now wants to apply the attribute to their DTO's.

They will be applying it to "limit the rate, of their API call (rather than rate the limit :) ). So I feel that [LimitRate] is more discoverable than [RateLimit]. However, it is just my opinion.

jezzsantos avatar Aug 15 '18 19:08 jezzsantos

I disagree on the name. If I've just installed a plugin called RateLimit, with RateLimitFeature and RateLimitHeaders etc, I would find RateLimitAttribute more discoverable because of it's consistency...

wwwlicious avatar Aug 15 '18 19:08 wwwlicious

Granted, so lets apply the maintainer test then. What about when you are looking at your DTO code in 6 weeks time? You have several hundred of these API's to scan. Which one says what it means - better than the other?

[RateLimit(1, PerSecond)]
public MyDtoResponse Get(MyDto request)
{
....
} 

OR

[LimitRate(1, PerSecond]
public MyDtoResponse Get(MyDto request)
{
...
}

jezzsantos avatar Aug 15 '18 19:08 jezzsantos

When I am RateLimiting my API, I think in terms of it's RateLimit. not it's Limit Rate so to me that makes the most sense.

wwwlicious avatar Aug 15 '18 19:08 wwwlicious

Righto, @wwwlicious you go for it.

jezzsantos avatar Aug 15 '18 19:08 jezzsantos

I am looking at repo right now. Did you put in those integration tests and placeholder in readme yet?

jezzsantos avatar Aug 15 '18 20:08 jezzsantos

Looks like there is a bug running the lua script with the version of redis-inside. Will need to figure out if I can get v4 running some other way. Won't be tonight though.

wwwlicious avatar Aug 15 '18 20:08 wwwlicious

In the develop branch I see 5 broken tests

jezzsantos avatar Aug 15 '18 20:08 jezzsantos

I'm confused @wwwlicious, where do you want me to go and add new tests?

jezzsantos avatar Aug 15 '18 20:08 jezzsantos

@jezzsantos Broken tests will be rewritten to use client but the lua script will need to fix the lua script/redis embedded instance first.

Not gotten to the other bits yet which is why you can't find them... haha!

wwwlicious avatar Aug 15 '18 20:08 wwwlicious