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

Adds the ability to specify static (constant) values with REST requests #916

Closed
wants to merge 1 commit into from

Conversation

philpowers
Copy link

Adds the ability to specify static (constant) values with REST requests. This allows values to be bound to DTO variables without being included in a REST request path.

This is useful for cases where an intermediate Service would otherwise be necessary for the sole purpose of re-routing the request.

…ts; this allows values to be bound to DTO variables without being included in a REST request path.
@philpowers
Copy link
Author

Any comments or questions about this pull request? I was looking at it again, and from an API perspective it might be more clear if "variableBindings" was just called "defaults".

@mythz
Copy link
Member

mythz commented May 15, 2014

I'm not a fan of this approach it modifies the Route classes and doesn't flow through and gets emitted in the auto-generated routes, it's also not clear what the result should look like if it did.

I also don't think the intent of the API is clear and I don't want to encourage the approach of expanding the route definitions whenever we want to add custom behavior. I think this custom behavior is best done within a Request Filter and I'm looking at ways to make that easier to do.

@philpowers
Copy link
Author

I agree I could have used better names in the API. It's really just setting "defaults" for DTO properties when those DTO properties are not variables within the URL. I personally don't agree that this is really too much of a custom behavior.

I also didn't break any sort of compatibility (just added a new overload, not changing any existing ones), and the changes to the RestPath class were actually pretty minimal.

But anyway, your project your rules...

@philpowers
Copy link
Author

Since you didn't like this approach, I've been thinking about this in the context of Request filters. What do you think about this:

Currently, a Request filter callback is an Action type (ie, it returns "void"). What if instead it were a Func<IRequest, IResponse, object, object>, allowing the filter to return a whole new DTO object/type if it wanted to. If it DID change types, then that would effectively reroute the request.

Of course, that has potentially more compatibility issues than my original approach.

@scottmcarthur
Copy link
Contributor

@philpowers Just to jump in, because I am intrigued, can you give a use-case scenario for when you would want to use this? Why would you want to change the route internally like that?

@philpowers
Copy link
Author

I'm essentially developing a multi-tenant host where each tenant has its own schema defined and where what's considered a "subresource" to the main service is a first-class resource to each tenant (defined in the per-tenant schema definition).

I want to give each tenant its own API base path (eg: /api/TenantName). Tenant functionality is implemented through my own plugins, so at initialization time the names are known, so I think having a routes like /api/{TenantName}/{Resource} or any other sorts of fallbacks shouldn't be necessary, and all of this should be handled at app startup/configuration time instead of having to go through intermediate services, etc.

I have the ability to register the URLs with static names (not variables) but only at runtime (I can't do it through attributes).

For example, I have something like the following DTO:

[Route("/api/tenant/{TenantName}/resourceType1"]
public class ListResourceType1 : IReturn<List<ResourceType1>>
{
    public string TenantName { get; set; }

    public string SubResourceName { get; set; }  // This should be exposed to the tenant as a first-class resource

    public string Arg1 { get; set; }
}

I want this URL to go to the service that handles that DTO, with TenantName and SubResourceName already filled out (SubResourceName should be 'mySpecializedResourceType1'):

GET /api/TenantOne/mySpecializedResourceType1

Which could be straightforward enough, but then imagine I have this:

GET /api/TenantOne/mySpecializedResourceType2

that should be routed to a completely different Service.

I think this Pull Request gives all that functionality I need, because I can apply new paths to pre-existing DTO types with static paths and defaults for some subset of the DTO properties, and has (I believe) minimal compatibility issues.

@mythz
Copy link
Member

mythz commented May 17, 2014

I've attached the Matching Route to the IRequest and have added Typed Request Filters in this commit: 017925f

So you can achieve what you want with:

public override void Configure(Container container)
{
    RegisterTypedRequestFilter<ResourceType1>((req, res, dto) =>
    {
        var route = req.GetRoute();
        if (route != null && route.Path == "/tenant/{TenantName}/resourceType1")
        {
            dto.SubResourceName = "CustomResource";
        }
    });
}

@mythz mythz closed this May 17, 2014
@philpowers
Copy link
Author

Sweet, that's awesome! That also happens to provide a better mechanism for implementing per request-type filters that I'm doing for these same requests.

Thanks again.

@tystol
Copy link
Contributor

tystol commented May 18, 2014

Looks good Demis!

I wonder, is it worth extending to support interfaces? This would allow for use cases where you want to apply some custom request logic across multiple DTOs.

e.g. In one of my apps the subdomain is used for routing, so I use a global request filter like this:

public class RegionFilter : IPlugin
{
    public void Register(IAppHost appHost)
    {
        appHost.RequestFilters.Add(PopulateRegionDtos);
    }

    public static void PopulateRegionDtos(IHttpRequest httpRequest, IHttpResponse httpResponse, object dto)
    {
        var regionRequest = dto as IRegionRequest;
        if (regionRequest != null)
            regionRequest.Region = httpRequest.ExtractRegionFromSubdomain();
    }
}

which would become

public class RegionFilter : IPlugin
{
    public void Register(IAppHost appHost)
    {
        appHost.RegisterTypedRequestFilter(PopulateRegionDtos);
    }

     public static void PopulateRegionDtos(IHttpRequest httpRequest, IHttpResponse httpResponse, IRegionRequest dto)
    {
        dto.Region = req.ExtractRegionFromSubdomain();
    }
}

It doesn't shorten it all that much, just makes it slightly more explicit and removes the cast, so not sure its worth it, the implementation does become more complicated. Thoughts?

@mythz
Copy link
Member

mythz commented May 18, 2014

@Its-Tyson yep good idea, extended to execute TypedFilters on DTO interfaces as well in this commit: 85f3574

Letting you now do:

RegisterTypedRequestFilter<IHasSharedProperty>((req, res, dtoInterface) => {
    dtoInterface.SharedProperty = "Is Shared";    
});

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