FIX: Correctly save provider-specific params for new models. (#744)

Creating a new model, either manually or from presets, doesn't initialize the `provider_params` object, meaning their custom params won't persist.

Additionally, this change adds some validations for Bedrock params, which are mandatory, and a clear message when a completion fails because we cannot build the URL.

admin/assets/javascripts/discourse/routes/admin-plugins-show-discourse-ai-llms-new.js

@@ -3,6 +3,7 @@ import DiscourseRoute from "discourse/routes/discourse";
 export default DiscourseRoute.extend({
   async model() {
     const record = this.store.createRecord("ai-llm");
+    record.provider_params = {};
     return record;
   },
 

app/models/llm_model.rb

@@ -12,6 +12,7 @@ class LlmModel < ActiveRecord::Base
   validates :url, presence: true, unless: -> { provider == BEDROCK_PROVIDER_NAME }
   validates_presence_of :name, :api_key
   validates :max_prompt_tokens, numericality: { greater_than: 0 }
+  validate :required_provider_params
 
   def self.provider_params
     {
@@ -79,6 +80,18 @@ class LlmModel < ActiveRecord::Base
   def lookup_custom_param(key)
     provider_params&.dig(key)
   end
+
+  private
+
+  def required_provider_params
+    return if provider != BEDROCK_PROVIDER_NAME
+
+    %w[access_key_id region].each do |field|
+      if lookup_custom_param(field).blank?
+        errors.add(:base, I18n.t("discourse_ai.llm_models.missing_provider_param", param: field))
+      end
+    end
+  end
 end
 
 # == Schema Information

assets/javascripts/discourse/components/ai-llm-editor-form.gjs

@@ -30,14 +30,6 @@ export default class AiLlmEditorForm extends Component {
   @tracked testError = null;
   @tracked apiKeySecret = true;
 
-  didReceiveAttrs() {
-    super.didReceiveAttrs(...arguments);
-
-    if (!this.args.model.provider_params) {
-      this.populateProviderParams(this.args.model.provider);
-    }
-  }
-
   get selectedProviders() {
     const t = (provName) => {
       return I18n.t(`discourse_ai.llms.providers.${provName}`);

config/locales/server.en.yml

@@ -326,3 +326,7 @@ en:
         hint:
           one: "Make sure the `%{settings}` setting was configured."
           other: "Make sure the settings of the provider you want were configured. Options are: %{settings}"
+
+    llm_models:
+      missing_provider_param: "%{param} can't be blank"
+      bedrock_invalid_url: "Please complete all the fields to contact this model."

db/migrate/20240807150605_add_default_to_provider_params.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+class AddDefaultToProviderParams < ActiveRecord::Migration[7.1]
+  def change
+    change_column_default :llm_models, :provider_params, from: nil, to: {}
+  end
+end

lib/completions/endpoints/aws_bedrock.rb

@@ -66,6 +66,10 @@ module DiscourseAi
               llm_model.name
             end
 
+          if region.blank? || bedrock_model_id.blank?
+            raise CompletionFailed.new(I18n.t("discourse_ai.llm_models.bedrock_invalid_url"))
+          end
+
           api_url =
             "https://bedrock-runtime.#{region}.amazonaws.com/model/#{bedrock_model_id}/invoke"
 

spec/requests/admin/ai_llms_controller_spec.rb

@@ -97,6 +97,46 @@ RSpec.describe DiscourseAi::Admin::AiLlmsController do
         expect(created_model).to be_nil
       end
     end
+
+    context "with provider-specific params" do
+      it "doesn't create a model if a Bedrock param is missing" do
+        post "/admin/plugins/discourse-ai/ai-llms.json",
+             params: {
+               ai_llm:
+                 valid_attrs.merge(
+                   provider: "aws_bedrock",
+                   provider_params: {
+                     region: "us-east-1",
+                   },
+                 ),
+             }
+
+        created_model = LlmModel.last
+
+        expect(response.status).to eq(422)
+        expect(created_model).to be_nil
+      end
+
+      it "creates the model if all required provider params are present" do
+        post "/admin/plugins/discourse-ai/ai-llms.json",
+             params: {
+               ai_llm:
+                 valid_attrs.merge(
+                   provider: "aws_bedrock",
+                   provider_params: {
+                     region: "us-east-1",
+                     access_key_id: "test",
+                   },
+                 ),
+             }
+
+        created_model = LlmModel.last
+
+        expect(response.status).to eq(201)
+        expect(created_model.lookup_custom_param("region")).to eq("us-east-1")
+        expect(created_model.lookup_custom_param("access_key_id")).to eq("test")
+      end
+    end
   end
 
   describe "PUT #update" do