Document and Test Orders API Endpoint

lin-d-hop commented 5 years ago

Description

As an enterprise user I want to be able to access my orders via the API. I want to be able to retrieve a set of completed orders that fall between two date-times. I want to be able to retrieve the set of orders for a specific enterprise. I want to only see orders for enterprises that I have appropriate permissions to view.

This Swagger page documents the current OFN API. https://app.swaggerhub.com/apis/haseleyi/ofn-staging/0.1#/

However this documentation is incomplete. @kristinalim examined app/controllers/api/orders_controller.rb and found that actually there is much more flexibility.

We would like to document and test this for the use case specified above.

Acceptance Criteria

After completion of this issue we want to be able to publicly state that this end point is stable for external use. We will begin to encourage integrations with OFN via the API for reporting etc.

To complete this issue please create a google doc with a set of test cases that demonstrate the API is working as intended.

UPDATE We don't really need a GDoc for this. I think I wrote that when I hoped this would be tackled by someone in a dev hack or something. If we can update the swagger docs ideally with some examples that people can use to test and copy that would be ideal. Also ideal would be specs that cover the endpoint!

Parameters:

q[distributor_id_eq]
q[completed_at_gt] (greater than)
q[completed_at_lt] (less than)
q[state_eq]
q[payment_state_eq]
q[email_cont] (contains)

Results: The API endpoint returns a JSON order as per the schema here: https://app.swaggerhub.com/apis/haseleyi/ofn-staging/0.1#/orders/get_orders

UPDATE Since this issue was created the following became the main swagger documentation: https://app.swaggerhub.com/apis/luisramos0/the-open_food_network/0.1#/orders/get_orders

In an ideal world we will merge these and just have one official published documentation... #scopecreep

The set of orders returned includes only orders with order.completed_at greater than or equal to start and less than or equal to end.

The set of orders returned should only be orders for the enterprise matching the enterprise_id parameter.

The set of orders returned must only include orders that the user has the authorisation to view based on their API key.

lin-d-hop commented 4 years ago

Recent testing suggests that /order is not working in curl though it does work in the browser.

When this issue is being worked on it would be great to confirm that. If the fix is easy I would suggest just do that within this issue but if it is a bit more involved we can make a new issue for it :-)

mbudm commented 4 years ago

Hi all have been looking at this issue over the past couple of days and here's what I've got to.

Swagger doc I learnt some of the history of this from @luisramos0 and read up on the initial project that was completed by some community contributors. They had the same thoughts that I had - can we automate the documentation of the API? They were looking at class based decorators which seemed quite time consuming. Also a lot of the API is Spree based, so decorating Spree classes is problematic.

I also chatted to @lin-d-hop about the documentation needs, whether a more friendly human readable document is needed or whether the Swagger/OpenAPI format is ok. We both felt that a well described and complete swagger doc is the best approach, the key being it has to have clear descriptions and be complete (and work - see auth!). Lin also flagged that it would be great if the public swagger doc was under a username that was more OFN official

Ransack object querying I've manually tested the query options that need to be documented. This is easy to do as the library we are using, ransack has good documentation on the search operators that it provides.

