From 6dc4adb5a5d020a49b62c53c264bcd24de96637c Mon Sep 17 00:00:00 2001 From: Karol Date: Wed, 31 Jul 2024 20:55:00 +0200 Subject: [PATCH] refactor and split GptService --- app/controllers/tasks_controller.rb | 2 +- app/models/user.rb | 2 +- .../generate_tests.rb} | 39 +-------- app/services/gpt_service/gpt_service_base.rb | 23 +++++ app/services/gpt_service/validate_api_key.rb | 22 +++++ spec/controllers/tasks_controller_spec.rb | 8 +- spec/models/user_spec.rb | 11 +-- .../generate_tests_spec.rb} | 50 +++-------- .../gpt_service/validate_api_key_spec.rb | 83 +++++++++++++++++++ 9 files changed, 154 insertions(+), 86 deletions(-) rename app/services/{task_service/gpt_generate_tests.rb => gpt_service/generate_tests.rb} (63%) create mode 100644 app/services/gpt_service/gpt_service_base.rb create mode 100644 app/services/gpt_service/validate_api_key.rb rename spec/services/{task_service/gpt_generate_tests_spec.rb => gpt_service/generate_tests_spec.rb} (79%) create mode 100644 spec/services/gpt_service/validate_api_key_spec.rb diff --git a/app/controllers/tasks_controller.rb b/app/controllers/tasks_controller.rb index d66e5bea7..2a4c7315a 100644 --- a/app/controllers/tasks_controller.rb +++ b/app/controllers/tasks_controller.rb @@ -209,7 +209,7 @@ def export_external_confirm # rubocop:enable Metrics/AbcSize def generate_test - TaskService::GptGenerateTests.call(task: @task, openai_api_key: current_user.openai_api_key) + GptService::GenerateTests.call(task: @task, openai_api_key: current_user.openai_api_key) flash[:notice] = I18n.t('tasks.task_service.gpt_generate_tests.successful_generation') rescue Gpt::Error => e flash[:alert] = e.localized_message diff --git a/app/models/user.rb b/app/models/user.rb index 799ae3774..d7867f0f6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -152,7 +152,7 @@ def to_s def validate_openai_api_key return unless openai_api_key_changed? - TaskService::GptGenerateTests.new_client! openai_api_key + GptService::ValidateApiKey.call(openai_api_key:) rescue Gpt::Error::InvalidApiKey errors.add(:base, :invalid_api_key) end diff --git a/app/services/task_service/gpt_generate_tests.rb b/app/services/gpt_service/generate_tests.rb similarity index 63% rename from app/services/task_service/gpt_generate_tests.rb rename to app/services/gpt_service/generate_tests.rb index b5977cf17..fdd9e6316 100644 --- a/app/services/task_service/gpt_generate_tests.rb +++ b/app/services/gpt_service/generate_tests.rb @@ -1,14 +1,13 @@ # frozen_string_literal: true -module TaskService - class GptGenerateTests < ServiceBase +module GptService + class GenerateTests < GptServiceBase def initialize(task:, openai_api_key:) super() raise Gpt::Error::MissingLanguage if task.programming_language&.language.blank? @task = task - # We can skip validating here, since an invalid API key will raise during `execute`, too. - @client = self.class.new_client! openai_api_key, validate: false + @client = new_client! openai_api_key end def execute @@ -20,14 +19,6 @@ def execute @task.tests << test end - def self.new_client!(access_token, validate: true) - raise Gpt::Error::InvalidApiKey if access_token.blank? - - client = OpenAI::Client.new(access_token:) - validate! client if validate - client - end - private def gpt_response @@ -72,29 +63,5 @@ def training_prompts PROMPT ] end - - def wrap_api_error!(...) - # Use a custom forward for the private class method :wrap_api_error! and forward all arguments - self.class.send(:wrap_api_error!, ...) - end - - def self.validate!(client) - wrap_api_error! do - response = client.models.list - raise Gpt::Error::InvalidApiKey unless response['data'] - end - end - private_class_method :validate! - - def self.wrap_api_error! - yield - rescue Faraday::UnauthorizedError, OpenAI::Error => e - raise Gpt::Error::InvalidApiKey.new("Could not authenticate with OpenAI: #{e.message}") - rescue Faraday::Error => e - raise Gpt::Error::InternalServerError.new("Could not communicate with OpenAI: #{e.inspect}") - rescue Net::OpenTimeout, Net::ReadTimeout, Errno::ECONNRESET, SocketError, EOFError => e - raise Gpt::Error.new(e) - end - private_class_method :wrap_api_error! end end diff --git a/app/services/gpt_service/gpt_service_base.rb b/app/services/gpt_service/gpt_service_base.rb new file mode 100644 index 000000000..7d8a46ba5 --- /dev/null +++ b/app/services/gpt_service/gpt_service_base.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module GptService + class GptServiceBase < ServiceBase + def new_client!(access_token) + raise Gpt::Error::InvalidApiKey if access_token.blank? + + OpenAI::Client.new(access_token:) + end + + private + + def wrap_api_error! + yield + rescue Faraday::UnauthorizedError, OpenAI::Error => e + raise Gpt::Error::InvalidApiKey.new("Could not authenticate with OpenAI: #{e.message}") + rescue Faraday::Error => e + raise Gpt::Error::InternalServerError.new("Could not communicate with OpenAI: #{e.inspect}") + rescue Net::OpenTimeout, Net::ReadTimeout, Errno::ECONNRESET, SocketError, EOFError => e + raise Gpt::Error.new(e) + end + end +end diff --git a/app/services/gpt_service/validate_api_key.rb b/app/services/gpt_service/validate_api_key.rb new file mode 100644 index 000000000..c54697e69 --- /dev/null +++ b/app/services/gpt_service/validate_api_key.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module GptService + class ValidateApiKey < GptServiceBase + def initialize(openai_api_key:) + super() + + @client = new_client! openai_api_key + end + + def execute + validate! + end + + def validate! + wrap_api_error! do + response = @client.models.list + raise Gpt::Error::InvalidApiKey unless response['data'] + end + end + end +end diff --git a/spec/controllers/tasks_controller_spec.rb b/spec/controllers/tasks_controller_spec.rb index c623018f5..04d2cb051 100644 --- a/spec/controllers/tasks_controller_spec.rb +++ b/spec/controllers/tasks_controller_spec.rb @@ -1088,12 +1088,12 @@ context 'when GptGenerateTests is successful' do before do - allow(TaskService::GptGenerateTests).to receive(:call) + allow(GptService::GenerateTests).to receive(:call) post :generate_test, params: {id: task.id} end it 'calls the GptGenerateTests service with the correct parameters' do - expect(TaskService::GptGenerateTests).to have_received(:call).with(task:, openai_api_key: 'valid_api_key') + expect(GptService::GenerateTests).to have_received(:call).with(task:, openai_api_key: 'valid_api_key') end it 'redirects to the task show page' do @@ -1107,7 +1107,7 @@ context 'when GptGenerateTests raises Gpt::Error::MissingLanguage' do before do - allow(TaskService::GptGenerateTests).to receive(:call).and_raise(Gpt::Error::MissingLanguage) + allow(GptService::GenerateTests).to receive(:call).and_raise(Gpt::Error::MissingLanguage) post :generate_test, params: {id: task.id} end @@ -1122,7 +1122,7 @@ context 'when GptGenerateTests raises Gpt::Error::InvalidTaskDescription' do before do - allow(TaskService::GptGenerateTests).to receive(:call).and_raise(Gpt::Error::InvalidTaskDescription) + allow(GptService::GenerateTests).to receive(:call).and_raise(Gpt::Error::InvalidTaskDescription) post :generate_test, params: {id: task.id} end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fc7e012d7..2ab663aa5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -52,11 +52,10 @@ end context 'when openai_api_key is present and valid' do - let(:openai_client) { instance_double(OpenAI::Client) } let(:openai_api_key) { 'valid_key' } before do - allow(TaskService::GptGenerateTests).to receive(:new_client!).with(openai_api_key).and_return(openai_client) + allow(GptService::ValidateApiKey).to receive(:call).with(openai_api_key:) user.update(openai_api_key:) end @@ -66,11 +65,10 @@ end context 'when openai_api_key is present and invalid' do - let(:openai_client) { instance_double(OpenAI::Client) } let(:openai_api_key) { 'invalid_key' } before do - allow(TaskService::GptGenerateTests).to receive(:new_client!).with(openai_api_key).and_raise(Gpt::Error::InvalidApiKey) + allow(GptService::ValidateApiKey).to receive(:call).with(openai_api_key:).and_raise(Gpt::Error::InvalidApiKey) user.update(openai_api_key:) end @@ -85,16 +83,15 @@ end context 'when openai_api_key remains the same' do - let(:openai_client) { instance_double(OpenAI::Client) } let(:openai_api_key) { 'same_key' } before do - allow(TaskService::GptGenerateTests).to receive(:new_client!).with(openai_api_key).and_return(openai_client) + allow(GptService::ValidateApiKey).to receive(:call).with(openai_api_key:) user.update(openai_api_key:) end it 'does not trigger API validation' do - expect(TaskService::GptGenerateTests).not_to receive(:new_client!) + expect(GptService::ValidateApiKey).not_to receive(:call) expect { user.update(openai_api_key:) }.not_to change(user, :openai_api_key) end diff --git a/spec/services/task_service/gpt_generate_tests_spec.rb b/spec/services/gpt_service/generate_tests_spec.rb similarity index 79% rename from spec/services/task_service/gpt_generate_tests_spec.rb rename to spec/services/gpt_service/generate_tests_spec.rb index 4abb69bac..d30aec325 100644 --- a/spec/services/task_service/gpt_generate_tests_spec.rb +++ b/spec/services/gpt_service/generate_tests_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe TaskService::GptGenerateTests do +RSpec.describe GptService::GenerateTests do let(:openai_api_key) { 'valid_api_key' } let(:openai_client) { OpenAI::Client.new(access_token: openai_api_key) } let(:openai_models) { instance_double(OpenAI::Models, list: {'data' => [{'id' => 'model-id'}]}) } @@ -47,42 +47,6 @@ end end - describe '.new_client!' do - subject(:gpt_generate_tests_service) { described_class.new_client!(openai_api_key, validate: true) } - - context 'when API key is invalid' do - let(:openai_api_key) { 'invalid_api_key' } - - before do - allow(openai_models).to receive(:list).and_raise(Faraday::UnauthorizedError) - end - - it 'raises InvalidApiKeyError' do - expect { gpt_generate_tests_service }.to raise_error(Gpt::Error::InvalidApiKey) - end - end - - context 'when OpenAI is not responding' do - before do - allow(openai_models).to receive(:list).and_raise(Faraday::Error) - end - - it 'raises InternalServerError' do - expect { gpt_generate_tests_service }.to raise_error(Gpt::Error::InternalServerError) - end - end - - context 'when the network connection is broken' do - before do - allow(openai_models).to receive(:list).and_raise(EOFError) - end - - it 'raises an error' do - expect { gpt_generate_tests_service }.to raise_error(Gpt::Error) - end - end - end - describe '#call' do subject(:gpt_generate_tests) { described_class.call(task:, openai_api_key:) } @@ -138,5 +102,17 @@ expect { gpt_generate_tests }.to raise_error(Gpt::Error) end end + + context 'when API key is invalid' do + let(:openai_api_key) { 'invalid_api_key' } + + before do + allow(openai_client).to receive(:chat).and_raise(Faraday::UnauthorizedError) + end + + it 'raises InvalidApiKeyError' do + expect { gpt_generate_tests }.to raise_error(Gpt::Error::InvalidApiKey) + end + end end end diff --git a/spec/services/gpt_service/validate_api_key_spec.rb b/spec/services/gpt_service/validate_api_key_spec.rb new file mode 100644 index 000000000..58938df2e --- /dev/null +++ b/spec/services/gpt_service/validate_api_key_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe GptService::ValidateApiKey do + let(:openai_api_key) { 'valid_api_key' } + let(:openai_client) { OpenAI::Client.new(access_token: openai_api_key) } + let(:openai_models) { instance_double(OpenAI::Models, list: {'data' => models_list}) } + let(:models_list) { [{'id' => 'model-id'}] } + + before do + allow(OpenAI::Client).to receive(:new).and_return(openai_client) + allow(openai_client).to receive(:models).and_return(openai_models) + end + + describe '.new' do + subject(:validate_api_key) { described_class.new(openai_api_key:) } + + it 'assigns the client for OpenAI' do + expect(validate_api_key.instance_variable_get(:@client)).to be openai_client + end + + it 'stores the OpenAI API key in the client' do + expect(validate_api_key.instance_variable_get(:@client).access_token).to eq openai_api_key + end + + context 'when API key is missing' do + let(:openai_api_key) { nil } + + it 'raises InvalidApiKeyError' do + expect { validate_api_key }.to raise_error(Gpt::Error::InvalidApiKey) + end + end + end + + describe '#call' do + subject(:validate_api_key) { described_class.call(openai_api_key:) } + + it 'does not raise an error' do + expect { validate_api_key }.not_to raise_error + end + + context 'when model list is empty' do + let(:models_list) {} + + it 'raises correct error' do + expect { validate_api_key }.to raise_error(Gpt::Error::InvalidApiKey) + end + end + + context 'when API key is invalid' do + let(:openai_api_key) { 'invalid_api_key' } + + before do + allow(openai_models).to receive(:list).and_raise(Faraday::UnauthorizedError) + end + + it 'raises InvalidApiKeyError' do + expect { validate_api_key }.to raise_error(Gpt::Error::InvalidApiKey) + end + end + + context 'when OpenAI is not responding' do + before do + allow(openai_models).to receive(:list).and_raise(Faraday::Error) + end + + it 'raises InternalServerError' do + expect { validate_api_key }.to raise_error(Gpt::Error::InternalServerError) + end + end + + context 'when the network connection is broken' do + before do + allow(openai_models).to receive(:list).and_raise(EOFError) + end + + it 'raises an error' do + expect { validate_api_key }.to raise_error(Gpt::Error) + end + end + end +end