Re: [cpp-netlib-devel] Comments about the HTTP client interface

[Dean M. B. <mik...@gm...>]

Hi Divye,

On Fri, Aug 29, 2008 at 2:25 AM, Divye Kapoor <div...@gm...> wrote:
>
> On Thu, Aug 28, 2008 at 12:55 PM, Dean Michael Berris
> <mik...@gm...> wrote:
>>
>
> It is quite likely that applications that base their decisions on "inputs"
> from the user will have to create a switch-case logic in their client code.
> It would save a lot of work for various users if the switch-case is
> implemented as a generic perform(...) method which throws an exception on
> invalid methods.

Right. But validating the input at runtime at the library level and
throwing an exception from there is a bad idea especially if you can
avoid it at compile time.

I'd like to keep the switch-case logic to remain in the application
code, because that is something that is specific to the application
and not something the library is supposed to provide. Much like how
std::map<>'s insert returns a pair containing an iterator and a
boolean -- the boolean says whether the data was inserted, and the
library user is free to decide what to do.

The logic that's choosing which HTTP method to perform should be
specific to the application, and is something that the library should
not know about.

>>
>> >> - Type safety (?) is broken because now the users will be able to
>> >> perform 'unsupported' or 'ill-formed' HTTP requests
>> >
>
> Not necessarily, see above.
>

Actually, the idea is simple:

1. Provide a limited HTTP interface to those supported to convey
exactly what the client does support.
2. If you want a custom HTTP client that supports other "non-standard"
methods, you extend the existing client and build on top of what's
already there.

I think type-safety is the wrong word, but it's more about keeping the
interface simple.

>>
>> > I can't think of one off-hand, but I think there may be cases where it
>> > makes sense to let the calling code vary the verb dynamically.
>> > Essentially, something like:
>> >
>> >  client.request("POST", headers, body);
>>
>> >
>
> I would see client.perform(request, method) as a useful addition to the
> client interface as it handles a common use-case.
>

Right, but if method was a string, then I have a problem with it. We
can make 'method' a function signature which performs a compile-time
dispatch to the correct (supported) implementation to choose. But then
that would bring me back to just implement an interface of the actual
methods that the client does support.

>
>>
>> 1. The request object is nowhere to be found (http::request object),
>> which should encapsulate all this information (headers, body) already
>> which is of very little consequence to the client.
>
> The client requires information about the request type (GET, POST etc) and
> the request object. We are simply providing the request type as a runtime
> string rather than as a static method. I realize that it will cause a
> performance penalty due to control paths being included in the code, but
> this can be treated as a convenience function that will reduce the number of
> ugly switch-case constructs in the client code while enforcing method
> validity by exceptions.
>

Why should the library do that, when the user who needs this
functionality should do that on their own? i.e. wrap the dispatching
for their case in a function, and do the determination at runtime and
keep the library simple and intuitive without exposing a potentially
unsafe/fragile and inefficient interface?

I'd rather provide a library that provides a set of robust
implementations than support a wider range of users and end up with an
"ugly" and unsafe interface. ;-)

>> 3. This means inside client::request(...) I'll have to check whether I
>> will ignore/use the body depending on the method string provided.
>> Contrast that to a fixed interface which actually at compile time only
>> puts the code that's needed to perform the request without having to
>> do checks of what to do at runtime.
>
> Users who want such a type of dynamic functionality will have to implement
> such a dispatch table in any case. This function could simply provide a
> dispatch table for each of the typed functions as a convenient shorthand. It
> localizes code that would otherwise have to be rewritten in different
> applications. For those who already know the type of request to make,
> choosing one of the static methods would still be a better option.
>

Right, in this case since they have to implement it for their
application (because their use-case is not the use-case I want to (or
IMO, doesn't make sense) to support) then they should do it and not
ask the library to support it for the general case.

>>
>> The static mapping between the behavior of client::get and HTTP GET
>> requests is intentional because primarily:
>>
>> 1. I don't see a point in making it a run-time check when it can be
>> made clear at compile-time.
>> 2. The semantics of usage are unclear when using 'dynamic' or runtime
>> values to do the decision making that can otherwise be done at compile
>> time in a clearer manner.
>
> I would say:
> 1. Use the typed functions when you already know which type of request to
> make.
> 2. Use the generic function if the decision is based on runtime variables.
> However, be aware of the performance pitfalls and the possible exceptions.
>

Sure, but then it would stick out like a sore thumb and beg the
question: "why do we even have the typed functions when there's
already the generic (unsafe) implementation?". To avoid this, we
support just #1, because that's the best thing to do, and just give
users that need that dispatch facility implement it themselves.

>>
>> If helping the developer avoid shooting themselves in the foot matters
>> (which I really think does, a lot) then keeping the static mapping and
>> semantics intact would be a good premium. I'd think it's more a
>> feature than a deficiency that the HTTP client allows for an
>> expressive interface to allow users to "say what they mean" instead of
>> leaving a wide open door to allow users to easily get things wrong.
>> One of the goals is to make it easier to use the library, and part of
>> that is to make 'getting it wrong' harder than getting it right. :)
>
> This would count on  my list as an ease of use feature with the possible
> performance penalties dissuading me from indiscriminate use. However, I
> acknowledge the merits of retaining a static only mapping to methods. I
> would be happy sticking to the static-only mapping if this feature is felt
> to be against the good sense of making 'getting it wrong' easier.
>

I tend to be conservative in this sense: for example, I'd like to
avoid questions like:

* why doesn't `client.perform(request, "get") do what
client.perform(request,"GET") do?`
* why does client.perform(request, "PSOT") not do anything?

So before the obvious bugs get to runtime, I'd rather:

* client.psot(request) // fail, because there's no method 'psot' in client
* client.GET(request) // fail, because there's no method 'GET' in client

Hope this makes sense why I have a problem supporting a generic
'.perform' method. ;-)

-- 
Dean Michael C. Berris
Software Engineer, Friendster, Inc.