fix/12783-error-for-dup-model-name

frontend/src/__tests__/cypress/cypress/pages/modelRegistry/registerModelPage.ts

@@ -56,6 +56,10 @@ class RegisterModelPage {
   findSubmitButton() {
     return cy.findByTestId('create-button');
   }
+
+  findModelNameError() {
+    return cy.findByTestId('model-name-error');
+  }
 }
 
 export const registerModelPage = new RegisterModelPage();

frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerModel.cy.ts

@@ -23,8 +23,10 @@ import {
   type ModelArtifact,
 } from '~/concepts/modelRegistry/types';
 import { mockModelRegistryService } from '~/__mocks__/mockModelRegistryService';
+import { mockRegisteredModelList } from '~/__mocks__/mockRegisteredModelsList';
 
 const MODEL_REGISTRY_API_VERSION = 'v1alpha3';
+const existingModelName = 'model1';
 
 const initIntercepts = () => {
   cy.interceptOdh(
@@ -63,6 +65,21 @@ const initIntercepts = () => {
     ]),
   );
 
+  cy.interceptOdh(
+    `GET /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models`,
+    {
+      path: { serviceName: 'modelregistry-sample', apiVersion: MODEL_REGISTRY_API_VERSION },
+    },
+    mockRegisteredModelList({
+      items: [
+        mockRegisteredModel({
+          id: '1',
+          name: existingModelName,
+        }),
+      ],
+    }),
+  ).as('getRegisteredModels');
+
   cy.interceptOdh(
     'POST /api/service/modelregistry/:serviceName/api/model_registry/:apiVersion/registered_models',
     {
@@ -205,6 +222,24 @@ describe('Register model page', () => {
     registerModelPage.findSubmitButton().should('be.enabled');
   });
 
+  it('Disables submit if model name is duplicated', () => {
+    registerModelPage.findSubmitButton().should('be.disabled');
+    registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).type('Test model name');
+    registerModelPage.findFormField(FormFieldSelector.VERSION_NAME).type('Test version name');
+    registerModelPage.findFormField(FormFieldSelector.LOCATION_TYPE_OBJECT_STORAGE).click();
+    registerModelPage
+      .findFormField(FormFieldSelector.LOCATION_ENDPOINT)
+      .type('http://s3.amazonaws.com/');
+    registerModelPage.findFormField(FormFieldSelector.LOCATION_BUCKET).type('test-bucket');
+    registerModelPage
+      .findFormField(FormFieldSelector.LOCATION_PATH)
+      .type('demo-models/flan-t5-small-caikit');
+    registerModelPage.findSubmitButton().should('be.enabled');
+    registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).clear().type(existingModelName);
+    registerModelPage.findSubmitButton().should('be.disabled');
+    registerModelPage.findModelNameError().contains('Model name already exists');
+  });
+
   it('Creates expected resources on submit in object storage mode', () => {
     const veryLongName = 'Test name'.repeat(15); // A string over 128 characters
     registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).type('Test model name');

frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx

@@ -21,8 +21,14 @@ import ApplicationsPage from '~/pages/ApplicationsPage';
 import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils';
 import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext';
 import { useAppSelector } from '~/redux/hooks';
+import useRegisteredModels from '~/concepts/modelRegistry/apiHooks/useRegisteredModels';
 import { useRegisterModelData } from './useRegisterModelData';
-import { isNameValid, isRegisterModelSubmitDisabled, registerModel } from './utils';
+import {
+  isModelNameExisting,
+  isNameValid,
+  isRegisterModelSubmitDisabled,
+  registerModel,
+} from './utils';
 import RegistrationCommonFormSections from './RegistrationCommonFormSections';
 import PrefilledModelRegistryField from './PrefilledModelRegistryField';
 import RegistrationFormFooter from './RegistrationFormFooter';
@@ -36,12 +42,18 @@ const RegisterModel: React.FC = () => {
   const [isSubmitting, setIsSubmitting] = React.useState(false);
   const [submitError, setSubmitError] = React.useState<Error | undefined>(undefined);
   const [formData, setData] = useRegisterModelData();
-  const isModelNameValid = isNameValid(formData.modelName);
-  const isSubmitDisabled = isSubmitting || isRegisterModelSubmitDisabled(formData);
-  const { modelName, modelDescription } = formData;
   const [registeredModelName, setRegisteredModelName] = React.useState<string>('');
   const [versionName, setVersionName] = React.useState<string>('');
   const [errorName, setErrorName] = React.useState<string | undefined>(undefined);
+  const [registeredModels, registeredModelsLoaded, registeredModelsLoadError] =
+    useRegisteredModels();
+
+  const isModelNameValid = isNameValid(formData.modelName);
+  const isModelNameDuplicate = isModelNameExisting(formData.modelName, registeredModels);
+  const hasModelNameError = !isModelNameValid || isModelNameDuplicate;
+  const isSubmitDisabled =
+    isSubmitting || isRegisterModelSubmitDisabled(formData, registeredModels);
+  const { modelName, modelDescription } = formData;
 
   const handleSubmit = async () => {
     setIsSubmitting(true);
@@ -76,7 +88,8 @@ const RegisterModel: React.FC = () => {
           <BreadcrumbItem>Register model</BreadcrumbItem>
         </Breadcrumb>
       }
-      loaded
+      loaded={registeredModelsLoaded}
+      loadError={registeredModelsLoadError}
       empty={false}
     >
       <PageSection hasBodyWrapper={false} isFilled>
@@ -98,13 +111,15 @@ const RegisterModel: React.FC = () => {
                     name="model-name"
                     value={modelName}
                     onChange={(_e, value) => setData('modelName', value)}
-                    validated={isModelNameValid ? 'default' : 'error'}
+                    validated={hasModelNameError ? 'error' : 'default'}
                   />
-                  {!isModelNameValid && (
+                  {hasModelNameError && (
                     <FormHelperText>
                       <HelperText>
-                        <HelperTextItem variant="error">
-                          Cannot exceed {MR_CHARACTER_LIMIT} characters
+                        <HelperTextItem variant="error" data-testid="model-name-error">
+                          {isModelNameDuplicate
+                            ? 'Model name already exists'
+                            : `Cannot exceed ${MR_CHARACTER_LIMIT} characters`}
                         </HelperTextItem>
                       </HelperText>
                     </FormHelperText>

frontend/src/pages/modelRegistry/screens/RegisterModel/__tests__/utils.spec.ts

@@ -0,0 +1,33 @@
+import { RegisteredModelList } from '~/concepts/modelRegistry/types';
+import {
+  isModelNameExisting,
+  isNameValid,
+} from '~/pages/modelRegistry/screens/RegisterModel/utils';
+import { MR_CHARACTER_LIMIT } from '~/pages/modelRegistry/screens/RegisterModel/const';
+
+describe('RegisterModel utils', () => {
+  describe('isModelNameExisting', () => {
+    const existingModelName = 'model2';
+    const newModelName = 'model4';
+    const modelList = {
+      items: [{ name: 'model1' }, { name: existingModelName }, { name: 'model3' }],
+    } as RegisteredModelList;
+    it('should return true if model name exists in list', () => {
+      expect(isModelNameExisting(existingModelName, modelList)).toBe(true);
+    });
+
+    it('should return false if model name does not exist in list', () => {
+      expect(isModelNameExisting(newModelName, modelList)).toBe(false);
+    });
+  });
+
+  describe('isNameValid', () => {
+    it('should return true for valid model names (currently only limited by character count)', () => {
+      expect(isNameValid('x'.repeat(MR_CHARACTER_LIMIT))).toBe(true);
+      expect(isNameValid('')).toBe(true); //will be caught by form 'required' validation
+    });
+    it('should return false for names that are too long', () => {
+      expect(isNameValid('x'.repeat(MR_CHARACTER_LIMIT + 1))).toBe(false);
+    });
+  });
+});

frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts

@@ -4,6 +4,7 @@ import {
   ModelState,
   ModelVersion,
   RegisteredModel,
+  RegisteredModelList,
 } from '~/concepts/modelRegistry/types';
 import { ModelRegistryAPIState } from '~/concepts/modelRegistry/context/useModelRegistryAPIState';
 import { objectStorageFieldsToUri } from '~/concepts/modelRegistry/utils';
@@ -140,12 +141,19 @@ const isSubmitDisabledForCommonFields = (formData: RegistrationCommonFormData):
   );
 };
 
-export const isRegisterModelSubmitDisabled = (formData: RegisterModelFormData): boolean =>
+export const isRegisterModelSubmitDisabled = (
+  formData: RegisterModelFormData,
+  registeredModels: RegisteredModelList,
+): boolean =>
   !formData.modelName ||
   isSubmitDisabledForCommonFields(formData) ||
-  !isNameValid(formData.modelName);
+  !isNameValid(formData.modelName) ||
+  isModelNameExisting(formData.modelName, registeredModels);
 
 export const isRegisterVersionSubmitDisabled = (formData: RegisterVersionFormData): boolean =>
   !formData.registeredModelId || isSubmitDisabledForCommonFields(formData);
 
 export const isNameValid = (name: string): boolean => name.length <= MR_CHARACTER_LIMIT;
+
+export const isModelNameExisting = (name: string, registeredModels: RegisteredModelList): boolean =>
+  registeredModels.items.some((model) => model.name === name);