Translating these query options to Swagger documentation is interesting though as it's inconclusive how to document this in OpenAPI 3.x without verbosely documenting all the options. All options would theoretically be # of predicates * # of fields in the response model. Which for the orders endpoint is around 600 query options...! One option is to document the overall pattern with placeholders, like:

      parameters:
        - in: query
          name: q[<property><predicate>]
          schema:
            type: string
          style: deepObject
          description: Filter results with a <predicate> applied to <property> of '#/components/schemas/Order_Concise'. Predicates are defined in [Ransack Search Matchers](https://github.com/activerecord-hackery/ransack#search-matchers). 
          required: false

However this is probably not reliably understandable by all our API consumers. We could flesh out the description field with some concrete examples, that might make this approach work. I'd also need to mention sub properties here, which through trial and error I discovered are also just appended with an underscore separator For example:

distributor: {
  id: 4
},

Is queried as:

/api/orders?q[distributor_id_eq]=4

Documenting via tests In searching for some options for automating this process I cam across some libraries that suggest using tests to document the API, and then generate the swagger doc from the tests. This seems like a promising workaround to the automation issue raised by the original OFN swagger doc team - how do we document Spree based API endpoints. This is pertinent as we may move off Spree and continue to support the endpoints in our own app, so having docs (and tests) that survive this migration would be great. Having said that I haven't dived into all the discussion and docs from the original OFN swagger team, they may have been down this path and found an issue I haven't discovered yet.

The gems I found were rswag and rspec-rails-swagger. The latter only supports OpenApi 2.x and seems to be based off rswag so I decided tor try rswag first. I followed the getting started section and this immediately worked - WIP commit. Generating swagger from a spec seems to work fine, however I haven't started to doc the params yet which may be fiddly, and the tests don't work... because...

API authentication The tests generated by rswag respond with a 403 status, as I'm not providing any authentication. The current swagger doc mentions an api_key but on further investigation it looks like we may have removed support for this and now allow only browser based auth via login/cookie. Is that the case still @sauloperez ? Next week I was going to try adding auth to the tests which will I assume work, but I'm curious about how this situation works for API consumers - do they all just query via the browser?

API Load and support scope The last issue I want to realise is API load. The index endpoint and these quite complex query options are very powerful, but can we support them, as I imagine we could see some serious performance issues with a complex query.

Perhaps we should support only a subset of these queries that are most useful for or API consumers? The list above mentioned by @lin-d-hop seems like a good start? This would also reduce the complexity of API docs, and make the docs more comprehensible to API consumers.

kirstenalarsen commented 4 years ago

thanks @mbudm !! and/or should we be supporting the api from the replica database/s that are being discussed / setup for metabase? just linking across the conversations in case is relevant

mbudm commented 4 years ago

thanks @mbudm !! and/or should we be supporting the api from the replica database/s that are being discussed / setup for metabase? just linking across the conversations in case is relevant

Yeah that's a way around the load problem. In theory you could spin up another OFN instance just for API access that targets a replica db. Or for a little more work configure the API to hit a different db endpoint (solving db perf but possibly affecting the app perf).

lin-d-hop commented 4 years ago

The others params I'd add to the list above is

q[order_cycle_id_eq]
q[shipping_method_id_eq]
q[payment_method_id_eq]

With these additional search keys we can search for a list of orders with the same queries that we can search orders in our reports.

If any of these can already be supported by the API let's add them now. Any that cannot be supported off the cuff we can create separate issues to support them.

luisramos0 commented 4 years ago

Exciting!!!

We don't depend on spree_api anymore, we only depend on spree_core, thank god! All API endpoints are already on the OFN side and outside the spree namespace: controllers/api, that's it.

Good thinking on the "Ransack object querying". I think we should get to a strucuture that doesnt give us too much work but is usable. It doesnt have to be perfect. Agree, let's not support too many things, keep it simple. We don't necessarily need to support/document everything the API does (like exposing all the power of ransack), only what we think is useful for outside users.

Documenting via tests - this is really good, I'd invest a bit more time on it and verify if it will work for documeting params. I'd go for it if it works. I'd abandon it if it doesn't cover our basic needs.

API authentication works fine: https://github.com/openfoodfoundation/openfoodnetwork/wiki/API-documentation#authorization Except in the orders endpoint: bug recently found: https://github.com/openfoodfoundation/openfoodnetwork/issues/5500 It already has a PR, it just needs tests: https://github.com/openfoodfoundation/openfoodnetwork/pull/5475/files I have left this PR pending as it was not an S2. I think we should finish it as part of this work. It's a S1 from the API perspective. It's just that API is not YET a first class citizen product.

I think the API should be operational, we should run on the operational DB, not the replica. I'd be more inclined to use standard API cache solutions to resolve the load problems, when we have them.

Matt-Yorkley commented 4 years ago

In terms of those query params like q[payment_method_id_eq], they should be usable already, right? I don't think we whitelist them.

mbudm commented 4 years ago

I've been investigating using the rswag gem, to create tests for API endpoints, that will double as the source of an autogenerated Swagger (OpenAPI) spec. Rswag AFAICT defines a DSL that extends/maps to rspec methods.

However I've ran into a brick wall with authentication, so these notes are for anyone who is very familiar with Rspec and may be able to work out why things do not work as rswag and rspec documentation say they should on the OFN api.

In the meantime I'm updating the Swagger spec manually so that issue #3770 isn't blocked.

First up, here is the swagger_helper.rb that rswag docs suggest should be what we need to define authentication for tests using the OFN API:

# frozen_string_literal: true

require 'spec_helper'

RSpec.configure do |config|
  config.swagger_root = Rails.root.join('swagger').to_s
  config.swagger_docs = {
    'v1/swagger.yaml' => {
      openapi: '3.0.1',
      info: {
        title: 'API V1',
        version: 'v1'
      },
      components: {
        securitySchemes: {
          api_key: {
            type: :apiKey,
            name: 'X-Spree-Token',
            in: :header
          }
        }
      },
      paths: {},
      servers: [
          # ...
      ]
    }
  }
  config.swagger_format = :yaml
end

And this is a very basic test spec for the orders endpoint. I've added debug points prior to request being sent and when the response is received (note: I'm using the alternate syntax for run_tests! as this issue comment suggested it is better for debugging. It is.):

