From 7d8ded5ab855cbe6b3e7f5af7da5589a8346a00a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 20 Jun 2021 09:07:21 +0100 Subject: [PATCH 01/22] Bring simplified / cleaned up base controller into V1 - Inherits from ActionController::API - Lots of superfluous junk removed --- app/controllers/api/v1/base_controller.rb | 76 +++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 app/controllers/api/v1/base_controller.rb diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb new file mode 100644 index 0000000000..879b155491 --- /dev/null +++ b/app/controllers/api/v1/base_controller.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Api + module V1 + class BaseController < ActionController::API + include CanCan::ControllerAdditions + check_authorization + + attr_accessor :current_api_user + + before_action :authenticate_user + + rescue_from Exception, with: :error_during_processing + rescue_from CanCan::AccessDenied, with: :unauthorized + rescue_from ActiveRecord::RecordNotFound, with: :not_found + + private + + def spree_current_user + @spree_current_user ||= request.env['warden'].user + end + + # Use logged in user (spree_current_user) for API authentication (current_api_user) + def authenticate_user + return if (@current_api_user = spree_current_user) + + if api_key.blank? + # An anonymous user + @current_api_user = Spree.user_class.new + return + end + + return if (@current_api_user = Spree.user_class.find_by(spree_api_key: api_key.to_s)) + + invalid_api_key + end + + def error_during_processing(exception) + Bugsnag.notify(exception) + + render json: { exception: exception.message }, + status: :unprocessable_entity + end + + def current_ability + Spree::Ability.new(current_api_user) + end + + def api_key + request.headers["X-Spree-Token"] || params[:token] + end + helper_method :api_key + + def invalid_resource!(resource) + render json: { error: I18n.t(:invalid_resource, scope: "spree.api"), + errors: resource.errors }, + status: :unprocessable_entity + end + + def invalid_api_key + render json: { error: I18n.t(:invalid_api_key, key: api_key, scope: "spree.api") }, + status: :unauthorized + end + + def unauthorized + render json: { error: I18n.t(:unauthorized, scope: "spree.api") }, + status: :unauthorized + end + + def not_found + render json: { error: I18n.t(:resource_not_found, scope: "spree.api") }, + status: :not_found + end + end + end +end From c4e2c8cb4c45af21f071d3072d2c9c831bf0bcb8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 11:21:05 +0100 Subject: [PATCH 02/22] Tidy up error handling and update response formats for JSON:API standard Update translation key namespacing --- app/controllers/api/v1/base_controller.rb | 51 +++++++++++++++-------- config/locales/en.yml | 5 ++- 2 files changed, 37 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 879b155491..12f7f8748e 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -35,13 +35,6 @@ module Api invalid_api_key end - def error_during_processing(exception) - Bugsnag.notify(exception) - - render json: { exception: exception.message }, - status: :unprocessable_entity - end - def current_ability Spree::Ability.new(current_api_user) end @@ -49,27 +42,49 @@ module Api def api_key request.headers["X-Spree-Token"] || params[:token] end - helper_method :api_key - def invalid_resource!(resource) - render json: { error: I18n.t(:invalid_resource, scope: "spree.api"), - errors: resource.errors }, - status: :unprocessable_entity + def error_during_processing(exception) + Bugsnag.notify(exception) + + render status: :unprocessable_entity, + json: json_api_error(exception.message, backtrace: exception.backtrace) + end + + def invalid_resource!(resource = nil) + render status: :unprocessable_entity, + json: json_api_invalid( + I18n.t(:invalid_resource, scope: "api"), + resource&.errors + ) end def invalid_api_key - render json: { error: I18n.t(:invalid_api_key, key: api_key, scope: "spree.api") }, - status: :unauthorized + render status: :unauthorized, + json: json_api_error(I18n.t(:invalid_api_key, key: api_key, scope: "api")) end def unauthorized - render json: { error: I18n.t(:unauthorized, scope: "spree.api") }, - status: :unauthorized + render status: :unauthorized, + json: json_api_error(I18n.t(:unauthorized, scope: "api")) end def not_found - render json: { error: I18n.t(:resource_not_found, scope: "spree.api") }, - status: :not_found + render status: :not_found, + json: json_api_error(I18n.t(:resource_not_found, scope: "api")) + end + + def json_api_error(message, **options) + error_response = { errors: [{ detail: message }] } + if options[:backtrace] && (Rails.env.development? || Rails.env.test?) + error_response.merge!(meta: [options[:backtrace]]) + end + error_response + end + + def json_api_invalid(message, errors) + error_response = { errors: [{ detail: message }] } + error_response.merge!(meta: { validation_errors: errors.to_a }) if errors.any? + error_response end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 96c26482c5..95fcaecaec 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -165,7 +165,6 @@ en: no_default_card: "^No default card available for this customer" shipping_method: not_available_to_shop: "is not available to %{shop}" - card_details: "Card details" card_type: "Card type" cardholder_name: "Cardholder name" @@ -1410,6 +1409,10 @@ en: # API # api: + invalid_api_key: "Invalid API key (%{key}) specified." + unauthorized: "You are not authorized to perform that action." + invalid_resource: "Invalid resource. Please fix errors and try again." + resource_not_found: "The resource you were looking for could not be found." enterprise_logo: destroy_attachment_does_not_exist: "Logo does not exist" enterprise_promo_image: From 3128232d7ebe7ed152eb06e3d529cb5a45db5ad1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 20 Jun 2021 09:08:05 +0100 Subject: [PATCH 03/22] Create customers controller And add new customers routes. Disable them in production for now. --- app/controllers/api/v1/customers_controller.rb | 15 +++++++++++++++ config/routes/api.rb | 10 ++++++++++ 2 files changed, 25 insertions(+) create mode 100644 app/controllers/api/v1/customers_controller.rb diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb new file mode 100644 index 0000000000..472e2e92b3 --- /dev/null +++ b/app/controllers/api/v1/customers_controller.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Api + module V1 + class CustomersController < Api::V1::BaseController + def index; end + + def show; end + + def update; end + + def destroy; end + end + end +end diff --git a/config/routes/api.rb b/config/routes/api.rb index 062958c495..9fa70e1d13 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -87,6 +87,16 @@ Openfoodnetwork::Application.routes.draw do constraints: lambda { |_| Flipper.enabled?(:api_reports) } end + unless Rails.env.production? + namespace :v1 do + resources :customers + + resources :enterprises do + resources :customers, only: :index + end + end + end + match '*path', to: redirect(path: "/api/v0/%{path}"), via: :all, constraints: { path: /(?!v[0-9]).+/ } end end From cc4192047e976bc375c84ac687e99dd49b823691 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 11:26:48 +0100 Subject: [PATCH 04/22] Fill out customers controller Add customer serializer --- Gemfile | 1 + Gemfile.lock | 3 + .../api/v1/customers_controller.rb | 74 ++++++++++++++++++- app/serializers/api/v1/customer_serializer.rb | 13 ++++ 4 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 app/serializers/api/v1/customer_serializer.rb diff --git a/Gemfile b/Gemfile index 72210c3408..c7fe245943 100644 --- a/Gemfile +++ b/Gemfile @@ -57,6 +57,7 @@ gem 'devise-token_authenticatable' gem 'jwt', '~> 2.3' gem 'oauth2', '~> 1.4.7' # Used for Stripe Connect +gem 'jsonapi-serializer' gem 'pagy', '~> 5.1' gem 'rswag-api' diff --git a/Gemfile.lock b/Gemfile.lock index c5950fa8b2..a3a6b9e440 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -346,6 +346,8 @@ GEM json_spec (1.1.5) multi_json (~> 1.0) rspec (>= 2.0, < 4.0) + jsonapi-serializer (2.2.0) + activesupport (>= 4.2) jwt (2.3.0) knapsack (4.0.0) rake @@ -739,6 +741,7 @@ DEPENDENCIES jquery-ui-rails (~> 4.2) json json_spec (~> 1.1.4) + jsonapi-serializer jwt (~> 2.3) knapsack letter_opener (>= 1.4.1) diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 472e2e92b3..0d9da27542 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -1,15 +1,81 @@ # frozen_string_literal: true +require 'open_food_network/permissions' + module Api module V1 class CustomersController < Api::V1::BaseController - def index; end + skip_authorization_check only: :index - def show; end + before_action :set_customer, only: [:show, :update, :destroy] + before_action :authorize_action, only: [:show, :update, :destroy] - def update; end + def index + customers = search_customers - def destroy; end + render json: Api::V1::CustomerSerializer.new(customers, is_collection: true) + end + + def show + render json: Api::V1::CustomerSerializer.new(@customer) + end + + def create + authorize! :update, Enterprise.find(customer_params[:enterprise_id]) + @customer = Customer.new(customer_params) + + if @customer.save + render json: Api::V1::CustomerSerializer.new(@customer), status: :created + else + invalid_resource! @customer + end + end + + def update + if @customer.update(customer_params) + render json: Api::V1::CustomerSerializer.new(@customer) + else + invalid_resource! @customer + end + end + + def destroy + if @customer.destroy + render json: Api::V1::CustomerSerializer.new(@customer) + else + invalid_resource! @customer + end + end + + private + + def set_customer + @customer = Customer.find(params[:id]) + end + + def authorize_action + authorize! action_name.to_sym, @customer + end + + def search_customers + customers = visible_customers + customers = customers.where(enterprise_id: params[:enterprise_id]) if params[:enterprise_id] + customers.ransack(params[:q]).result + end + + def visible_customers + Customer.where(user_id: current_api_user.id).or( + Customer.where(enterprise_id: editable_enterprises) + ) + end + + def customer_params + params.require(:customer).permit(:email, :enterprise_id) + end + + def editable_enterprises + OpenFoodNetwork::Permissions.new(current_api_user).editable_enterprises.select(:id) + end end end end diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb new file mode 100644 index 0000000000..e87391f997 --- /dev/null +++ b/app/serializers/api/v1/customer_serializer.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Api + module V1 + class CustomerSerializer + include JSONAPI::Serializer + + attributes :id, :enterprise_id, :name, :code, :email + + belongs_to :enterprise, record_type: :enterprise, serializer: :id + end + end +end From 39f4feed4a128bd2d1ff7c4a5319af7e2d2d4bc8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 9 Oct 2021 20:14:56 +0100 Subject: [PATCH 05/22] Generate docs with `rails rswag` --- swagger/v1/swagger.yaml | 121 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index d387a8fd33..d341f1807d 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -3,6 +3,125 @@ openapi: 3.0.1 info: title: API V1 version: v1 -paths: {} +paths: + "/api/v0/orders": + get: + summary: list orders + tags: + - Orders + parameters: + - name: X-Spree-Token + in: header + schema: + type: string + - name: q[distributor_id_eq] + in: query + required: false + description: Query orders for a specific distributor id. + schema: + type: string + - name: q[completed_at_gt] + in: query + required: false + description: Query orders completed after a date. + schema: + type: string + - name: q[completed_at_lt] + in: query + required: false + description: Query orders completed before a date. + schema: + type: string + - name: q[state_eq] + in: query + required: false + description: Query orders by order state, eg 'cart', 'complete'. + schema: + type: string + - name: q[payment_state_eq] + in: query + required: false + description: Query orders by order payment_state, eg 'balance_due', 'paid', + 'failed'. + schema: + type: string + - name: q[email_cont] + in: query + required: false + description: Query orders where the order email contains a string. + schema: + type: string + - name: q[order_cycle_id_eq] + in: query + required: false + description: Query orders for a specific order_cycle id. + schema: + type: string + responses: + '200': + description: get orders + content: + application/json: + schema: + "$ref": "#/components/schemas/Order_Concise" + "/api/v1/customers/{id}": + get: + summary: Show customer + parameters: + - name: id + in: path + required: true + schema: + type: string + responses: + '200': + description: customer found + content: + application/json: + schema: + type: object + properties: + data: + type: object + properties: + id: + type: string + type: + type: string + attributes: + type: object + properties: + id: + type: integer + name: + type: string + nullable: true + code: + type: string + email: + type: string + required: + - id + - name + - code + - email + required: + - id + - type + - attributes + - relationships + relationships: + - enterprise + required: + - data + '404': + description: not found + content: + application/json: + schema: + type: object + properties: + error: + type: string servers: - url: "/" From 76f14a03c6f9bd478fb285707dc0d385e6ac58fa Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 11:28:51 +0100 Subject: [PATCH 06/22] Add specs and JSON schemas Include test helpers --- .rubocop_styleguide.yml | 5 + .rubocop_todo.yml | 1 + app/json_schemas/customer_schema.rb | 22 +++ app/json_schemas/errors_schema.rb | 24 +++ app/json_schemas/json_api_schema.rb | 65 +++++++ app/serializers/api/v1/customer_serializer.rb | 2 +- spec/requests/api/v1/customers_spec.rb | 166 ++++++++++++++++++ spec/support/api_helper.rb | 8 + spec/swagger_helper.rb | 16 ++ 9 files changed, 308 insertions(+), 1 deletion(-) create mode 100644 app/json_schemas/customer_schema.rb create mode 100644 app/json_schemas/errors_schema.rb create mode 100644 app/json_schemas/json_api_schema.rb create mode 100644 spec/requests/api/v1/customers_spec.rb diff --git a/.rubocop_styleguide.yml b/.rubocop_styleguide.yml index 31e65d089b..500b642c70 100644 --- a/.rubocop_styleguide.yml +++ b/.rubocop_styleguide.yml @@ -27,11 +27,16 @@ Metrics/BlockLength: "class_eval", "collection", "context", + "delete", "describe", "feature", + "get", "it", "member", "namespace", + "path", + "post", + "put", "resource", "resources", "scenario", diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 14cbe53446..1ba490f5ec 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -466,6 +466,7 @@ Metrics/BlockLength: - 'spec/lib/open_food_network/group_buy_report_spec.rb' - 'spec/requests/api/orders_spec.rb' - 'spec/spec_helper.rb' + - 'spec/swagger_helper.rb' - 'spec/support/cancan_helper.rb' - 'spec/support/matchers/select2_matchers.rb' - 'spec/support/matchers/table_matchers.rb' diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb new file mode 100644 index 0000000000..5e7cb94e21 --- /dev/null +++ b/app/json_schemas/customer_schema.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class CustomerSchema < JsonApiSchema + def self.object_name + "customer" + end + + def self.attributes + { + id: { type: :integer, example: 1 }, + enterprise_id: { type: :integer, example: 2 }, + first_name: { type: :string, nullable: true, example: "Alice" }, + last_name: { type: :string, nullable: true, example: "Springs" }, + code: { type: :string, nullable: true, example: "BUYER1" }, + email: { type: :string, example: "alice@example.com" } + } + end + + def self.required_attributes + [:enterprise_id, :email] + end +end diff --git a/app/json_schemas/errors_schema.rb b/app/json_schemas/errors_schema.rb new file mode 100644 index 0000000000..590dda849d --- /dev/null +++ b/app/json_schemas/errors_schema.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class ErrorsSchema + def self.schema + { + type: :object, + properties: { + errors: { + type: :array, + items: { + type: :object, + properties: { + title: { type: :string }, + detail: { type: :string }, + source: { type: :object } + }, + required: [:detail] + } + } + }, + required: [:errors] + } + end +end diff --git a/app/json_schemas/json_api_schema.rb b/app/json_schemas/json_api_schema.rb new file mode 100644 index 0000000000..1bc13098ef --- /dev/null +++ b/app/json_schemas/json_api_schema.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +class JsonApiSchema + class << self + def attributes + {} + end + + def required_attributes + [] + end + + def all_attributes + attributes.keys + end + + def schema(options = {}) + { + type: :object, + properties: { + data: { + type: :object, + properties: data_properties(**options) + }, + meta: { type: :object } + }, + required: [:data] + } + end + + def collection(options) + { + type: :object, + properties: { + data: { + type: :array, + items: { + type: :object, + properties: data_properties(**options) + } + }, + meta: { type: :object } + }, + required: [:data] + } + end + + private + + def data_properties(require_all: false) + required = require_all ? all_attributes : required_attributes + + { + id: { type: :string, example: "1" }, + type: { type: :string, example: object_name }, + attributes: { + type: :object, + properties: attributes, + required: required + }, + relationships: { type: :object } + } + end + end +end diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb index e87391f997..cea4d53d2e 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -5,7 +5,7 @@ module Api class CustomerSerializer include JSONAPI::Serializer - attributes :id, :enterprise_id, :name, :code, :email + attributes :id, :enterprise_id, :first_name, :last_name, :code, :email belongs_to :enterprise, record_type: :enterprise, serializer: :id end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb new file mode 100644 index 0000000000..a97667489d --- /dev/null +++ b/spec/requests/api/v1/customers_spec.rb @@ -0,0 +1,166 @@ +# frozen_string_literal: true + +require "swagger_helper" + +describe "Customers", type: :request do + let!(:enterprise1) { create(:enterprise) } + let!(:enterprise2) { create(:enterprise) } + let!(:customer1) { create(:customer, enterprise: enterprise1) } + let!(:customer2) { create(:customer, enterprise: enterprise1) } + let!(:customer3) { create(:customer, enterprise: enterprise2) } + + before { login_as enterprise1.owner } + + path "/api/v1/customers" do + get "List customers" do + tags "Customers" + parameter name: :enterprise_id, in: :query, type: :string + produces "application/json" + + response "200", "Customers list" do + param(:enterprise_id) { enterprise1.id } + schema CustomerSchema.collection(require_all: true) + + run_test! + end + end + + describe "returning results based on permissions" do + context "as an enterprise owner" do + before { login_as enterprise1.owner } + + it "returns customers of enterprises the user manages" do + get "/api/v1/customers" + expect(json_response_ids).to eq [customer1.id.to_s, customer2.id.to_s] + end + end + + context "as another enterprise owner" do + before { login_as enterprise2.owner } + + it "returns customers of enterprises the user manages" do + get "/api/v1/customers" + expect(json_response_ids).to eq [customer3.id.to_s] + end + end + end + + post "Create customer" do + tags "Customers" + consumes "application/json" + produces "application/json" + + parameter name: :customer, in: :body, schema: { + type: :object, + properties: CustomerSchema.attributes.except(:id), + required: CustomerSchema.required_attributes + } + + response "201", "Customer created" do + param(:customer) do + { + email: "test@example.com", + enterprise_id: enterprise1.id.to_s + } + end + schema CustomerSchema.schema(require_all: true) + + run_test! + end + + response "422", "Unprocessable entity" do + param(:customer) { {} } + schema ErrorsSchema.schema + + run_test! + end + end + end + + path "/api/v1/customers/{id}" do + get "Show customer" do + tags "Customers" + parameter name: :id, in: :path, type: :string + produces "application/json" + + response "200", "Customer" do + param(:id) { customer1.id } + schema CustomerSchema.schema(require_all: true) + + run_test! + end + + response "404", "Not found" do + param(:id) { 0 } + schema ErrorsSchema.schema + + run_test! do + expect(json_error_detail).to eq "The resource you were looking for could not be found." + end + end + end + + put "Update customer" do + tags "Customers" + parameter name: :id, in: :path, type: :string + consumes "application/json" + produces "application/json" + + parameter name: :customer, in: :body, schema: { + type: :object, + properties: CustomerSchema.attributes, + required: CustomerSchema.required_attributes + } + + response "200", "Customer updated" do + param(:id) { customer1.id } + param(:customer) do + { + id: customer1.id.to_s, + email: "test@example.com", + enterprise_id: enterprise1.id.to_s + } + end + schema CustomerSchema.schema(require_all: true) + + run_test! + end + + response "422", "Unprocessable entity" do + param(:id) { customer1.id } + param(:customer) { {} } + schema ErrorsSchema.schema + + run_test! + end + end + + delete "Delete customer" do + tags "Customers" + parameter name: :id, in: :path, type: :string + produces "application/json" + + response "200", "Customer deleted" do + param(:id) { customer1.id } + schema CustomerSchema.schema(require_all: true) + + run_test! + end + end + end + + path "/api/v1/enterprises/{enterprise_id}/customers" do + get "List customers of an enterprise" do + tags "Customers", "Enterprises" + parameter name: :enterprise_id, in: :path, type: :string, required: true + produces "application/json" + + response "200", "Customers list" do + param(:enterprise_id) { enterprise1.id } + schema CustomerSchema.collection(require_all: true) + + run_test! + end + end + end +end diff --git a/spec/support/api_helper.rb b/spec/support/api_helper.rb index 9ac5c7a1cb..d4591ece3d 100644 --- a/spec/support/api_helper.rb +++ b/spec/support/api_helper.rb @@ -14,6 +14,14 @@ module OpenFoodNetwork end end + def json_response_ids + json_response[:data].map{ |item| item["id"] } + end + + def json_error_detail + json_response[:errors][0][:detail] + end + def assert_unauthorized! expect(json_response).to eq("error" => "You are not authorized to perform that action.") expect(response.status).to eq 401 diff --git a/spec/swagger_helper.rb b/spec/swagger_helper.rb index d47d6a49ae..9bcd2c4064 100644 --- a/spec/swagger_helper.rb +++ b/spec/swagger_helper.rb @@ -3,6 +3,9 @@ require 'spec_helper' RSpec.configure do |config| + config.include Devise::Test::IntegrationHelpers, type: :request + config.include OpenFoodNetwork::ApiHelper, type: :request + # Specify a root folder where Swagger JSON files are generated # NOTE: If you're using the rswag-api to serve API descriptions, you'll need # to ensure that it's configured to serve Swagger from the same folder @@ -21,6 +24,12 @@ RSpec.configure do |config| title: 'API V1', version: 'v1' }, + components: { + schemas: { + error_response: ErrorsSchema.schema, + customer: CustomerSchema.schema + } + }, paths: {}, servers: [ { url: "/" } @@ -41,3 +50,10 @@ RSpec.configure do |config| # Defaults to json. Accepts ':json' and ':yaml'. config.swagger_format = :yaml end + +module RswagExtension + def param(args, &block) + public_send(:let, args) { instance_eval(&block) } + end +end +Rswag::Specs::ExampleGroupHelpers.prepend RswagExtension From 46f9d3ef81c2ef2a11933389672b7f271b818e05 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 14:31:50 +0100 Subject: [PATCH 07/22] Test permissions combined with Ransack searches --- spec/requests/api/v1/customers_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index a97667489d..93c233e699 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -43,6 +43,16 @@ describe "Customers", type: :request do expect(json_response_ids).to eq [customer3.id.to_s] end end + + context "with ransack params searching for specific customers" do + before { login_as enterprise2.owner } + + it "does not show results the user doesn't have permissions to view" do + get "/api/v1/customers", params: { q: { id_eq: customer2.id } } + + expect(json_response_ids).to eq [] + end + end end post "Create customer" do From 028d02ccca9395c801901da9233714028b387f5d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 14:46:36 +0100 Subject: [PATCH 08/22] Test serialized relationship data --- spec/requests/api/v1/customers_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 93c233e699..5b9cab952b 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -108,6 +108,20 @@ describe "Customers", type: :request do expect(json_error_detail).to eq "The resource you were looking for could not be found." end end + + describe "related records" do + it "serializes the enterprise relationship" do + expected_enterprise_data = { + "data" => { + "id" => customer1.enterprise_id.to_s, + "type" => "enterprise" + } + } + + get "/api/v1/customers/#{customer1.id}" + expect(json_response[:data][:relationships][:enterprise]).to eq(expected_enterprise_data) + end + end end put "Update customer" do From 56bc554f29fa364b45253cfc7b80933133e8058c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 11:30:13 +0100 Subject: [PATCH 09/22] Regenerate swagger docs with `rails rswag` --- swagger/v1/swagger.yaml | 601 +++++++++++++++++++++++++++++++++++----- 1 file changed, 528 insertions(+), 73 deletions(-) diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index d341f1807d..de4d0e1186 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -3,79 +3,146 @@ openapi: 3.0.1 info: title: API V1 version: v1 +components: + schemas: + error_response: + type: object + properties: + errors: + type: array + items: + type: object + properties: + title: + type: string + detail: + type: string + source: + type: object + required: + - detail + required: + - errors + customer: + type: object + properties: + data: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - enterprise_id + - email + relationships: + type: object + meta: + type: object + required: + - data paths: - "/api/v0/orders": + "/api/v1/customers": get: - summary: list orders + summary: List customers tags: - - Orders + - Customers parameters: - - name: X-Spree-Token - in: header - schema: - type: string - - name: q[distributor_id_eq] + - name: enterprise_id in: query - required: false - description: Query orders for a specific distributor id. - schema: - type: string - - name: q[completed_at_gt] - in: query - required: false - description: Query orders completed after a date. - schema: - type: string - - name: q[completed_at_lt] - in: query - required: false - description: Query orders completed before a date. - schema: - type: string - - name: q[state_eq] - in: query - required: false - description: Query orders by order state, eg 'cart', 'complete'. - schema: - type: string - - name: q[payment_state_eq] - in: query - required: false - description: Query orders by order payment_state, eg 'balance_due', 'paid', - 'failed'. - schema: - type: string - - name: q[email_cont] - in: query - required: false - description: Query orders where the order email contains a string. - schema: - type: string - - name: q[order_cycle_id_eq] - in: query - required: false - description: Query orders for a specific order_cycle id. schema: type: string responses: '200': - description: get orders + description: Customers list content: application/json: schema: - "$ref": "#/components/schemas/Order_Concise" - "/api/v1/customers/{id}": - get: - summary: Show customer - parameters: - - name: id - in: path - required: true - schema: - type: string + type: object + properties: + data: + type: array + items: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + meta: + type: object + required: + - data + post: + summary: Create customer + tags: + - Customers + parameters: [] responses: - '200': - description: customer found + '201': + description: Customer created content: application/json: schema: @@ -86,42 +153,430 @@ paths: properties: id: type: string + example: '1' type: type: string + example: customer attributes: type: object properties: id: type: integer - name: + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: type: string nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs code: type: string + nullable: true + example: BUYER1 email: type: string + example: alice@example.com required: - id - - name + - enterprise_id + - first_name + - last_name - code - email - required: - - id - - type - - attributes - - relationships - relationships: - - enterprise + relationships: + type: object + meta: + type: object required: - data - '404': - description: not found + '422': + description: Unprocessable entity content: application/json: schema: type: object properties: - error: - type: string + errors: + type: array + items: + type: object + properties: + title: + type: string + detail: + type: string + source: + type: object + required: + - detail + required: + - errors + requestBody: + content: + application/json: + schema: + type: object + properties: + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - enterprise_id + - email + "/api/v1/customers/{id}": + get: + summary: Show customer + tags: + - Customers + parameters: + - name: id + in: path + required: true + schema: + type: string + responses: + '200': + description: Customer + content: + application/json: + schema: + type: object + properties: + data: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + meta: + type: object + required: + - data + '404': + description: Not found + content: + application/json: + schema: + type: object + properties: + errors: + type: array + items: + type: object + properties: + title: + type: string + detail: + type: string + source: + type: object + required: + - detail + required: + - errors + put: + summary: Update customer + tags: + - Customers + parameters: + - name: id + in: path + required: true + schema: + type: string + responses: + '200': + description: Customer updated + content: + application/json: + schema: + type: object + properties: + data: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + meta: + type: object + required: + - data + '422': + description: Unprocessable entity + content: + application/json: + schema: + type: object + properties: + errors: + type: array + items: + type: object + properties: + title: + type: string + detail: + type: string + source: + type: object + required: + - detail + required: + - errors + requestBody: + content: + application/json: + schema: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - enterprise_id + - email + delete: + summary: Delete customer + tags: + - Customers + parameters: + - name: id + in: path + required: true + schema: + type: string + responses: + '200': + description: Customer deleted + content: + application/json: + schema: + type: object + properties: + data: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + meta: + type: object + required: + - data + "/api/v1/enterprises/{enterprise_id}/customers": + get: + summary: List customers of an enterprise + tags: + - Customers + - Enterprises + parameters: + - name: enterprise_id + in: path + required: true + schema: + type: string + responses: + '200': + description: Customers list + content: + application/json: + schema: + type: object + properties: + data: + type: array + items: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + meta: + type: object + required: + - data servers: - url: "/" From a222b507fb3ace07aef7ea761e34d82a25188cf1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 13:59:33 +0100 Subject: [PATCH 10/22] Update and document authentication methods --- app/controllers/api/v1/base_controller.rb | 13 ++++--------- spec/swagger_helper.rb | 20 ++++++++++++++++++++ swagger/v1/swagger.yaml | 16 ++++++++++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 12f7f8748e..1ed61ecd6c 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -16,21 +16,16 @@ module Api private - def spree_current_user - @spree_current_user ||= request.env['warden'].user - end - - # Use logged in user (spree_current_user) for API authentication (current_api_user) def authenticate_user - return if (@current_api_user = spree_current_user) + return if (@current_api_user = request.env['warden'].user) if api_key.blank? # An anonymous user - @current_api_user = Spree.user_class.new + @current_api_user = Spree::User.new return end - return if (@current_api_user = Spree.user_class.find_by(spree_api_key: api_key.to_s)) + return if (@current_api_user = Spree::User.find_by(spree_api_key: api_key.to_s)) invalid_api_key end @@ -40,7 +35,7 @@ module Api end def api_key - request.headers["X-Spree-Token"] || params[:token] + request.headers["X-Api-Token"] || params[:token] end def error_during_processing(exception) diff --git a/spec/swagger_helper.rb b/spec/swagger_helper.rb index 9bcd2c4064..d8d5094673 100644 --- a/spec/swagger_helper.rb +++ b/spec/swagger_helper.rb @@ -28,6 +28,26 @@ RSpec.configure do |config| schemas: { error_response: ErrorsSchema.schema, customer: CustomerSchema.schema + }, + securitySchemas: { + api_key_header: { + type: :apiKey, + name: 'X-Api-Token', + in: :header, + description: "Authenticates via API key passed in specified header" + }, + api_key_param: { + type: :apiKey, + name: 'token', + in: :query, + description: "Authenticates via API key passed in specified query param" + }, + session: { + type: :http, + name: '_ofn_session', + in: :cookie, + description: "Authenticates using the current user's session if logged in" + }, } }, paths: {}, diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index de4d0e1186..7fade2d482 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -68,6 +68,22 @@ components: type: object required: - data + securitySchemas: + api_key_header: + type: apiKey + name: X-Api-Token + in: header + description: Authenticates via API key passed in specified header + api_key_param: + type: apiKey + name: token + in: query + description: Authenticates via API key passed in specified query param + session: + type: http + name: _ofn_session + in: cookie + description: Authenticates using the current user's session if logged in paths: "/api/v1/customers": get: From 8e61428cce81b20a942bd3e43ad2826ed703b8c9 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 15:59:20 +0100 Subject: [PATCH 11/22] Add `401 Unauthorized` response test --- spec/requests/api/v1/customers_spec.rb | 13 +++++++++++++ swagger/v1/swagger.yaml | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 5b9cab952b..9141a49c1e 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -109,6 +109,19 @@ describe "Customers", type: :request do end end + context "without authentication" do + before { logout } + + response "401", "Unauthorized" do + param(:id) { customer1.id } + schema ErrorsSchema.schema + + run_test! do + expect(json_error_detail).to eq "You are not authorized to perform that action." + end + end + end + describe "related records" do it "serializes the enterprise relationship" do expected_enterprise_data = { diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 7fade2d482..2019f25b2e 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -346,6 +346,28 @@ paths: - detail required: - errors + '401': + description: Unauthorized + content: + application/json: + schema: + type: object + properties: + errors: + type: array + items: + type: object + properties: + title: + type: string + detail: + type: string + source: + type: object + required: + - detail + required: + - errors put: summary: Update customer tags: From c102ce8e7e6c1c16238a452a07a9a0d769944285 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 17:42:52 +0100 Subject: [PATCH 12/22] Add RequestTimeout concern --- app/controllers/api/v1/base_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 1ed61ecd6c..d317d97b50 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -4,6 +4,8 @@ module Api module V1 class BaseController < ActionController::API include CanCan::ControllerAdditions + include RequestTimeouts + check_authorization attr_accessor :current_api_user From 3dbf00f302c692e38385c9f0ed80efb8576ee472 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 11 Oct 2021 19:47:28 +0100 Subject: [PATCH 13/22] Add pagination with Pagy using structure from JSON:API specification https://jsonapi.org/examples/#pagination Update schema for collections rendered with pagination data --- .rubocop_todo.yml | 1 + app/controllers/api/v1/base_controller.rb | 2 + .../api/v1/customers_controller.rb | 4 +- .../concerns/json_api_pagination.rb | 74 +++++++++++++++++ app/json_schemas/json_api_schema.rb | 31 ++++++- swagger/v1/swagger.yaml | 80 +++++++++++++++++++ 6 files changed, 187 insertions(+), 5 deletions(-) create mode 100644 app/controllers/concerns/json_api_pagination.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 1ba490f5ec..8a58f51f78 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -582,6 +582,7 @@ Metrics/MethodLength: - 'app/controllers/spree/orders_controller.rb' - 'app/helpers/checkout_helper.rb' - 'app/helpers/spree/admin/navigation_helper.rb' + - "app/json_schemas/json_api_schema.rb" - 'app/models/spree/ability.rb' - 'app/models/spree/gateway/pay_pal_express.rb' - 'app/models/spree/order/checkout.rb' diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index d317d97b50..c3884f668e 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -5,6 +5,8 @@ module Api class BaseController < ActionController::API include CanCan::ControllerAdditions include RequestTimeouts + include Pagy::Backend + include JsonApiPagination check_authorization diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 0d9da27542..65783afc70 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -11,9 +11,9 @@ module Api before_action :authorize_action, only: [:show, :update, :destroy] def index - customers = search_customers + @pagy, customers = pagy(search_customers, pagy_options) - render json: Api::V1::CustomerSerializer.new(customers, is_collection: true) + render json: Api::V1::CustomerSerializer.new(customers, pagination_options) end def show diff --git a/app/controllers/concerns/json_api_pagination.rb b/app/controllers/concerns/json_api_pagination.rb new file mode 100644 index 0000000000..1bf53351d4 --- /dev/null +++ b/app/controllers/concerns/json_api_pagination.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +module JsonApiPagination + extend ActiveSupport::Concern + + DEFAULT_PER_PAGE = 50 + MAX_PER_PAGE = 200 + + def pagination_options + { + is_collection: true, + meta: meta_options, + links: links_options, + } + end + + def pagy_options + { items: final_per_page_value } + end + + private + + def meta_options + { + pagination: { + results: @pagy.count, + pages: total_pages, + page: current_page, + per_page: final_per_page_value + } + } + end + + def links_options + { + self: pagination_url(current_page), + first: pagination_url(1), + prev: pagination_url(previous_page), + next: pagination_url(next_page), + last: pagination_url(total_pages) + } + end + + def pagination_url(page_number) + return if page_number.nil? + + url_for(only_path: false, params: request.query_parameters.merge(page: page_number)) + end + + # User-specified value, or DEFAULT_PER_PAGE, capped at MAX_PER_PAGE + def final_per_page_value + (params[:per_page] || DEFAULT_PER_PAGE).to_i.clamp(1, MAX_PER_PAGE) + end + + def current_page + params[:page] || 1 + end + + def total_pages + @pagy.pages + end + + def previous_page + return nil if current_page < 2 + + current_page - 1 + end + + def next_page + return nil if current_page >= total_pages + + current_page + 1 + end +end diff --git a/app/json_schemas/json_api_schema.rb b/app/json_schemas/json_api_schema.rb index 1bc13098ef..db12a1c425 100644 --- a/app/json_schemas/json_api_schema.rb +++ b/app/json_schemas/json_api_schema.rb @@ -22,7 +22,8 @@ class JsonApiSchema type: :object, properties: data_properties(**options) }, - meta: { type: :object } + meta: { type: :object }, + links: { type: :object } }, required: [:data] } @@ -39,9 +40,33 @@ class JsonApiSchema properties: data_properties(**options) } }, - meta: { type: :object } + meta: { + type: :object, + properties: { + pagination: { + type: :object, + properties: { + results: { type: :integer, example: 250 }, + pages: { type: :integer, example: 5 }, + page: { type: :integer, example: 2 }, + per_page: { type: :integer, example: 50 }, + } + } + }, + required: [:pagination] + }, + links: { + type: :object, + properties: { + self: { type: :string }, + first: { type: :string }, + prev: { type: :string, nullable: true }, + next: { type: :string, nullable: true }, + last: { type: :string } + } + } }, - required: [:data] + required: [:data, :meta, :links] } end diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 2019f25b2e..652fef8fb3 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -66,6 +66,8 @@ components: type: object meta: type: object + links: + type: object required: - data securitySchemas: @@ -149,8 +151,43 @@ paths: type: object meta: type: object + properties: + pagination: + type: object + properties: + results: + type: integer + example: 250 + pages: + type: integer + example: 5 + page: + type: integer + example: 2 + per_page: + type: integer + example: 50 + required: + - pagination + links: + type: object + properties: + self: + type: string + first: + type: string + prev: + type: string + nullable: true + next: + type: string + nullable: true + last: + type: string required: - data + - meta + - links post: summary: Create customer tags: @@ -208,6 +245,8 @@ paths: type: object meta: type: object + links: + type: object required: - data '422': @@ -322,6 +361,8 @@ paths: type: object meta: type: object + links: + type: object required: - data '404': @@ -430,6 +471,8 @@ paths: type: object meta: type: object + links: + type: object required: - data '422': @@ -546,6 +589,8 @@ paths: type: object meta: type: object + links: + type: object required: - data "/api/v1/enterprises/{enterprise_id}/customers": @@ -614,7 +659,42 @@ paths: type: object meta: type: object + properties: + pagination: + type: object + properties: + results: + type: integer + example: 250 + pages: + type: integer + example: 5 + page: + type: integer + example: 2 + per_page: + type: integer + example: 50 + required: + - pagination + links: + type: object + properties: + self: + type: string + first: + type: string + prev: + type: string + nullable: true + next: + type: string + nullable: true + last: + type: string required: - data + - meta + - links servers: - url: "/" From d66d6d6bd6feb11b30d7cbf7236a37a2ce9c388f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 12 Dec 2021 19:13:16 +0000 Subject: [PATCH 14/22] Update use of links on relationships --- app/serializers/api/v1/base_serializer.rb | 13 +++++++++++++ app/serializers/api/v1/customer_serializer.rb | 10 ++++++---- app/serializers/api/v1/enterprise_serializer.rb | 15 +++++++++++++++ spec/requests/api/v1/customers_spec.rb | 3 +++ 4 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 app/serializers/api/v1/base_serializer.rb create mode 100644 app/serializers/api/v1/enterprise_serializer.rb diff --git a/app/serializers/api/v1/base_serializer.rb b/app/serializers/api/v1/base_serializer.rb new file mode 100644 index 0000000000..4689406779 --- /dev/null +++ b/app/serializers/api/v1/base_serializer.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Api + module V1 + class BaseSerializer + include JSONAPI::Serializer + + def self.url_helpers + Rails.application.routes.url_helpers + end + end + end +end diff --git a/app/serializers/api/v1/customer_serializer.rb b/app/serializers/api/v1/customer_serializer.rb index cea4d53d2e..6f71b39159 100644 --- a/app/serializers/api/v1/customer_serializer.rb +++ b/app/serializers/api/v1/customer_serializer.rb @@ -2,12 +2,14 @@ module Api module V1 - class CustomerSerializer - include JSONAPI::Serializer - + class CustomerSerializer < BaseSerializer attributes :id, :enterprise_id, :first_name, :last_name, :code, :email - belongs_to :enterprise, record_type: :enterprise, serializer: :id + belongs_to :enterprise, links: { + related: ->(object) { + url_helpers.api_v1_enterprise_url(id: object.enterprise_id) + } + } end end end diff --git a/app/serializers/api/v1/enterprise_serializer.rb b/app/serializers/api/v1/enterprise_serializer.rb new file mode 100644 index 0000000000..abbb9cdd3c --- /dev/null +++ b/app/serializers/api/v1/enterprise_serializer.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Api + module V1 + class EnterpriseSerializer < BaseSerializer + attributes :id, :name + + has_many :customers, links: { + related: ->(object) { + url_helpers.api_v1_enterprise_customers_url(enterprise_id: object.id) + } + } + end + end +end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 9141a49c1e..58c9f7e60a 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -128,6 +128,9 @@ describe "Customers", type: :request do "data" => { "id" => customer1.enterprise_id.to_s, "type" => "enterprise" + }, + "links" => { + "related" => "http://test.host/api/v1/enterprises/#{customer1.enterprise_id}" } } From d87e1805afc6543bf8b549e93fe23f5c318f0621 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 12 Dec 2021 20:51:39 +0000 Subject: [PATCH 15/22] Add relationships to resource serializers --- app/json_schemas/customer_schema.rb | 4 ++++ app/json_schemas/json_api_schema.rb | 11 ++++++++++- app/json_schemas/relationship_schema.rb | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 app/json_schemas/relationship_schema.rb diff --git a/app/json_schemas/customer_schema.rb b/app/json_schemas/customer_schema.rb index 5e7cb94e21..4652ae3bbf 100644 --- a/app/json_schemas/customer_schema.rb +++ b/app/json_schemas/customer_schema.rb @@ -19,4 +19,8 @@ class CustomerSchema < JsonApiSchema def self.required_attributes [:enterprise_id, :email] end + + def self.relationships + [:enterprise] + end end diff --git a/app/json_schemas/json_api_schema.rb b/app/json_schemas/json_api_schema.rb index db12a1c425..9551becccc 100644 --- a/app/json_schemas/json_api_schema.rb +++ b/app/json_schemas/json_api_schema.rb @@ -10,6 +10,10 @@ class JsonApiSchema [] end + def relationships + [] + end + def all_attributes attributes.keys end @@ -83,7 +87,12 @@ class JsonApiSchema properties: attributes, required: required }, - relationships: { type: :object } + relationships: { + type: :object, + properties: relationships.to_h do |name| + [name, { "$ref" => "#/components/schemas/relationship" }] + end + } } end end diff --git a/app/json_schemas/relationship_schema.rb b/app/json_schemas/relationship_schema.rb new file mode 100644 index 0000000000..5b31dcf630 --- /dev/null +++ b/app/json_schemas/relationship_schema.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class RelationshipSchema + def self.schema + { + type: :object, + properties: { + data: { + type: [:object, :array] + }, + links: { + type: :object, + properties: { + related: { type: :string } + } + } + } + } + end +end From 2e59812bc1570aff33db9c6373719f95cedb8c8d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 12 Dec 2021 20:56:38 +0000 Subject: [PATCH 16/22] Refactor to make more use of schema refs, and regenerate swagger file --- spec/requests/api/v1/customers_spec.rb | 20 +- spec/swagger_helper.rb | 5 +- swagger/v1/swagger.yaml | 629 ++++++------------------- 3 files changed, 163 insertions(+), 491 deletions(-) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 58c9f7e60a..b84416cf04 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -19,7 +19,7 @@ describe "Customers", type: :request do response "200", "Customers list" do param(:enterprise_id) { enterprise1.id } - schema CustomerSchema.collection(require_all: true) + schema "$ref": "#/components/schemas/resources/customers_collection" run_test! end @@ -73,14 +73,14 @@ describe "Customers", type: :request do enterprise_id: enterprise1.id.to_s } end - schema CustomerSchema.schema(require_all: true) + schema "$ref": "#/components/schemas/resources/customer" run_test! end response "422", "Unprocessable entity" do param(:customer) { {} } - schema ErrorsSchema.schema + schema "$ref": "#/components/schemas/error_response" run_test! end @@ -95,14 +95,14 @@ describe "Customers", type: :request do response "200", "Customer" do param(:id) { customer1.id } - schema CustomerSchema.schema(require_all: true) + schema "$ref": "#/components/schemas/resources/customer" run_test! end response "404", "Not found" do param(:id) { 0 } - schema ErrorsSchema.schema + schema "$ref": "#/components/schemas/error_response" run_test! do expect(json_error_detail).to eq "The resource you were looking for could not be found." @@ -114,7 +114,7 @@ describe "Customers", type: :request do response "401", "Unauthorized" do param(:id) { customer1.id } - schema ErrorsSchema.schema + schema "$ref": "#/components/schemas/error_response" run_test! do expect(json_error_detail).to eq "You are not authorized to perform that action." @@ -161,7 +161,7 @@ describe "Customers", type: :request do enterprise_id: enterprise1.id.to_s } end - schema CustomerSchema.schema(require_all: true) + schema "$ref": "#/components/schemas/resources/customer" run_test! end @@ -169,7 +169,7 @@ describe "Customers", type: :request do response "422", "Unprocessable entity" do param(:id) { customer1.id } param(:customer) { {} } - schema ErrorsSchema.schema + schema "$ref": "#/components/schemas/error_response" run_test! end @@ -182,7 +182,7 @@ describe "Customers", type: :request do response "200", "Customer deleted" do param(:id) { customer1.id } - schema CustomerSchema.schema(require_all: true) + schema "$ref": "#/components/schemas/resources/customer" run_test! end @@ -197,7 +197,7 @@ describe "Customers", type: :request do response "200", "Customers list" do param(:enterprise_id) { enterprise1.id } - schema CustomerSchema.collection(require_all: true) + schema "$ref": "#/components/schemas/resources/customers_collection" run_test! end diff --git a/spec/swagger_helper.rb b/spec/swagger_helper.rb index d8d5094673..46e7a7760d 100644 --- a/spec/swagger_helper.rb +++ b/spec/swagger_helper.rb @@ -27,7 +27,10 @@ RSpec.configure do |config| components: { schemas: { error_response: ErrorsSchema.schema, - customer: CustomerSchema.schema + resources: { + customer: CustomerSchema.schema(require_all: true), + customers_collection: CustomerSchema.collection(require_all: true) + } }, securitySchemas: { api_key_header: { diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 652fef8fb3..b18bed201d 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -23,53 +23,150 @@ components: - detail required: - errors - customer: - type: object - properties: - data: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: + resources: + customer: + type: object + properties: + data: + type: object + properties: + id: + type: string + example: '1' + type: + type: string + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + properties: + enterprise: + "$ref": "#/components/schemas/relationship" + meta: + type: object + links: + type: object + required: + - data + customers_collection: + type: object + properties: + data: + type: array + items: type: object properties: id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: type: string - nullable: true - example: Alice - last_name: + example: '1' + type: type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - enterprise_id - - email - relationships: - type: object - meta: - type: object - links: - type: object - required: - - data + example: customer + attributes: + type: object + properties: + id: + type: integer + example: 1 + enterprise_id: + type: integer + example: 2 + first_name: + type: string + nullable: true + example: Alice + last_name: + type: string + nullable: true + example: Springs + code: + type: string + nullable: true + example: BUYER1 + email: + type: string + example: alice@example.com + required: + - id + - enterprise_id + - first_name + - last_name + - code + - email + relationships: + type: object + properties: + enterprise: + "$ref": "#/components/schemas/relationship" + meta: + type: object + properties: + pagination: + type: object + properties: + results: + type: integer + example: 250 + pages: + type: integer + example: 5 + page: + type: integer + example: 2 + per_page: + type: integer + example: 50 + required: + - pagination + links: + type: object + properties: + self: + type: string + first: + type: string + prev: + type: string + nullable: true + next: + type: string + nullable: true + last: + type: string + required: + - data + - meta + - links securitySchemas: api_key_header: type: apiKey @@ -103,91 +200,7 @@ paths: content: application/json: schema: - type: object - properties: - data: - type: array - items: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: - type: object - properties: - id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: - type: string - nullable: true - example: Alice - last_name: - type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - id - - enterprise_id - - first_name - - last_name - - code - - email - relationships: - type: object - meta: - type: object - properties: - pagination: - type: object - properties: - results: - type: integer - example: 250 - pages: - type: integer - example: 5 - page: - type: integer - example: 2 - per_page: - type: integer - example: 50 - required: - - pagination - links: - type: object - properties: - self: - type: string - first: - type: string - prev: - type: string - nullable: true - next: - type: string - nullable: true - last: - type: string - required: - - data - - meta - - links + "$ref": "#/components/schemas/resources/customers_collection" post: summary: Create customer tags: @@ -199,78 +212,13 @@ paths: content: application/json: schema: - type: object - properties: - data: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: - type: object - properties: - id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: - type: string - nullable: true - example: Alice - last_name: - type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - id - - enterprise_id - - first_name - - last_name - - code - - email - relationships: - type: object - meta: - type: object - links: - type: object - required: - - data + "$ref": "#/components/schemas/resources/customer" '422': description: Unprocessable entity content: application/json: schema: - type: object - properties: - errors: - type: array - items: - type: object - properties: - title: - type: string - detail: - type: string - source: - type: object - required: - - detail - required: - - errors + "$ref": "#/components/schemas/error_response" requestBody: content: application/json: @@ -315,100 +263,19 @@ paths: content: application/json: schema: - type: object - properties: - data: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: - type: object - properties: - id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: - type: string - nullable: true - example: Alice - last_name: - type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - id - - enterprise_id - - first_name - - last_name - - code - - email - relationships: - type: object - meta: - type: object - links: - type: object - required: - - data + "$ref": "#/components/schemas/resources/customer" '404': description: Not found content: application/json: schema: - type: object - properties: - errors: - type: array - items: - type: object - properties: - title: - type: string - detail: - type: string - source: - type: object - required: - - detail - required: - - errors + "$ref": "#/components/schemas/error_response" '401': description: Unauthorized content: application/json: schema: - type: object - properties: - errors: - type: array - items: - type: object - properties: - title: - type: string - detail: - type: string - source: - type: object - required: - - detail - required: - - errors + "$ref": "#/components/schemas/error_response" put: summary: Update customer tags: @@ -425,78 +292,13 @@ paths: content: application/json: schema: - type: object - properties: - data: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: - type: object - properties: - id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: - type: string - nullable: true - example: Alice - last_name: - type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - id - - enterprise_id - - first_name - - last_name - - code - - email - relationships: - type: object - meta: - type: object - links: - type: object - required: - - data + "$ref": "#/components/schemas/resources/customer" '422': description: Unprocessable entity content: application/json: schema: - type: object - properties: - errors: - type: array - items: - type: object - properties: - title: - type: string - detail: - type: string - source: - type: object - required: - - detail - required: - - errors + "$ref": "#/components/schemas/error_response" requestBody: content: application/json: @@ -543,56 +345,7 @@ paths: content: application/json: schema: - type: object - properties: - data: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: - type: object - properties: - id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: - type: string - nullable: true - example: Alice - last_name: - type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - id - - enterprise_id - - first_name - - last_name - - code - - email - relationships: - type: object - meta: - type: object - links: - type: object - required: - - data + "$ref": "#/components/schemas/resources/customer" "/api/v1/enterprises/{enterprise_id}/customers": get: summary: List customers of an enterprise @@ -611,90 +364,6 @@ paths: content: application/json: schema: - type: object - properties: - data: - type: array - items: - type: object - properties: - id: - type: string - example: '1' - type: - type: string - example: customer - attributes: - type: object - properties: - id: - type: integer - example: 1 - enterprise_id: - type: integer - example: 2 - first_name: - type: string - nullable: true - example: Alice - last_name: - type: string - nullable: true - example: Springs - code: - type: string - nullable: true - example: BUYER1 - email: - type: string - example: alice@example.com - required: - - id - - enterprise_id - - first_name - - last_name - - code - - email - relationships: - type: object - meta: - type: object - properties: - pagination: - type: object - properties: - results: - type: integer - example: 250 - pages: - type: integer - example: 5 - page: - type: integer - example: 2 - per_page: - type: integer - example: 50 - required: - - pagination - links: - type: object - properties: - self: - type: string - first: - type: string - prev: - type: string - nullable: true - next: - type: string - nullable: true - last: - type: string - required: - - data - - meta - - links + "$ref": "#/components/schemas/resources/customers_collection" servers: - url: "/" From bd9bed7323f8fcd8e9d908775c5e0c95c6844cbb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 12 Dec 2021 22:41:46 +0000 Subject: [PATCH 17/22] Update schemas for relationships --- app/json_schemas/json_api_schema.rb | 17 ++++++++++++- app/json_schemas/relationship_schema.rb | 32 +++++++++++++++++++++++-- swagger/v1/swagger.yaml | 32 +++++++++++++++++++++++-- 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/app/json_schemas/json_api_schema.rb b/app/json_schemas/json_api_schema.rb index 9551becccc..069de6633d 100644 --- a/app/json_schemas/json_api_schema.rb +++ b/app/json_schemas/json_api_schema.rb @@ -90,10 +90,25 @@ class JsonApiSchema relationships: { type: :object, properties: relationships.to_h do |name| - [name, { "$ref" => "#/components/schemas/relationship" }] + [ + name, + relationship_schema(name) + ] end } } end + + def relationship_schema(name) + if is_singular?(name) + RelationshipSchema.schema(name) + else + RelationshipSchema.collection(name) + end + end + + def is_singular?(name) + name.to_s.singularize == name.to_s + end end end diff --git a/app/json_schemas/relationship_schema.rb b/app/json_schemas/relationship_schema.rb index 5b31dcf630..a223fd390f 100644 --- a/app/json_schemas/relationship_schema.rb +++ b/app/json_schemas/relationship_schema.rb @@ -1,12 +1,40 @@ # frozen_string_literal: true class RelationshipSchema - def self.schema + def self.schema(resource_name = nil) { type: :object, properties: { data: { - type: [:object, :array] + type: :object, + properties: { + id: { type: :string }, + type: { type: :string, example: resource_name } + } + }, + links: { + type: :object, + properties: { + related: { type: :string } + } + } + } + } + end + + def self.collection(resource_name = nil) + { + type: :object, + properties: { + data: { + type: :array, + items: { + type: :object, + properties: { + id: { type: :string }, + type: { type: :string, example: resource_name } + } + } }, links: { type: :object, diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index b18bed201d..9b2642959d 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -71,7 +71,21 @@ components: type: object properties: enterprise: - "$ref": "#/components/schemas/relationship" + type: object + properties: + data: + type: object + properties: + id: + type: string + type: + type: string + example: enterprise + links: + type: object + properties: + related: + type: string meta: type: object links: @@ -127,7 +141,21 @@ components: type: object properties: enterprise: - "$ref": "#/components/schemas/relationship" + type: object + properties: + data: + type: object + properties: + id: + type: string + type: + type: string + example: enterprise + links: + type: object + properties: + related: + type: string meta: type: object properties: From 414bf5d074b7ef4a47189cc314f627137e289ead Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 15 Feb 2022 12:41:35 +1100 Subject: [PATCH 18/22] Don't list guest customer records to guest users --- app/controllers/api/v1/customers_controller.rb | 2 +- spec/requests/api/v1/customers_spec.rb | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v1/customers_controller.rb b/app/controllers/api/v1/customers_controller.rb index 65783afc70..6c403ce2d4 100644 --- a/app/controllers/api/v1/customers_controller.rb +++ b/app/controllers/api/v1/customers_controller.rb @@ -64,7 +64,7 @@ module Api end def visible_customers - Customer.where(user_id: current_api_user.id).or( + current_api_user.customers.or( Customer.where(enterprise_id: editable_enterprises) ) end diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index b84416cf04..59c4de7ec2 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -26,6 +26,22 @@ describe "Customers", type: :request do end describe "returning results based on permissions" do + context "as guest user" do + before { login_as nil } + + it "returns no customers" do + get "/api/v1/customers" + expect(json_response_ids).to eq [] + end + + it "returns not even customers without user id" do + customer3.update!(user_id: nil) + + get "/api/v1/customers" + expect(json_response_ids).to eq [] + end + end + context "as an enterprise owner" do before { login_as enterprise1.owner } From 75fc35574ecfca1516814f335314ce0e5fb115b2 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 15 Feb 2022 13:03:55 +1100 Subject: [PATCH 19/22] Make json_response test helper deal with error response It raised an error: NoMethodError: undefined method `map' for nil:NilClass --- spec/requests/api/v1/customers_spec.rb | 9 +++++++++ spec/support/api_helper.rb | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 59c4de7ec2..de80b87901 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -71,6 +71,15 @@ describe "Customers", type: :request do end end + describe "pagination" do + it "renders the first page" do + pending "pagination logic compares string with integer" + + get "/api/v1/customers", params: { page: "1" } + expect(json_response_ids).to eq [customer1.id.to_s, customer2.id.to_s] + end + end + post "Create customer" do tags "Customers" consumes "application/json" diff --git a/spec/support/api_helper.rb b/spec/support/api_helper.rb index d4591ece3d..37d1d6a4d1 100644 --- a/spec/support/api_helper.rb +++ b/spec/support/api_helper.rb @@ -15,7 +15,7 @@ module OpenFoodNetwork end def json_response_ids - json_response[:data].map{ |item| item["id"] } + json_response[:data]&.map{ |item| item["id"] } end def json_error_detail From b89715149cdf81eaa2ab169347571c19e5d7692b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 15 Feb 2022 13:17:31 +1100 Subject: [PATCH 20/22] Sanitise pagination input for new API --- app/controllers/concerns/json_api_pagination.rb | 2 +- spec/requests/api/v1/customers_spec.rb | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/json_api_pagination.rb b/app/controllers/concerns/json_api_pagination.rb index 1bf53351d4..b014e3119b 100644 --- a/app/controllers/concerns/json_api_pagination.rb +++ b/app/controllers/concerns/json_api_pagination.rb @@ -53,7 +53,7 @@ module JsonApiPagination end def current_page - params[:page] || 1 + (params[:page] || 1).to_i end def total_pages diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index de80b87901..3884d09354 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -73,11 +73,14 @@ describe "Customers", type: :request do describe "pagination" do it "renders the first page" do - pending "pagination logic compares string with integer" - get "/api/v1/customers", params: { page: "1" } expect(json_response_ids).to eq [customer1.id.to_s, customer2.id.to_s] end + + it "renders the second page" do + get "/api/v1/customers", params: { page: "2", per_page: "1" } + expect(json_response_ids).to eq [customer2.id.to_s] + end end post "Create customer" do From 4aa70c1ffd5ee2577479a5b39afe2babf5d1fd83 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 15 Feb 2022 15:49:47 +1100 Subject: [PATCH 21/22] Render pagination errors --- app/controllers/api/v1/base_controller.rb | 6 ++++++ spec/requests/api/v1/customers_spec.rb | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index c3884f668e..60307ba6c7 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -17,6 +17,7 @@ module Api rescue_from Exception, with: :error_during_processing rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from ActiveRecord::RecordNotFound, with: :not_found + rescue_from Pagy::VariableError, with: :invalid_pagination private @@ -49,6 +50,11 @@ module Api json: json_api_error(exception.message, backtrace: exception.backtrace) end + def invalid_pagination(exception) + render status: :unprocessable_entity, + json: json_api_error(exception.message) + end + def invalid_resource!(resource = nil) render status: :unprocessable_entity, json: json_api_invalid( diff --git a/spec/requests/api/v1/customers_spec.rb b/spec/requests/api/v1/customers_spec.rb index 3884d09354..7c8519a4b0 100644 --- a/spec/requests/api/v1/customers_spec.rb +++ b/spec/requests/api/v1/customers_spec.rb @@ -81,6 +81,17 @@ describe "Customers", type: :request do get "/api/v1/customers", params: { page: "2", per_page: "1" } expect(json_response_ids).to eq [customer2.id.to_s] end + + it "renders beyond the available pages" do + get "/api/v1/customers", params: { page: "2" } + expect(json_response_ids).to eq [] + end + + it "informs about invalid pages" do + get "/api/v1/customers", params: { page: "0" } + expect(json_response_ids).to eq nil + expect(json_error_detail).to eq 'expected :page >= 1; got "0"' + end end post "Create customer" do From 12d989568ed142175770e2cab85eaf6477bf7a56 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 15 Feb 2022 16:09:01 +1100 Subject: [PATCH 22/22] Safer API error reporting We don't know what unknown errors would report. They could expose sensitive data. So let's not pass that data on to the public while we have the full details in Bugsnag. Also, let's not catch Exception because that could catch interrupts to gracefully shut down the application. --- app/controllers/api/v1/base_controller.rb | 17 +++++++++-------- config/locales/en.yml | 1 + 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 60307ba6c7..aca1c09e55 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -14,7 +14,7 @@ module Api before_action :authenticate_user - rescue_from Exception, with: :error_during_processing + rescue_from StandardError, with: :error_during_processing rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from Pagy::VariableError, with: :invalid_pagination @@ -46,8 +46,13 @@ module Api def error_during_processing(exception) Bugsnag.notify(exception) - render status: :unprocessable_entity, - json: json_api_error(exception.message, backtrace: exception.backtrace) + if Rails.env.development? || Rails.env.test? + render status: :unprocessable_entity, + json: json_api_error(exception.message, meta: exception.backtrace) + else + render status: :unprocessable_entity, + json: json_api_error(I18n.t(:unknown_error, scope: "api")) + end end def invalid_pagination(exception) @@ -79,11 +84,7 @@ module Api end def json_api_error(message, **options) - error_response = { errors: [{ detail: message }] } - if options[:backtrace] && (Rails.env.development? || Rails.env.test?) - error_response.merge!(meta: [options[:backtrace]]) - end - error_response + { errors: [{ detail: message }] }.merge(options) end def json_api_invalid(message, errors) diff --git a/config/locales/en.yml b/config/locales/en.yml index 95fcaecaec..a20297cbf8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1409,6 +1409,7 @@ en: # API # api: + unknown_error: "Something went wrong. Our team has been notified." invalid_api_key: "Invalid API key (%{key}) specified." unauthorized: "You are not authorized to perform that action." invalid_resource: "Invalid resource. Please fix errors and try again."