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

Add API support for optional arguments #126

Merged
merged 10 commits into from
May 14, 2019

Conversation

nathanielhourt
Copy link

FC_API now supports optional arguments! Any trailing arguments of
an fc::optional type are now optional, and do not need to be
supplied. If omitted, they will be inferred to be null optionals.

Note that I have not made the binary_api_connection support optional arguments. This is never used, and thus I didn't feel it was worth the effort. We might consider removing binary_api_connection entirely.

@nathanielhourt
Copy link
Author

nathanielhourt commented May 2, 2019

Oh, hmm, we'll need to require a less ancient boost or ship hana ourselves... I don't know how to manipulate parameter packs that way without hana. :( Hana is header-only so shipping it is fairly trivial.

@pmconrad
Copy link

pmconrad commented May 2, 2019

We're still targeting Ubuntu-16.04 which comes with boost-1.58. It would be good if we could avoid bumping boost for now.
(FTR, boost::hana came with 1.61.)

@pmconrad
Copy link

pmconrad commented May 2, 2019

Btw, binary_api_connection has already been removed by #120

@nathanielhourt
Copy link
Author

OK, so what say I just add hana's headers to FC's vendor directory?

@pmconrad
Copy link

pmconrad commented May 3, 2019

Hm, that's a lot of stuff, but might be worth it.

@nathanielhourt
Copy link
Author

nathanielhourt commented May 3, 2019

Alternatively, I can just add the headers I'm using right now. Is that better? We can just leave it like that until Ubuntu 16.04 goes EOL and we can bump the minimum boost to 1.61+

@nathanielhourt
Copy link
Author

So I thought about it for quite a while and figured out a way to do it without hana, so I just rolled my own solution. =)

tests/api.cpp Outdated Show resolved Hide resolved
tests/api.cpp Outdated Show resolved Hide resolved
tests/api_tests.cpp Outdated Show resolved Hide resolved
Copy link

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one thing I don't understand yet:
Given a call with n fixed and m optional parameters, what triggers the instantiation of the m+1 required templates for n, n+1, ..., n+m actual parameters?

For the client side this is obvious, but for the server side it isn't. In the unit test, both client and server are within the same compilation unit, so the client-side trigger is probably sufficient for both.

Am I missing something?

src/network/http/websocket.cpp Outdated Show resolved Hide resolved
src/network/http/websocket.cpp Show resolved Hide resolved
@nathanielhourt
Copy link
Author

nathanielhourt commented May 8, 2019

There is one thing I don't understand yet:
Given a call with n fixed and m optional parameters, what triggers the instantiation of the m+1 required templates for n, n+1, ..., n+m actual parameters?

For the client side this is obvious, but for the server side it isn't. In the unit test, both client and server are within the same compilation unit, so the client-side trigger is probably sufficient for both.

OK, so as you see, the handling of arguments from the client side is all done in api_connection.hpp where it just checks if the missing parameters are optional types, and if so, defaults them to null. That was easy enough, but it doesn't apply if the caller is in C++. It only comes into play if the remote end passes a short argument list, which the C++ will never do. I've tested that it works by calling with curl, but I didn't write a unit test for this...

The server-internal side (call from C++ that knows about the FC_API) was the tricky part. Initially, my plan was to do as you said: generate templates for all the possible calls, so that when client code makes a call, there's a matching prototype waiting for it.