require 'swagger_helper'

RSpec.describe 'api/orders', type: :request do
  path '/api/orders' do

    get('list orders') do
      tags 'Order Tests'
      security [{ api_key: [] }]

      response(200, 'successful') do
        let(:api_key) { 'some key' }

          before do |example|
            binding.pry
            submit_request(example.metadata)
          end

          it do
            binding.pry
          end
      end
    end
  end
end

I'm also debugging in app/controllers/api/base_controller.rb to see what the API receives:

    def authenticate_user
      binding.pry
      return if @current_api_user = try_spree_current_user
    # ...

The first debug point in the before block shows the security and tags options are being included in the operation:

[6] pry(#<RSpec::ExampleGroups::ApiOrders::ApiOrders::Get::Successful>)> example.metadata
=> {:block=>#<Proc:0x00007fac692817a8@/Users/steveroberts/dev/openfoodnetwork/spec/requests/api/orders_spec.rb:39>,
 :description_args=>[],
 :description=>"",
 :full_description=>"api/orders /api/orders get successful ",
 :described_class=>:get,
 :file_path=>"./spec/requests/api/orders_spec.rb",
 :line_number=>39,
 :location=>"./spec/requests/api/orders_spec.rb:39",
 :absolute_file_path=>"/Users/steveroberts/dev/openfoodnetwork/spec/requests/api/orders_spec.rb",
 :rerun_file_path=>"./spec/requests/api/orders_spec.rb",
 :scoped_id=>"1:1:1:1:1",
 :type=>:request,
 :path_item=>{:template=>"/api/orders"},
 :operation=>{:verb=>:get, :summary=>"list orders", :tags=>["Order Tests"], :security=>[{:api_key=>[]}]},
 :response=>{:code=>200, :description=>"successful"},

At the base_controller debug point, there is no sign of the expected Header with the API key:

# headers are empty, but maybe not populated yet:
[3] pry(#<Api::OrdersController>)> @_headers
=> {"Content-Type"=>nil}

# request.env is more promising, would expect to see the header here?
[11] pry(#<Api::OrdersController>)> @_request.env
=> {"rack.version"=>[1, 1],
 "rack.input"=>#<StringIO:0x00007fac5f6752d8>,
 "rack.errors"=>#<StringIO:0x00007fac5f675418>,
 "rack.multithread"=>false,
 "rack.multiprocess"=>true,
 "rack.run_once"=>false,
 "REQUEST_METHOD"=>"GET",
 "SERVER_NAME"=>"www.example.com",
 "SERVER_PORT"=>"80",
 "QUERY_STRING"=>"",
 "PATH_INFO"=>"/api/orders",
 "rack.url_scheme"=>"http",
 "HTTPS"=>"off",
 "SCRIPT_NAME"=>"",
 "CONTENT_LENGTH"=>"0",
 "rack.test"=>true,
 "REMOTE_ADDR"=>"127.0.0.1",
 "REQUEST_URI"=>"/api/orders",
 "HTTP_HOST"=>"www.example.com",
 "CONTENT_TYPE"=>"application/x-www-form-urlencoded",
 "HTTP_ACCEPT"=>
  "text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5",
 "HTTP_COOKIE"=>"",
 "ORIGINAL_FULLPATH"=>"/api/orders",
 "action_dispatch.routes"=>

Lastly the debug point in the it block lets us examine the response, which has a @request and @response object in scope. No headers and a 401 (I tested it with an actual api key too - same result):

[4] pry(#<RSpec::ExampleGroups::ApiOrders::ApiOrders::Get::Successful>)> @request.headers
=> {"rack.version"=>[1, 1],
 "rack.input"=>#<StringIO:0x00007fac5f6752d8>,
 "rack.errors"=>#<StringIO:0x00007fac5f675418>,
 "rack.multithread"=>true,
 "rack.multiprocess"=>true,
 "rack.run_once"=>false,
 "REQUEST_METHOD"=>"GET",
 "SERVER_NAME"=>"www.example.com",
 "SERVER_PORT"=>"80",
 "QUERY_STRING"=>"",
 "PATH_INFO"=>"/api/orders",
 "rack.url_scheme"=>"http",
 "HTTPS"=>"off",
 "SCRIPT_NAME"=>"",
 "CONTENT_LENGTH"=>"0",
 "rack.test"=>true,
 "REMOTE_ADDR"=>"127.0.0.1",
 "REQUEST_URI"=>"/api/orders",
 "HTTP_HOST"=>"www.example.com",
 "CONTENT_TYPE"=>"application/x-www-form-urlencoded",
 "HTTP_ACCEPT"=>
  "text/xml,application/xml,application/xhtml+xml,text/html;q=0.9,text/plain;q=0.8,image/png,*/*;q=0.5",
 "HTTP_COOKIE"=>"",
 "ORIGINAL_FULLPATH"=>"/api/orders",
 "action_dispatch.routes"=>

 [3] pry(#<RSpec::ExampleGroups::ApiOrders::ApiOrders::Get::Successful>)> @response
=> #<ActionDispatch::TestResponse:0x00007fc47ba53ce0
 @blank=false,
 @body=["{\"error\":\"You are not authorized to perform that action.\"}"],
 @cache_control={:"no-cache"=>true},
 @charset="utf-8",
 @content_type=
  #<Mime::Type:0x00007fc4742b4f18
   @hash=-1510248677849182119,
   @string="application/json",
   @symbol=:json,
   @synonyms=["text/x-json", "application/jsonrequest"]>,
 @etag=nil,
 @header=
  {"Content-Type"=>"application/json",
   "X-UA-Compatible"=>"IE=Edge,chrome=1",
   "Cache-Control"=>"no-cache",
   "X-Request-Id"=>"0dca0f1eef5a04206b44edbb85711a5e",
   "X-Runtime"=>"4.135662",
   "Content-Length"=>"58"},
 @sending_file=false,
 @status=401>

My first thought was, this is because I'm doing something non standard. The rswag docs defaults show use of basic auth and an api_key in the query string, not as a header. So next I tried using these defaults to see if what happens.

# in swagger_helper.rb
securitySchemes: {
    basic_auth: {
        type: :http,
        scheme: :basic
    },
    api_key: {
        type: :apiKey,
        name: 'api_key',
        in: :query
    }
}

# in orders_spec.rb
    response(200, 'successful') do
        let(:Authorization) { "Basic XYZ" }
        let(:api_key) { 'some key' }

With these settings I'd expect to see a query parameter of api_key and an Authorization header. Neither were present. I was surprised that even the QUERY_STRING was empty.

So this led me to believe that something; rswag, the versions of rails/rpsec that OFN use, or some other lib/gem, one of these was restricting/manipulating what is added to requests.

At this point my plan was to write this doc to see if anyone had any ideas, and then move on to manually updating the swagger doc. However I thought it would still be nice to have some basic rspec request tests, to define that the API does indeed work as expected, and perhaps these could be converted to rswag at a later stage so we can eventually get swagger doc automation.

So I tried this vanilla rspec request test, being careful to ensure I was using an example of syntax from the rspec version we are using. I also tried a standard header first to see if that worked:

require 'spec_helper'

RSpec.describe 'Orders API', type: :request do

  describe 'GET /api/orders' do
    context 'as an authenticated user' do
      it 'returns status code 200' do
        headers = { "CONTENT_TYPE" => "application/json" }
        get "/api/orders", :params => { :widget => {:name => "My Widget"} }, :headers => headers

        expect(response).to have_http_status(200)
      end
    end
  end
end

And in the base_controller debug point, I see something very weird - the header I specified is stuffed into the QUERY_STRING:

"QUERY_STRING"=>"params[widget][name]=My+Widget&headers[CONTENT_TYPE]=application%2Fjson",
 "PATH_INFO"=>"/api/orders",
 "rack.url_scheme"=>"http",,
 "HTTPS"=>"off","/api/orders",
 "SCRIPT_NAME"=>"",  (press RETURN)
 "CONTENT_LENGTH"=>"0",
 "rack.test"=>true,
 "REMOTE_ADDR"=>"127.0.0.1",
 "REQUEST_URI"=>"/api/orders",
 "HTTP_HOST"=>"www.example.com",
 "CONTENT_TYPE"=>"application/x-www-form-urlencoded",

This is definitely a sign that I should halt and work on the manual swagger doc update instead.

Any ideas or clues into this seemingly odd behavior would be most welcome!

mbudm commented 4 years ago

In terms of those query params like q[payment_method_id_eq], they should be usable already, right? I don't think we whitelist them.

@Matt-Yorkley this is a dilemma, I think these are the options:

we document all the possible q[] params in swagger (~600 combinations by my count) - 😱
we document in a non standard way (eg example in my comment above) and let API users figure out the syntax - we could add a few common examples in the description to make it easier
we document only the params we want to support, effectively whitelisting.

Which option would you choose?

Matt-Yorkley commented 4 years ago

I don't think we can document all the combinations. We can give some examples of the most common/useful options. We can also point to the Ransack documentation: https://github.com/activerecord-hackery/ransack/blob/master/README.md#search-matchers

mbudm commented 4 years ago

@lin-d-hop a couple of the other fields you wanted to search on are currently not on the model that this endpoint uses, so yeah we should make new issues for these:

q[shipping_method_id_eq] q[payment_method_id_eq]

mbudm commented 4 years ago

Ah thanks @sigmundpetersen I came back on to just that ^. Not sure how I managed to close the issue though 🤔 Probably hit the wrong button in a hurry, whoops.

sigmundpetersen commented 4 years ago

No worries @mbudm 👍 Are you using the Zenhub extension? You can connect PR to the issue and they move along the pipe nicely together 🙂

luisramos0 commented 4 years ago

Hey @mbudm Based on this post: https://github.com/rswag/rswag/issues/48#issuecomment-283254574

I found a solution for your problem with the keywork parameter, have a look at this PR where the spec is working: https://github.com/mbudm/openfoodnetwork/pull/1/files

(I reverted back to the run_test! syntax just to rule out the possibility it was causing issues in my initial investigation, I haven't tried putting back the submit_request syntax but it should also work)

mbudm commented 4 years ago

No worries @mbudm 👍 Are you using the Zenhub extension? You can connect PR to the issue and they move along the pipe nicely together 🙂

Yep I have that set up - thanks

mbudm commented 4 years ago

Awesome @luisramos0 thanks! And that is super fascinating as the let(:api_key) syntax by my reading should have the same effect in run_test! as the parameter approach... 🤷‍♂️

Matt-Yorkley commented 4 years ago

a couple of the other fields you wanted to search on are currently not on the model that this endpoint uses, so yeah we should make new issues for these:

q[shipping_method_id_eq] q[payment_method_id_eq]

Ransack can search attributes on associations as long as the association is defined in the model, so there are probably a lot more options than we've listed. For exampleOrder has a belongs_to relationship with OrderCycle, so we can do this when searching orders: q[order_cycle_name_cont] = "Farmhouse" to search for orders where the order's order_cycle's name contains "Farmhouse". Or to find orders placed in London, you could do this: q[billing_address_city_eq] = "London".

Basically as well as q[<attribute>_<matcher>] we can also do: q[<associated_object>_<attribute>_<matcher>]. As far as I know, these options are not currently restricted either on the orders endpoint, so there are a vast number of potential combinations.

I think in the specific case of shipping method and payment method we might have some issues though. Currently we have to go through a convoluted route to get to those records. The shipping_method requires something like this: order.shipments.first.shipping_method, so it can't currently be accessed directly through an association, but we could change that quite easily. So for example, we could get rid of some of this complexity, have shipping_method directly available via a has_one association on order and clean up a lot of related mess elsewhere in the codebase whilst greatly improving the utility of this endpoint.

luisramos0 commented 4 years ago

I think that an has_many shipping_methods in the order would be more or less straight forward to get. The has_one shipping method would be more complicated because of the "selected" flag on shipping rates and all the code that changes it. This is definitely something we should explore!

We recently added the ship method filter to the orders api endpoint here: https://github.com/openfoodfoundation/openfoodnetwork/pull/5354/files

Re your order_cycle_name example, I wonder if it just works or if we would need to make sure the order_cycles table is included in the search query.

Matt-Yorkley commented 4 years ago

I'm pretty sure the example Just Works™, it's part of Ransack's basic functionality.

Matt-Yorkley commented 4 years ago

The has_one shipping method would be more complicated because of the "selected" flag on shipping rates and all the code that changes it.

All the complexity with shipping_rates and the selected flags etc is essentially there to handle Spree 2's logic of "orders can have multiple shipments", right? But in OFN, orders do not have multiple shipments. If we update the data model a bit to reflect the reality that Order actually has_one Shipment, I think we can :fire: a lot of that complexity, along with stuff like this:

https://github.com/openfoodfoundation/openfoodnetwork/blob/a91c81059f34335d12a43a024f636996b925d553/lib/open_food_network/orders_and_fulfillments_report/customer_totals_report.rb#L212-L220

Matt-Yorkley commented 4 years ago

the "selected" flag on shipping rates and all the code that changes it

luisramos0 commented 4 years ago

lol, yes, agree, but it has lots of implications in different parts of the code, both ofn and spree. We will only be able to do that after most of #4826

openfoodfoundation / openfoodnetwork

Document and Test Orders API Endpoint #3770

Description

Acceptance Criteria