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

Excluding action arguments with [FromUri] attribute from cache key #225

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Iamcerba
Copy link

Scenario: One of action arguments is a class attributed with [FromUri].

[HttpGet]
[Route(@"api/v{version:apiVersion}/users")]
public async Task<IHttpActionResult> GetUsers([FromUri]SearchUsersRequestDto searchRequest)
{
    ...        
}

As a result the following key generated:

userservice.web.api.v1.m0.controllers.usercontroller-getusers-searchRequest=UserService.Web.Api.V1.M0.Models.SearchUsersRequestDto&skip=0&take=10:application/json; charset=utf-8

Expected result:

userservice.web.api.v1.m0.controllers.usercontroller-getusers-skip=0&take=10:application/json; charset=utf-8

@Iamcerba Iamcerba changed the title Excluding action arguments with [FromUri] attribute from cache key. Excluding action arguments with [FromUri] attribute from cache key Dec 21, 2017
@JobaDiniz
Copy link

Hi, your implementation works for this specific scenario, but will break cases such as when using ModelBinder

[RequireCertificate]
        [HttpGet]
        [Route("~/secureapi/{culture}/bot/{botHandle}")]
        public async Task<IHttpActionResult> GetBot(string botHandle, [ModelBinder(BinderType = typeof(CertificateModelBinder))] MyCertificate certificate)

MyCertificate is a complex type and shouldn't be ignored in this case.
I think this library should implement some kind of hashing objects.

Related: #227

@Iamcerba
Copy link
Author

Hi, @JobaDiniz,

Could you please clarify why it will not work in your scenario?

I went on the path of least resistance and just removed redundant part from cache key, because seems it will always simply call .ToString() method of complex argument (in your scenario it will be something like this (if .ToString() will not be overridden): certificate={namespace].MyCertificate).
I didn't test, but believe that .GetQueryNameValuePairs() should handle rest of work...

@JobaDiniz
Copy link

Your implementation ignores reference types:

var actionParameters =.ActionArguments.Where(x => x.Value != null && (x.Value is string || x.Value.GetType().IsValueType))

You're filtering out reference types with GetType().IsValueType.

@Iamcerba
Copy link
Author

@JobaDiniz,

Yes, they are ignored. Their properties will be extracted from query string later as raw key-value pairs (i.e.: &point=1,2 will be threaded by model binder, but .GetQueryNameValuePairs() will add it to cache key as raw value: ...-...&point=1,2&...:application/json; charset=utf-8).

@JobaDiniz
Copy link

JobaDiniz commented Mar 12, 2018

Their properties will be extracted from query string later

That's my point, in the case of ModelBinder, you cannot assume the values will be in the query string. For example, in my case, they aren't.

[ModelBinder(BinderType = typeof(CertificateModelBinder))] MyCertificate certificate it doesn't come from body or querystring, it comes from client certificate.

It can come from headers or claims, etc.

@Iamcerba
Copy link
Author

I see now, thank you. But I believe old implementation also doesn't catch such scenario.

I assume client certificate is being used for authentication. If yes and if it is not used within controller as argument maybe it will be better to implement your own CacheKeyGenerator to store certificate data as part of cache key? Anyway it will be tricky to handle such scenarios within DefaultCacheKeyGenerator.

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

Successfully merging this pull request may close these issues.

2 participants