Seems simple enough in principle, but I couldn't get it working. I couldn't figure out a way to keep track of the left-side (non-optional) parameter types while iterating across to the right-side (optional) ones. It seemed like I needed to take two parameter packs in the same template, like <typename... ProvidedArgs, typename Arg, typename... MissingArgs> -- but two packs like that is not allowed (it totally could be, if you used some disambiguating delimiter between the packs like an int parameter, but the standard doesn't allow that, probably because it would be an epic hack).

Finally, I realized I didn't need to generate all possible overload templates at all; instead, I can make a single template that gets instantiated by the call site with the arguments the caller provided. From there, I check whether the provided arguments are compatible with the expected ones, and if so, make the call. This is workable because this mechanism allows me to have what I wanted: two parameter packs in the same template. The pack of expected parameters comes from the class instantiation, and the pack of the provided arguments from the method instantiation. This all happens in optionals_callable, as discussed below:

First we have the optionals_callable class instantiated with the pack of expected parameters from the function itself. The class itself is just a subclass of std::function, and it exposes std::function's function call operator so as to handle the case where the caller provides all arguments as expected.

    template<typename R, typename... Parameters>
    struct optionals_callable : public std::function<R(Parameters...)> {
       using std::function<R(Parameters...)>::operator();

Next is some infrastructure to cut up the parameter packs. I didn't need this with the original hana implementation, but I had to write it to replace hana. pack_cutter removes N types from the beginning of a parameter pack, and short_pack is just a dummy type that holds the pack of remaining types leftover from the cutting.

       template<typename... CutList>
       struct short_pack {};
       /// This metafunction removes the first several types from a variadic parameter pack of types.
       /// The first parameter is the number of arguments to remove from the beginning of the pack.
       /// All subsequent parameters are types in the list to be cut
       /// The result pack_cutter<...>::type is a short_pack<RemainingTypes>
       template<unsigned RemoveCount, typename... Types>
       struct pack_cutter;
/* implementation details elided */

Next is a simple helper function that just invokes a callable with default instantiations of the optional types. The only reason it's here is because I needed to call a separate function to re-extract the cut types pack from short_pack.

       template<typename F, typename... OptionalTypes>
       R call_function(F&& f, short_pack<OptionalTypes...>) {
           return f(OptionalTypes()...);
       }

And finally, we have the template that gets instantiated by the caller. It's SFINAE'd out unless the caller provides fewer arguments than the function expects. When instantiated, it makes a lambda to partially apply the std::function base class's call operator with the arguments that were provided, then passes that partial application to call_function above to instantiate and pass the null optionals, thus completing the argument list.

       /// Overload the function call operator, enabled if the caller provides fewer arguments than there are parameters.
       /// Pads out the provided arguments with default-constructed optionals, checking that they are indeed optional types
       template<class... Args>
       std::enable_if_t<sizeof...(Args) < sizeof...(Parameters), R> operator()(Args... args) {
           // Partially apply with the arguments provided
           auto partial_function = [this, &args...](auto&&... rest) {
               return (*this)(std::forward<decltype(args)>(args)..., std::move(rest)...);
           };
           // Cut the provided arguments' types out of the Parameters list, and store the rest in a dummy type
           pack_cutter_t<sizeof...(Args), std::decay_t<Parameters>...> dummy;
           // Pass the partially applied function and the dummy type to another function which can deduce the optional
           // types and call the function with default instantiations of those types
           return call_function(std::move(partial_function), dummy);
       }
};

@nathanielhourt
Copy link
Author

nathanielhourt commented May 8, 2019

Whew! Sorry if that was unnecessarily verbose, but I thought it would be good to detail out what I did and how it works, for posterity. :) Is it clear now?

nathanielhourt added a commit to nathanielhourt/fc that referenced this pull request May 8, 2019
@pmconrad
Copy link

pmconrad commented May 9, 2019

Thanks for the explanation. That's the part that I understood, but it's good to see it confirmed. :-)

I see now what I was getting at - call_generic in api_connection.hpp recurses through the m+1 calls and invokes the right one depending on the length of the variants input.

pmconrad
pmconrad previously approved these changes May 9, 2019
@abitmore
Copy link
Member

abitmore commented May 9, 2019

I hope that we didn't introduce new deep recursion issues in this PR.

@pmconrad
Copy link

Recursion depth in that call is the same as before, i. e. the number of parameters in the api method signature.

abitmore referenced this pull request in bitshares/bitshares-core May 10, 2019
@abitmore
Copy link
Member

We have a conflict in api.cpp now due to merge of #119 / 03cc93d. Easy to fix.

By the way, named arguments can be easily implemented with the extension template, as least for new APIs. Although not that easy for old APIs (consider compatibility). Have we discussed it in this issue?

For example,

struct argument_type {
  optional<int> arg1;
  optional<string> arg2;
};
using argument_list_type = optional<graphene::protocol::extension<argument_type>>;
// Declare the API
result_type api_name(argument_list_type arguments);

Then the clients can call the API with

{"method":"call", "parameters":[api_id_or_type_name,"api_name",[{"arg1":1,"arg2":"str"}]]}

@pmconrad
Copy link

named arguments can be easily implemented with the extension template

Nice idea. We should replace the assert in ext.hpp with FC_ASSERT though, to catch misspelled arguments.

@nathanielhourt
Copy link
Author

Thanks for the explanation. That's the part that I understood, but it's good to see it confirmed. :-)

I see now what I was getting at - call_generic in api_connection.hpp recurses through the m+1 calls and invokes the right one depending on the length of the variants input.

Not quite... call_generic will always recurse through all n+m arguments and will invariably invoke the API call with all fixed and optional arguments in its base case. It works by taking the parameter types extracted in the FC_API macro as a parameter pack, as well as an iterator range. Then it peels a parameter off of the pack, casts an argument from the iterator range to that type, and partially applies that value to the final call. It then recurses, passing one fewer type in the parameter pack, one fewer item in the iterator range, and the partially applied functor. The base case hits when the parameter pack is empty (all n+m arguments have been partially applied).

My change here was to check: if the argument wasn't provided in the range, and all remaining types in the parameter pack are optionals... then instead of partially applying an item from the range, partially apply a null optional.

FC_API now supports optional arguments! Any trailing arguments of
an fc::optional type are now optional, and do not need to be
supplied. If omitted, they will be inferred to be null optionals.
So sad... the hana version was way more expressive... But hey, I'm kinda
proud I actually got this working! :D
General cleanup of a lot of nonsense
Add tests of optional parameters
Add missing functionality to websocket_client and websocket_server to
make API tests more reliable and to make it possible to use a random
port for the tests.
It doesn't work as expected, so get rid of it.
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.

3 participants