WithEndpoint() is evil

[knocte]

I know that we're already having conversations in other github-issues about mutability and concurrency, but maybe we can tackle things slowly - low hanging fruit first. Like this case:

• Having the ctor of FluentApi set the _requestData.UriPath property based on the type decorators is good, but
• The whole approach above gets destroyed as soon as we still leave the mutating WithEndpoint() method live. Can we kill it in favour of the other approach?

[gregsochanik]

+1

[raoulmillais]

As far as I can see in www.7digital.com we're only using this in unit tests to verify correct endpoints get called and I can't see how much benefit there is in that considering the fluent api is generic. This should be straightforward to fixup on our side.

[knocte]

Cool. Thing is, I just spent about 15 minutes trying to do the change to propose the PR, but inside the library itself it is being used in HasBasketParameterExtensions.

My understanding is that these extensions should be killed, and replaced by proper endpoint classes which have the corresponding [ApiEndpoint()] attribute. However I lack the BasketAPI knowledge to do this properly and quickly.

[raoulmillais]

That's a workaround for a flaw in the design of the metadata-based approach. The basket endpoints all return the same schema type Basket and we have no way of infering which url you mean when you ask for a FluentApi<Basket> it could be get, create, addItem etc

[raoulmillais]

Maybe we should allow allow multiple ApiEndpointAttributes on a schema object and overload it to allow an int (enum) and endpoint url on a schema type and check at the point the user calls Please() whether there are multiple and that they have specified which of those endpoints they want to call.

e.g.

Schema:

    [ApiEndpoint(Basket.Get, "basket")]
    [ApiEndpoint(Basket.Create,  "basket/create")]
    [ApiEndpoint(Basket.Add, "basket/addItem")]
    [XmlRoot("basket")]
    public class Basket : HasBasketParameter
    {
       ...
    }

Caller:

    Api<Basket>
        .Create
        .SelectEndpoint(Basket.Add)
        .AddItem(Guid.NewGuid(),1,1)
        .Please();

As I see it, that would:

1. Keep the metadata in the attributes.
2. Allow us to restrict which urls can be varied via that FluentApi method.
3. Remove magic strings from the fluent api methods and put the information into the schema (better locality of reference as the schema is more likely to change with the URLs than with the config/dispatch/deserialization process in the wrapper dll)

I'm still not entirely happy with that approach as you could pass a different enum type and it wouldn't complain , but I can submit a pull request, or feel free if you have the time, to give us something more concrete to discuss.

[gregsochanik]

The meta approach is an attempt to acknowledge the concrete relationship between a URI and it's resource entity (prob born of too much exposure to OpenRasta/Rest in practice!). It does help keep things nice and simple though....

Therefore, following the model strictly you'd end up with 4 different entities for 4 different endpoints:

[ApiEndpoint("basket")] public class Basket{}

[ApiEndpoint("basket/addItem")] public class BasketAddItem(){}

[ApiEndpoint("basket/create")] public class BasketCreate() {}

[ApiEndpoint("basket/removeItem")] public class BasketRemoveItem(){}

All with the same underlying structure, which could lend itself to some good old fashioned inheritance!

Then you could get rid of the AddItem and RemoveItem extension methods and just use one for all of them (apart from Create) called WithBasketId that takes a Guid.

I'll knock up and example toute suite to see if it works.

On Thu, Nov 15, 2012 at 8:40 PM, Raoul Millais notifications@github.comwrote:

Maybe we should allow allow multiple ApiEndpointAttributes on a schema object and overload it to allow an int (enum) and endpoint url on a schema type and check at the point the user calls Please() whether there are multiple and that they have specified which of those endpoints they want to call.

e.g.

Schema:

[ApiEndpoint(Basket.Get, "basket")]    [ApiEndpoint(Basket.Create,  "basket/create")]    [ApiEndpoint(Basket.Add, "basket/addItem")]    [XmlRoot("basket")]
public class Basket : HasBasketParameter
{
   ...
}

Caller:

Api<Basket>.Create.SelectEndpoint(Basket.Add).AddItem(Guid.NewGuid(),1,1);

As I see it, that would:

1. Keep the metadata in the attributes.
2. Allow us to restrict which urls can be varied via that extension method.
3. Remove magic strings and put the information into the schema dll.

I'm not entirely happy with that approach, but I can submit a pull request, or feel free if you have the time, to give us something more concrete to discuss.

— Reply to this email directly or view it on GitHubhttps://github.com/7digital/SevenDigital.Api.Wrapper/issues/40#issuecomment-10424478.

[gregsochanik]

https://github.com/7digital/SevenDigital.Api.Wrapper/pull/47

[gregsochanik]

This pull request contains the basic implementation I described above. It's potentially a breaking change but demonstrates how theses kind of api endpoints were supposed to be represented in the Schema.

I didn't change the underlying HasBasketParameter extension methods yet as I outlined above as thought this would be better dealt with in a separate commit. It does however allow us to remove WithEndpoint()

[knocte]

I like Greg's idea, it's a bit simpler, and in sync with the current design.

[raoulmillais]

Yeah I agree. It restricts the mechanism of control of which endpoint you're using to the generic type parameter, which makes the interface simpler for callers. The inheritance for the sake of it and the empty classes raises my hackles a bit, but I think it's an acceptable trade-off: the real issue is the choice of URLs in the API.

How about we choose more grammatical names though?

BasketCreate -> CreateBasket BasketAddItem -> AddItemToBasket BaketRemoveItem -> RemoveItemFromBasket

var basket = Api<CreateBasket>.Create.Please();
basket = Api<AddItemToBasket>.Create.AddItem(new Guid(basket.Id), 1234);

[gregsochanik]

Sure, fine by me!

The empty classes are the only downside to this model, but in my opinion, it highlights more an inefficiency with the api design; i.e. if we were being a bit more RESTful and leveraged HTTP a bit more efficiently, we wouldn't need these empty classes as we (perhaps) would have one endpoint being accessed via relevant http verbs and could pass an entity to add in the post body rather than the query string.

But as downsides go, they really are very minimal and will allow us to remove a lot of code, especially in the HasBasketParameterExtensions which I'm going through now.

e.g.

Api<AddItemToBasket>.Create.AddItem(new Guid(basket.Id), 1234);

would become

Basket basket = Api<BasketCreate>.Create.Please();
Basket updatedBasket = Api<AddItemToBasket>.Create.UseBasketId(new Guid(basket.Id)).WithReleaseId(1234).Please();

[gregsochanik]

PS - By "remove a lot of code", I mean from the Api.Wrapper codebase :-)

The 2 line example above I feel is better as it better describes what's going on in the api (i.e. 2 requests are being made)

[raoulmillais]

Agreed :+1:

[raoulmillais]

Ewww emoticons. Scorn on you github!

[gregsochanik]

:8ball:

[gregsochanik]

crikey - there's loads of them....! :goberserk:

[gregsochanik]

https://github.com/7digital/SevenDigital.Api.Wrapper/pull/47

[knocte]

Can WithEndpoint be killed now then?

[gregsochanik]

Yup, as far as I know! Go for it.

On Fri, Nov 16, 2012 at 1:07 PM, Andres G. Aragoneses < notifications@github.com> wrote:

Can WithEndpoint be killed now then?

— Reply to this email directly or view it on GitHubhttps://github.com/7digital/SevenDigital.Api.Wrapper/issues/40#issuecomment-10446300.

[AnthonySteele]

As of now, WithEndpoint() is no longer used in the .com website.