search

aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

Make condition collection a first class citizen #198

Closed davidpeden3 closed 7 years ago

davidpeden3 commented 7 years ago

This PR builds on top of https://github.com/aspnet/BasicMiddleware/pull/169 and cleans up the conditions model. It adds a dedicated ConditionsCollection.

dnfclas commented 7 years ago

Hi @davidpeden3, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

davidpeden3 commented 7 years ago

@Tratcher i've branched this work off of the global rules branch. if you like this, i'd prefer to roll it in to my global rules branch asap so that i'm not maintaining a branch of a branch.

davidpeden3 commented 7 years ago

@Tratcher

Sure, sorry. I had been heads-down in the code for a bit did not explain the context I had in my head. Of course this PR needs more clarity.

This branch off of the globalrules branch does two things:

First, it addresses the Condition abstraction issues I've gotten to know while working in the codebase.

Conditions are passed around as an IList and expose an OrNext property. The only reason this property exists on Condition is to support ConditionHelper.Evaluate. In the absence of a proper collection which should contain this property (as well as TrackAllCaptures), the condition itself maintains state. This PR creates a dedicated collection for Conditions and also houses the grouping and trackAllCaptures at the appropriate level which cleans up a lot of the related classes.

e.g.,:

Each individual Condition should not expose the behavior of the collection. With this implementation, it's possible to define different values per Condition which is invalid via file-based configuration:

https://github.com/aspnet/BasicMiddleware/pull/198/commits/347d7bd455e9b536422f48006553e2714f894f02#diff-b9bceaec9393b4ce7c2e1a9057e1ad9fL22

It doesn't make any sense to have to pass trackAllCaptures to a method that parses a Condition:

https://github.com/aspnet/BasicMiddleware/pull/198/files#diff-4e6cd3225a4ad73df916578745fa9724L121

Second, when I originally set out to implement global rules, I was literally translating the observed behavior we had configured in IIS to this middleware. For global rules, that turned out to be that the full URI was evaluated for matches, not just the path. I built that without much consideration to the rest of the codebase (I simply wasn't as familiar with it as I am today).

But it turns out that there is demand for greater flexibility than that and that it would be great for local (non-global) rules also to benefit from full URI comparison:

https://github.com/aspnet/BasicMiddleware/issues/174 and https://github.com/aspnet/BasicMiddleware/issues/196

So, it made more sense to me to adjust the code to reflect matching URIs on a path/full basis explicitly rather than implicitly from global rules.