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

Support batch command #7

Open
stdweird opened this issue Apr 1, 2016 · 6 comments
Open

Support batch command #7

stdweird opened this issue Apr 1, 2016 · 6 comments
Assignees

Comments

@stdweird
Copy link
Owner

stdweird commented Apr 1, 2016

Allow the posibility of batching commands as the FreeIPA web does.
(method batch)

@NickCis NickCis self-assigned this Apr 2, 2016
@NickCis
Copy link
Collaborator

NickCis commented Apr 5, 2016

I was thinking in implementing a Net::FreeIPA::Request class. All Net::FreeIPA::API and Net::FreeIPA::Common methods will be constructors which return references of Net::FreeIPA::Request.

The Net::FreeIPA::Request will be responsible for storing the requests data and transform it into valid JSON. I think that the Net::FreeIPA::Convert class will be included in the Request in this way the coupling beetween Net::FreeIPA::Convert and Net::FreeIPA::RPC will be removed (the coupling is created by the rpc_api method).

In addition, the Request will have a method to process the query response, in this way Net::FreeIPA::Common method could modify the response or perform checks.

The batch method will be just a Net::FreeIPA::Common method which receives Net::FreeIPA::Requests objects as parameters.

The Net::FreeIPA class will implement the Common and Api methods through autoload.

I'll upload a branch when i finish a working draft of this.

@stdweird What do you think of this implementation?

@stdweird
Copy link
Owner Author

stdweird commented Apr 6, 2016

@NickCis can you give an mocked-up example how a client application would look like (or how it woul duse the batch code)? i want to avoid that it becomes unnecessarily complex to use for simple things.

i agree that the location of rpc_appi in Convert is wrong.

also have a look at #11. it simplifies the generated API code, and you can now pass option directly to rpc.
one such option could be __batch => 1, to indicate that the command is batch and that the actual rpc call is to be delayed (as opposed to a regular call, where the request can be performed immediatlly).

and for convenience we could create an autoload wrapper like

sub batch_$method {
    my $self = shift;
    return $self->api_$method(@_, __batch => 1);
}

@NickCis
Copy link
Collaborator

NickCis commented Apr 7, 2016

@stdweird A mock of how the client would call the batch method:

my $fi = Net::FreeIPA->new("host.example.com");
die("Failed to initialise the rest client") if ! $fi->{rc};
my $batch_response = $fi->batch(
    Net::FreeIPA::API::api_user_find("this"),
    Net::FreeIPA::API::api_user_find("is"),
    Net::FreeIPA::API::api_user_find("a"),
    Net::FreeIPA::API::api_user_find("example"),
);

# The idea is that $batch_response is a Net::FreeIPA::Response object (https://github.com/stdweird/p5-net-freeipa/pull/13#issuecomment-206947620)

if($batch_response){
  foreach my $response (@{$batch_response->result}){ 
      if($response){  # Do something
      }else {
          print 'Error', $batch_response;
      }
  }
}else {
  print 'Error', $batch_response;
}

In addition, api methos could be called without batch, as it was done previously (this is going to be achieve through Autoloading):

if ($fi->api_user_find("")) {
    print "Found ", scalar @{$fi->{result}}, " users\n";
} else {
    print "Something went wrong\n";
}

@stdweird
Copy link
Owner Author

stdweird commented Apr 7, 2016

@NickCis some remarks
a. i would like to use the batch method in a loop, so somethign like $batch_response->append(more_api) should be possible, to build up the whole batchrequest
b. i think you need a Request, BatchRequest, Response and BatchResponse class to make sure the code doesn't get too complicated (but it could also be SingleRequest and Request, i'm not sure if you consider the "request" as a complete batch request or not).
c. i'll adapt API as follows to allow easier development on your side

  • generate e.g. user_api functions, that return a very thin mocked Request instance (you can do the actually implementation of the full Request class)
  • autoload the current equivalent api_user_add, that works exactly as the current code
  • make all API function exportable (and with convenient groups, so you can do a use Net::FreeIPA::API qw(:user) to get all user_ functions

That way you can work on the (Batch)Request and equivalent (Batch)Response code.

@NickCis
Copy link
Collaborator

NickCis commented Apr 7, 2016

@stdweird
a. The simplest solution is to use perl arrays, as they are expanded, it's the same to pass an array that to pass many arguments to a function / method:

my @requests;
for (){
    push @requests, Net::FreeIPA::API::user_find();
}
my $response = $fi->batch(@requests);

b. As regards Request and BatchRequest, i think that it would be easier to treat the batch request as a Net::FreeIPA::Common method.

The BatchResponse is a good idea, i could be a subclass of Response.

c. It looks good to me!

If i have time, i'll try to write some code today or in the weekend, currently, i'm a bit busy.

@stdweird
Copy link
Owner Author

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

No branches or pull requests

2 participants