DEV-994 OFFSET and LIMIT Queries (#8)

* DEV-994 Rights API: OFFSET and LIMIT Queries
- Add `QueryParser` class.
- Process CGI parameters and Integrate `id` queries.
- Remove `wait-for` from Dockerfile and docker-compose
  - Use docker-compose healthchecks, remove `test` service
  - Rename test.yml to tests.yml
- Add millisecond timer and result field for Sequel operations.

.github/workflows/test.yml → .github/workflows/tests.yml

@@ -32,7 +32,7 @@ jobs:
         run: docker compose run web bundle exec standardrb
 
       - name: Run tests
-        run: docker compose run test
+        run: docker compose run web bundle exec rspec
 
       - name: Report to Coveralls
         uses: coverallsapp/[email protected]

Dockerfile

@@ -12,5 +12,3 @@ WORKDIR /usr/src/app
 ENV BUNDLE_PATH /gems
 #
 RUN gem install bundler
-
-RUN wget -O /usr/local/bin/wait-for https://github.com/eficode/wait-for/releases/download/v2.2.3/wait-for; chmod +x /usr/local/bin/wait-for

README.md

@@ -4,9 +4,9 @@
 ```bash
 git clone https://github.com/hathitrust/rights_api
 cd rights_api
-docker-compose build
-docker-compose run --rm web bundle install
-docker-compose up -d
+docker compose build
+docker compose run --rm web bundle install
+docker compose up -d
 ```
 
 ## Naming Conventions
@@ -59,7 +59,7 @@ See `lib/rights_api/app.rb` for all of the Sinatra routes.
 
 ## Results
 
-All API results, even 404s (which should only occur with a bad Sinatra route) should return
+All API results, even `404`s (which should only occur with a bad Sinatra route) should return
 the same general JSON structure. Here's the empty variant:
 
 ```JSON
@@ -93,6 +93,11 @@ Here's a truncated result from `http://localhost:4567/v1/access_profiles`:
 }
 
 ```
+
+`400` error results from bogus parameters (e.g., `&limit=blah`) will have an additional
+`error` Field in the return structure. This is human-readable and may include a backtrace,
+we're not sure yet.
+
 ## Testing
 The test suite is divided into unit and integration tests which can be run separately to
 give some orthogonality in checking for coverage gaps.

docker-compose.yml

@@ -1,8 +1,16 @@
 ---
 version: '3'
 
-services:
+x-condition-healthy: &healthy
+  condition: service_healthy
+
+x-healthcheck-defaults: &healthcheck-defaults
+  interval: 5s
+  timeout: 10s
+  start_period: 10s
+  retries: 5
 
+services:
   web:
     build: .
     ports:
@@ -15,18 +23,7 @@ services:
       RIGHTS_API_DATABASE_CONNECTION_STRING: "mysql2://ht_rights:ht_rights@mariadb/ht"
       RIGHTS_API_LOGGER_LEVEL: 1 # Logger::INFO
     depends_on:
-      - mariadb
-
-  test:
-    build: .
-    volumes:
-      - .:/usr/src/app
-      - gem_cache:/gems
-    command: bash -c "/usr/local/bin/wait-for mariadb:3306 && bundle exec rspec"
-    environment:
-      RIGHTS_API_DATABASE_CONNECTION_STRING: "mysql2://ht_rights:ht_rights@mariadb/ht"
-    depends_on:
-      - mariadb
+      mariadb: *healthy
 
   mariadb:
     image: ghcr.io/hathitrust/db-image
@@ -38,6 +35,9 @@ services:
       MYSQL_DATABASE: ht
       MYSQL_USER: ht_rights
       MYSQL_PASSWORD: ht_rights
+    healthcheck:
+      <<: *healthcheck-defaults
+      test: [ "CMD", "healthcheck.sh", "--su-mysql", "--connect", "--innodb_initialized" ]
 
 volumes:
   gem_cache:

lib/rights_api.rb

@@ -7,6 +7,7 @@ module RightsAPI
 require "rights_api/database"
 require "rights_api/query"
 require "rights_api/result"
+require "rights_api/result/error_result"
 require "rights_api/schema"
 require "rights_api/services"
 require "rights_api/settings"

lib/rights_api/app.rb

@@ -10,6 +10,7 @@
 
 require_relative "query"
 require_relative "result"
+require_relative "result/error_result"
 require_relative "schema"
 
 module RightsAPI
@@ -26,22 +27,29 @@ class App < Sinatra::Base
     # The full-featured search
     Schema.names.each do |name|
       get "/v1/#{name}/?" do
-        do_query(table_name: name)
+        params = CGI.parse(request.query_string)
+        model = Schema.model_for(name: name)
+        do_query(model: model, params: params)
       end
     end
 
     # The "by id" queries
     Schema.names.each do |name|
       get "/v1/#{name}/:id" do |id|
-        do_query(table_name: name, id: id)
+        model = Schema.model_for(name: name)
+        params = {model.default_key.to_s => [id]}
+        do_query(model: model, params: params)
       end
     end
 
     private
 
-    def do_query(table_name:, id: nil)
-      query = Query.new(table_name: table_name)
-      json query.run(id: id).to_h
+    def do_query(model:, params: {})
+      query = Query.new(model: model, params: params)
+      json query.run.to_h
+    rescue QueryParserError, Sequel::Error => e
+      status 400
+      json ErrorResult.new(exception: e).to_h
     end
   end
 end

lib/rights_api/error.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+module RightsAPI
+  class QueryParserError < StandardError
+    # This is raised whenever the query parser determines the query is ill-formed.
+    # Should result in a 400 Bad Request response
+  end
+end

lib/rights_api/query.rb

@@ -1,30 +1,44 @@
 # frozen_string_literal: true
 
+require "benchmark"
+require "cgi"
+
+require_relative "error"
+require_relative "query_parser"
 require_relative "result"
 require_relative "services"
 
 module RightsAPI
   class Query
-    DEFAULT_LIMIT = 1000
-    attr_reader :table_name
+    attr_reader :model, :params, :parser, :total, :dataset
 
-    # @param name [String, Symbol] The name of the table to be queried.
-    def initialize(table_name:)
-      @table_name = table_name
+    # @param model [Class] Sequel::Model subclass for the table being queried
+    # @param params [Hash] CGI parameters submitted to the Sinatra frontend
+    def initialize(model:, params: {})
+      @model = model
+      @params = params
+      @parser = QueryParser.new(model: model)
+      @total = 0
+      @dataset = nil
     end
 
-    # @param id [String] The primary value to retrieve, or nil for all rows.
     # @return [Result]
-    def run(id: nil)
-      model = Schema.model_for name: table_name
-      dataset = model.base_dataset
-      if id
-        where = {model.query_for_field(field: model.default_key) => id}
-        dataset = dataset.where(where)
+    def run
+      # This may raise QueryParserError
+      parser.parse(params: params)
+      time_delta = Benchmark.realtime do
+        @dataset = model.base_dataset
+        parser.where.each do |where|
+          @dataset = dataset.where(where)
+        end
+        # Save this here because offset and limit may alter the count.
+        @total = dataset.count
+        @dataset = dataset.order(*parser.order)
+          .offset(parser.offset)
+          .limit(parser.limit)
+          .all
       end
-      dataset = dataset.order(model.default_order)
-        .limit(DEFAULT_LIMIT).all
-      result = Result.new(total: dataset.count)
+      result = Result.new(offset: parser.offset, total: total, milliseconds: 1000 * time_delta)
       dataset.each do |row|
         result.add! row: row.to_h
       end

lib/rights_api/query_parser.rb

@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+require "sequel"
+
+require_relative "error"
+
+# Processes the Hash of URL parameters passed to the API into an
+# Array of WHERE constraints, as well as LIMIT, and OFFSET values.
+# ORDER BY (other than the schema default) will be handled here
+# in a future iteration.
+module RightsAPI
+  class QueryParser
+    DEFAULT_LIMIT = 1000
+    DEFAULT_OFFSET = 0
+    attr_reader :params, :model, :where, :order, :offset, :limit
+
+    # @param model [Class] Sequel::Model subclass for the table being queried
+    def initialize(model:)
+      @model = model
+      @where = []
+      @order = []
+      @limit = DEFAULT_LIMIT
+      @offset = DEFAULT_OFFSET
+    end
+
+    def parse(params: {})
+      @params = params
+      params.each do |key, values|
+        key = key.to_sym
+        case key
+        when :offset
+          parse_offset(values: values)
+        when :limit
+          parse_limit(values: values)
+        else
+          parse_parameter(key: key, values: values)
+        end
+      end
+      @order = [model.default_order] if @order.empty?
+      self
+    end
+
+    private
+
+    # Parses a general search parameter and appends the resulting Sequel
+    # expression to the @where array.
+    # Currently only handles primary key searches.
+    # Example: a URL ending with ?id=1 results in:
+    # parse_parameter(key: :id, values: [1])
+    # @param key [Symbol] The search parameter
+    # @param values [Array] One or more parameter values
+    def parse_parameter(key:, values:)
+      values.each do |value|
+        @where << {model.query_for_field(field: key.to_sym) => value}
+      end
+    end
+
+    # Extract a single integer that can be passed to dataset.offset.
+    def parse_offset(values:)
+      @offset = parse_int_value(values: values, type: "OFFSET")
+    end
+
+    # Extract a single integer that can be passed to dataset.limit.
+    # @param values [Array] One or more limit values
+    def parse_limit(values:)
+      @limit = parse_int_value(values: values, type: "LIMIT")
+    end
+
+    # Extract integer offset= or limit= value from potentially multiple values.
+    # If multiple values, use the last.
+    # @param values [Array] One or more offset or limit values
+    # @param type [String] "OFFSET" or "LIMIT", used only for reporting errors.
+    # @return [Integer]
+    def parse_int_value(values:, type:)
+      value = values.last.to_i
+      # Make sure the offset can make a round-trip conversion between Int and String
+      # https://stackoverflow.com/a/1235891
+      if value.to_s != values.last
+        raise QueryParserError, "#{type} expression '#{values.last}' is not an integer (#{value.to_s.class} vs #{values.last.class})"
+      end
+      value
+    end
+  end
+end

lib/rights_api/result.rb

@@ -5,13 +5,14 @@
 # and then data is populated according to the requested offset and limit.
 module RightsAPI
   class Result
-    attr_reader :offset, :total, :start, :end, :data
+    attr_reader :offset, :total, :start, :end, :milliseconds, :data
 
     # @param offset [Integer] The offset=x URL parameter.
     # @param total [Integer] The total number of results, regardless of paging.
-    def initialize(offset: 0, total: 0)
+    def initialize(offset: 0, total: 0, milliseconds: 0.0)
       @offset = offset
       @total = total
+      @milliseconds = milliseconds
       @start = 0
       @end = 0
       @data = []
@@ -37,6 +38,7 @@ def to_h
         "total" => @total,
         "start" => @start,
         "end" => @end,
+        "milliseconds" => @milliseconds,
         "data" => @data
       }
       finalize h

lib/rights_api/result/error_result.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module RightsAPI
+  class ErrorResult < Result
+    def initialize(exception:)
+      super(offset: 0, total: 0)
+      @exception = exception
+    end
+
+    private
+
+    def finalize(hash)
+      # FIXME: should we include the backtrace or just the message?
+      hash[:error] = @exception.to_s + " #{@exception.backtrace}"
+      hash
+    end
+  end
+end