Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds default http targeting context accessor #416
Adds default http targeting context accessor #416
Changes from 5 commits
87cb9e2
e5fe69b
387f648
7706d4d
622095b
01ca93a
f419784
18a4492
abc8ebf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's debatable who should own this cache key. It's odd to see the more generic initializer and middleware reference the cache key from a very specific targeting context accessor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed... I still am unable to come up with a satisfying place to put this. I suppose the owner could be the middleware- because the middleware's whole goal is to ensure that the
TargetingContext
is available at that key.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The targeting context accessor doesn't need to share a key.
private static object _cacheKey = new object();
should work fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's 3 classes that interact with HttpContext.Items for targeting context.
Initializer
,Middleware
, and theTargetingContextAccessor
.Initializer
depends upon theMiddleware
. TheMiddleware
depends upon aTargetingContextAccessor
existing. TheMiddleware
andInitializer
are tightly coupled- so they definitely need to share a key. TheTargetingContextAccessor
doesn't have to share the same key, but it reduces our HttpContext.Items footprint if it does- and is a clear shared cache location.So for
Initializer
andMiddleware
, we are going to have a shared key- it could be either an object ref or the string key"Microsoft.FeatureManagement.TargetingContext"
. I slightly prefer the string key since it's public (on the HttpContext.Items and as a public property). The key needs a home, and if it was just these two classes it would either be on the middleware or a shared constants class.Now the
DefaultTargetingContextAccessor
comes in- which is our default implementation. It needs a cache key as well. It could either define its own key (as a string or object ref) or used a shared constants class.I think the shared constants class makes things the most clear- as FM is defining where
TargetingContext
should live onHttpContext.Items
. I ended up not liking adding the constants class because it introduces yet another public class, and I ended up going with theDefaultTargetingContextAccessor
.