From 2a09f1d505aa2106f44a4984a91d33f594641b2c Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Tue, 18 Apr 2023 09:31:17 +0200 Subject: [PATCH 1/4] fix(graphql): return empty arrays from loaders when loading collections --- app/graphql/loaders/champ.rb | 2 +- app/graphql/loaders/record.rb | 2 +- spec/graphql/dossier_spec.rb | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/app/graphql/loaders/champ.rb b/app/graphql/loaders/champ.rb index b33672b10..5f5bee562 100644 --- a/app/graphql/loaders/champ.rb +++ b/app/graphql/loaders/champ.rb @@ -13,7 +13,7 @@ module Loaders def perform(keys) query(keys).each { |record| fulfill(record.stable_id, [record].compact) } - keys.each { |key| fulfill(key, nil) unless fulfilled?(key) } + keys.each { |key| fulfill(key, []) unless fulfilled?(key) } end private diff --git a/app/graphql/loaders/record.rb b/app/graphql/loaders/record.rb index 821a9f1cd..4d6b94fa1 100644 --- a/app/graphql/loaders/record.rb +++ b/app/graphql/loaders/record.rb @@ -21,7 +21,7 @@ module Loaders fulfilled_value = @array ? [record].compact : record fulfill(record.public_send(@column), fulfilled_value) end - keys.each { |key| fulfill(key, nil) unless fulfilled?(key) } + keys.each { |key| fulfill(key, @array ? [] : nil) unless fulfilled?(key) } end private diff --git a/spec/graphql/dossier_spec.rb b/spec/graphql/dossier_spec.rb index 105bcf91c..6e8e0f723 100644 --- a/spec/graphql/dossier_spec.rb +++ b/spec/graphql/dossier_spec.rb @@ -75,6 +75,27 @@ RSpec.describe Types::DossierType, type: :graphql do end end + describe 'dossier with selected champ' do + let(:procedure) { create(:procedure, types_de_champ_public: [{ libelle: 'yolo' }, { libelle: 'toto' }]) } + let(:dossier) { create(:dossier, :en_construction, :with_populated_champs, procedure:) } + let(:query) { DOSSIER_WITH_SELECTED_CHAMP_QUERY } + let(:variables) { { number: dossier.id, id: champ.to_typed_id } } + let(:champ) { dossier.champs_public.last } + + context 'when champ exists' do + it { + expect(data[:dossier][:champs].size).to eq 1 + expect(data[:dossier][:champs][0][:label]).to eq "toto" + } + end + + context "when champ dosen't exists" do + let(:variables) { { number: dossier.id, id: '1234' } } + + it { expect(data[:dossier][:champs].size).to eq 0 } + end + end + describe 'dossier with conditional champs' do include Logic let(:stable_id) { 1234 } @@ -390,4 +411,15 @@ RSpec.describe Types::DossierType, type: :graphql do } } GRAPHQL + + DOSSIER_WITH_SELECTED_CHAMP_QUERY = <<-GRAPHQL + query($number: Int!, $id: ID!) { + dossier(number: $number) { + champs(id: $id) { + id + label + } + } + } + GRAPHQL end From fbae6d941d76f3f315d3f13c7212ac7eb2f73fdc Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Tue, 18 Apr 2023 09:48:38 +0200 Subject: [PATCH 2/4] feat(graphql): add error codes to graphql errors --- app/controllers/api/v2/graphql_controller.rb | 36 ++++++++++++++----- app/graphql/api/v2/schema.rb | 2 +- app/graphql/api/v2/stored_query.rb | 2 +- app/graphql/types/base_object.rb | 6 ++++ .../api/v2/graphql_controller_spec.rb | 2 +- 5 files changed, 37 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/v2/graphql_controller.rb b/app/controllers/api/v2/graphql_controller.rb index ce1b7001b..6d858ab10 100644 --- a/app/controllers/api/v2/graphql_controller.rb +++ b/app/controllers/api/v2/graphql_controller.rb @@ -5,7 +5,9 @@ class API::V2::GraphqlController < API::V2::BaseController render json: result rescue GraphQL::ParseError, JSON::ParserError => exception - handle_parse_error(exception) + handle_parse_error(exception, :graphql_parse_failed) + rescue ArgumentError => exception + handle_parse_error(exception, :bad_request) rescue => exception if Rails.env.production? handle_error_in_production(exception) @@ -33,7 +35,12 @@ class API::V2::GraphqlController < API::V2::BaseController rescue ActionDispatch::Http::Parameters::ParseError => exception render json: { errors: [ - { message: exception.cause.message } + { + message: exception.cause.message, + extensions: { + code: :bad_request + } + } ], data: nil }, status: 400 @@ -75,10 +82,13 @@ class API::V2::GraphqlController < API::V2::BaseController end end - def handle_parse_error(exception) + def handle_parse_error(exception, code) render json: { errors: [ - { message: exception.message } + { + message: exception.message, + extensions: { code: } + } ], data: nil }, status: 400 @@ -90,22 +100,32 @@ class API::V2::GraphqlController < API::V2::BaseController render json: { errors: [ - { message: exception.message, backtrace: exception.backtrace } + { + message: exception.message, + extensions: { + code: :internal_server_error, + backtrace: exception.backtrace + } + } ], data: nil }, status: 500 end def handle_error_in_production(exception) - extra = { exception_id: SecureRandom.uuid } - Sentry.capture_exception(exception, extra:) + exception_id = SecureRandom.uuid + Sentry.with_scope do |scope| + scope.set_tags(exception_id:) + Sentry.capture_exception(exception) + end render json: { errors: [ { message: "Internal Server Error", extensions: { - exception: { id: extra[:exception_id] } + code: :internal_server_error, + exception_id: } } ], diff --git a/app/graphql/api/v2/schema.rb b/app/graphql/api/v2/schema.rb index af9d21d54..1f418e377 100644 --- a/app/graphql/api/v2/schema.rb +++ b/app/graphql/api/v2/schema.rb @@ -50,7 +50,7 @@ class API::V2::Schema < GraphQL::Schema when GroupeInstructeur Types::GroupeInstructeurType else - raise GraphQL::ExecutionError.new("Unexpected object: #{object}") + type_definition end end diff --git a/app/graphql/api/v2/stored_query.rb b/app/graphql/api/v2/stored_query.rb index 255e62e3c..1f5effe5c 100644 --- a/app/graphql/api/v2/stored_query.rb +++ b/app/graphql/api/v2/stored_query.rb @@ -8,7 +8,7 @@ class API::V2::StoredQuery when 'introspection' GraphQL::Introspection::INTROSPECTION_QUERY else - raise GraphQL::ExecutionError.new("No query with id \"#{query_id}\"") + raise GraphQL::ExecutionError.new("No query with id \"#{query_id}\"", extensions: { code: :bad_request }) end end diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb index 2d20f1c05..88a6be02a 100644 --- a/app/graphql/types/base_object.rb +++ b/app/graphql/types/base_object.rb @@ -1,5 +1,11 @@ module Types class BaseObject < GraphQL::Schema::Object field_class BaseField + + class InvalidNullError < GraphQL::InvalidNullError + def to_h + super.merge(extensions: { code: :invalid_null }) + end + end end end diff --git a/spec/controllers/api/v2/graphql_controller_spec.rb b/spec/controllers/api/v2/graphql_controller_spec.rb index 869b2d39f..f91064be9 100644 --- a/spec/controllers/api/v2/graphql_controller_spec.rb +++ b/spec/controllers/api/v2/graphql_controller_spec.rb @@ -684,7 +684,7 @@ describe API::V2::GraphqlController do end it "should return an error" do - expect(gql_errors).to eq([{ message: "Cannot return null for non-nullable field PersonneMorale.siegeSocial" }]) + expect(gql_errors).to eq([{ message: "Cannot return null for non-nullable field PersonneMorale.siegeSocial", extensions: { code: "invalid_null" } }]) end end end From f70532a8442696ddb27b982e831e0f8b99fc10c0 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Tue, 18 Apr 2023 10:08:18 +0200 Subject: [PATCH 3/4] feat(graphql): global not found error handling --- app/graphql/api/v2/schema.rb | 6 ++-- app/graphql/types/query_type.rb | 8 +---- .../graphql_controller_stored_queries_spec.rb | 36 +++++++++++++++++++ 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/app/graphql/api/v2/schema.rb b/app/graphql/api/v2/schema.rb index 1f418e377..25779ff04 100644 --- a/app/graphql/api/v2/schema.rb +++ b/app/graphql/api/v2/schema.rb @@ -25,8 +25,6 @@ class API::V2::Schema < GraphQL::Schema def self.object_from_id(id, ctx) ApplicationRecord.record_from_typed_id(id) - rescue => e - raise GraphQL::ExecutionError.new(e.message, extensions: { code: :not_found }) end def self.resolve_type(type_definition, object, ctx) @@ -129,6 +127,10 @@ class API::V2::Schema < GraphQL::Schema super end + rescue_from(ActiveRecord::RecordNotFound) do |_error, _object, _args, _ctx, field| + raise GraphQL::ExecutionError.new("#{field.type.unwrap.graphql_name} not found", extensions: { code: :not_found }) + end + class Timeout < GraphQL::Schema::Timeout def handle_timeout(error, query) Sentry.capture_exception(error, extra: query.context.query_info) diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index 47b7c1b9f..044eae3a6 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -26,13 +26,11 @@ module Types demarche_number = demarche.number.presence || ApplicationRecord.id_from_typed_id(demarche.id) Procedure .includes(draft_revision: :procedure, published_revision: :procedure) - .find_by(id: demarche_number) + .find(demarche_number) end def demarche(number:) Procedure.for_api_v2.find(number) - rescue => e - raise GraphQL::ExecutionError.new(e.message, extensions: { code: :not_found }) end def dossier(number:) @@ -42,14 +40,10 @@ module Types Dossier.visible_by_administration.for_api_v2.find(number) end DossierPreloader.load_one(dossier) - rescue => e - raise GraphQL::ExecutionError.new(e.message, extensions: { code: :not_found }) end def groupe_instructeur(number:) GroupeInstructeur.for_api_v2.find(number) - rescue => e - raise GraphQL::ExecutionError.new(e.message, extensions: { code: :not_found }) end def self.accessible?(context) diff --git a/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb b/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb index 5dd0fcebb..7b790db02 100644 --- a/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb +++ b/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb @@ -76,6 +76,15 @@ describe API::V2::GraphqlController do expect(gql_data[:dossier][:demandeur][:prenom]).to eq(dossier.individual.prenom) } + context 'not found' do + let(:variables) { { dossierNumber: 0 } } + + it { + expect(gql_errors.first[:message]).to eq('Dossier not found') + expect(gql_errors.first[:extensions]).to eq({ code: 'not_found' }) + } + end + context 'with entreprise' do let(:procedure) { create(:procedure, :published, :with_service, administrateurs: [admin], types_de_champ_public:) } let(:dossier) { create(:dossier, :en_construction, :with_entreprise, procedure: procedure) } @@ -114,6 +123,15 @@ describe API::V2::GraphqlController do expect(gql_data[:demarche][:dossiers]).to be_nil } + context 'not found' do + let(:variables) { { demarcheNumber: 0 } } + + it { + expect(gql_errors.first[:message]).to eq('Demarche not found') + expect(gql_errors.first[:extensions]).to eq({ code: 'not_found' }) + } + end + context 'include Dossiers' do let(:variables) { { demarcheNumber: procedure.id, includeDossiers: true } } @@ -182,6 +200,15 @@ describe API::V2::GraphqlController do expect(gql_data[:groupeInstructeur][:dossiers]).to be_nil } + context 'not found' do + let(:variables) { { groupeInstructeurNumber: 0 } } + + it { + expect(gql_errors.first[:message]).to eq('GroupeInstructeurWithDossiers not found') + expect(gql_errors.first[:extensions]).to eq({ code: 'not_found' }) + } + end + context 'include Dossiers' do let(:variables) { { groupeInstructeurNumber: groupe_instructeur.id, includeDossiers: true } } @@ -207,6 +234,15 @@ describe API::V2::GraphqlController do } end + context 'not found' do + let(:variables) { { demarche: { number: 0 } } } + + it { + expect(gql_errors.first[:message]).to eq('DemarcheDescriptor not found') + expect(gql_errors.first[:extensions]).to eq({ code: 'not_found' }) + } + end + context 'find by id' do let(:variables) { { demarche: { id: procedure.to_typed_id } } } From ae1ec87397dfc32541d986554dc3bffa52ae9a46 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Tue, 18 Apr 2023 10:27:20 +0200 Subject: [PATCH 4/4] feat(graphql): add code to timeout errors --- app/graphql/api/v2/schema.rb | 1 + .../api/v2/graphql_controller_stored_queries_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/app/graphql/api/v2/schema.rb b/app/graphql/api/v2/schema.rb index 25779ff04..9e81aaa49 100644 --- a/app/graphql/api/v2/schema.rb +++ b/app/graphql/api/v2/schema.rb @@ -133,6 +133,7 @@ class API::V2::Schema < GraphQL::Schema class Timeout < GraphQL::Schema::Timeout def handle_timeout(error, query) + error.extensions = { code: :timeout } Sentry.capture_exception(error, extra: query.context.query_info) end end diff --git a/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb b/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb index 7b790db02..f28c8c393 100644 --- a/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb +++ b/spec/controllers/api/v2/graphql_controller_stored_queries_spec.rb @@ -64,6 +64,18 @@ describe API::V2::GraphqlController do } end + context 'timeout' do + let(:variables) { { dossierNumber: dossier.id } } + let(:operation_name) { 'getDossier' } + + before { allow_any_instance_of(API::V2::Schema::Timeout).to receive(:max_seconds).and_return(0) } + + it { + expect(gql_errors.first[:message]).to eq('Timeout on Query.dossier') + expect(gql_errors.first[:extensions]).to eq({ code: 'timeout' }) + } + end + context 'getDossier' do let(:variables) { { dossierNumber: dossier.id } } let(:operation_name) { 'getDossier' }