yuki24/artemis

Allow to dynamically call the operation

Closed this issue · 4 comments

Hello,

I have a usage where I have some queries who can basically triggered by different UI actions, for this reason I would like to have an execute or call_operation that would accept a first param the name of the operation and then all the other args. For example

client.query_event(id: 1)

vs

client.execute(:query_event, id: 1)

This would allow to to make the :query_event param configurable. Internally it could also clean up some code so that the method_missing would delegate to the execute method instead.

 # Executes a given query, raises if we didn't define the operation
  #
  # @param [String] operation
  # @param [Hash] context
  # @param [Hash] arguments
  # @return [GraphQL::Client::Response]
  def execute(operation, context: {}, **arguments)
    raise Artemis::GraphQLFileNotFound unless self.class.resolve_graphql_file_path(operation)
    const_name = operation.to_s.camelize
    self.class.load_constant(const_name) unless self.class.const_defined?(const_name)
    client.query(self.class.const_get(const_name), variables: arguments, context: context)
  end

At Artsy we have a controller action that does what you described, which could be summarized to:

CLASS_MAP = {
  'accept' => AcceptOfferMutation,
  'confirm' => ApproveOrderMutation,
  'counter' => CounterOfferMutation,
  'decline' => DeclineOfferMutation,
  'pickup' => ConfirmPickupOrderMutation,
  'ship' => FulfillOrderMutation
}

# In controller:
def action
  type  = params[:mutation_type]
  klass = CLASS_MAP[type]

  # call the GraphQL API
  klass.do_stuff
end

@jonallured do you think we could clean up some of the code here if there was the execute method suggested above?

Hmmm, maybe! Being able to dynamically call an operation seems pretty neat, but then I would still be left with a response that has the name of the operation in it - for example:

operation_arguments = {}
response = client.approve_order(operation_arguments)
order = response.data.ecommerce_approve_order.order_or_error

Here there are two hard-coded pieces of information - the name of the operation (approve_order) and the name of the key in the data where my object is (ecommerce_approve_order).

So if I could do this:

operation_arguments = {}
operation_name = :approve_order
response = client.execute(operation_name, operation_arguments)

Then I'd still need to be able to do this:

operation_response_name = :ecommerce_approve_order
order = response.data[operation_response_name].order_or_error

I guess what I'm saying is that unless I can be dynamic on both the execution AND parsing of the response, this dynamism isn't really that valuable to me - does that make sense?

Thanks for the explanation @jonallured! So this wouldn't remove the need of an extra conditional unless the dynamically specified queries all have a consistent shape of response.

So in my imagination this:

case identifier
when 'my_query'
  Client.my_query(...)
when 'other_query'
  Client.other_query(...)
when ...

Could be re-written to:

data = Client.execute(my_query, ...) # Done!

When the reality is:

data = Client.execute(my_query, ...)

case identifier
when 'my_query'
  extract_data_from_my_query(data) # or render "my_query", ...
when 'other_query'
  extract_data_from_other_query(data) # or render "other_query", ...
when ...

Which does not look like a significant improvement.

@JanStevens Is your source code available online? Or is it possible to share with us a summarized snippet of code that you think could be simplified with the proposed method?

We rename the requested fields so they line up

mutation($input: [IdentifierTypeInput!]!) {
  action: identifierTypeUpsertMulti(input: $input) {
    response: identifierTypes {
      id
      name
    }
  }
}

This way our client can be generic

data = Client.execute(my_query, ...)
data.action.response

I must admit that the usage might be very slim but I like the concept overall since it then extract a lot of logic from method missing. I personally don't like to much magic in method missing and always let it delegate to another method