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

Declarative limits #2

Open
jezzsantos opened this issue Aug 7, 2018 · 10 comments
Open

Declarative limits #2

jezzsantos opened this issue Aug 7, 2018 · 10 comments
Assignees
Labels
Milestone

Comments

@jezzsantos
Copy link
Contributor

jezzsantos commented Aug 7, 2018

Hi Scott,

I am very tempted to use this plugin, but what is stopping me is that Id much rather declaratively set my limits on each operation/dto as an attribute, instead of in configuration settings.

Something like this in a service:

[LimitRate(5, 1)]
[LimitRate(15, 60)]
[LimitRate(100, 3600)]
public MyDtoReponse Post (MyDto request)
{
...
}

(Using the same rate limit set you demonstrate in the readme)

Is there some way to extend what you have to support this?
@wwwlicious
Copy link
Owner

Hi @jezzsantos

This would require a new implementation of the ILimitProvider that read the LimitRateAttribute values from the request DTO type, probably initialized on creation using reflection type scanning.

The plugin LimitProvider property could then just be set to use it.

Would welcome a PR to add this functionality if you decide to try this route.

@jezzsantos
Copy link
Contributor Author

jezzsantos commented Aug 7, 2018

Hey Scott, thanks.
I might give that a go, if you are saying conceptually it would work given the framework you have around it?
Anything you can see that might be a challenge for me?

@jezzsantos
Copy link
Contributor Author

jezzsantos commented Aug 7, 2018

Would you accept a version boost of ServiceStack to at least 4.5.8? or perhaps 4.5.14 (last version before 5.0.0)?

@wwwlicious
Copy link
Owner

@jezzsantos latest v4.5 would be fine. 👍
I think conceptually it will work.

@jezzsantos
Copy link
Contributor Author

Hey @wwwlicious

Well I started work on this, but I'm afraid I wont be able to go to 4.5.14, because it contains a bug that is not fixed until 5.0.2. (see https://forums.servicestack.net/t/stream-endpoint-change-after-4-5-8/6185/4).

My choices with my PR for this repo are go from 4.5.8 to 4.5.12, OR 4.5.8 to 5.0.2. (or 5.1.0).
Obviously the latter forces a massive piece of work on me (migrating from SS 4.X to 5.X) to move our other codebase before I can fully integrate this plugin.

@wwwlicious
Copy link
Owner

@jezzsantos any option to merge the fix back into the 4.x branch?

@jezzsantos
Copy link
Contributor Author

jezzsantos commented Aug 11, 2018

Yep, just got a workaround: https://forums.servicestack.net/t/stream-endpoint-change-after-4-5-8/6185/6.
I doubt Demis will release a 4.X version with this fix.

I am at least happy to go to 4.5.14 now! 👍

@jezzsantos
Copy link
Contributor Author

Hey @wwwlicious

Having a crap time trying to get a PR to test properly, can you have a look at #3?

The license is expired on the repo, and parallel testing cannot work since each test class will try to assign the ServiceStackHost to the same AppDomain.

@wwwlicious wwwlicious self-assigned this Aug 13, 2018
@wwwlicious wwwlicious added this to the 4.5.0 milestone Aug 13, 2018
@jezzsantos
Copy link
Contributor Author

Sorry mate, gave up waiting for you to respond. Went our own way.

@wwwlicious
Copy link
Owner

no problem

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

No branches or pull requests

2 participants