Add character length limit to both model name and version name

ppadti · Oct 25, 2024 · f58ff68 · f58ff68
1 parent 96e06a2
commit f58ff68
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 16 deletions.
diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerModel.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerModel.cy.ts
@@ -182,6 +182,7 @@ describe('Register model page', () => {
   });
 
   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');
     registerModelPage
       .findFormField(FormFieldSelector.MODEL_DESCRIPTION)
@@ -201,7 +202,17 @@ describe('Register model page', () => {
     registerModelPage
       .findFormField(FormFieldSelector.LOCATION_PATH)
       .type('demo-models/flan-t5-small-caikit');
+    registerModelPage.findSubmitButton().should('be.enabled');
 
+    registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).clear().type(veryLongName);
+    registerModelPage.findSubmitButton().should('be.disabled');
+    registerModelPage.findFormField(FormFieldSelector.VERSION_NAME).clear().type(veryLongName);
+    registerModelPage.findFormField(FormFieldSelector.MODEL_NAME).clear().type('Test model name');
+    registerModelPage.findSubmitButton().should('be.disabled');
+    registerModelPage
+      .findFormField(FormFieldSelector.VERSION_NAME)
+      .clear()
+      .type('Test version name');
     registerModelPage.findSubmitButton().click();
 
     cy.wait('@createRegisteredModel').then((interception) => {
@@ -224,7 +235,7 @@ describe('Register model page', () => {
     });
     cy.wait('@createModelArtifact').then((interception) => {
       expect(interception.request.body).to.containSubset({
-        name: 'Test model name-Test version name-artifact',
+        name: 'Test version name',
         description: 'Test version description',
         customProperties: {},
         state: ModelArtifactState.LIVE,
@@ -292,7 +303,7 @@ describe('Register model page', () => {
     });
     cy.wait('@createModelArtifact').then((interception) => {
       expect(interception.request.body).to.containSubset({
-        name: 'Test model name-Test version name-artifact',
+        name: 'Test version name',
         description: 'Test version description',
         customProperties: {},
         state: ModelArtifactState.LIVE,

diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerVersion.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerVersion.cy.ts
@@ -350,6 +350,7 @@ describe('Register model page with no preselected model', () => {
   });
 
   it('Creates expected resources on submit in object storage mode', () => {
+    const veryLongName = 'Test name'.repeat(15); // A string over 128 characters
     registerVersionPage.selectRegisteredModel('Test model 1');
     registerVersionPage.findFormField(FormFieldSelector.VERSION_NAME).type('Test version name');
     registerVersionPage
@@ -359,6 +360,14 @@ describe('Register model page with no preselected model', () => {
       .findFormField(FormFieldSelector.LOCATION_PATH)
       .type('demo-models/flan-t5-small-caikit');
 
+    registerVersionPage.findFormField(FormFieldSelector.VERSION_NAME).clear().type(veryLongName);
+    registerVersionPage.findSubmitButton().should('be.disabled');
+    registerVersionPage
+      .findFormField(FormFieldSelector.VERSION_NAME)
+      .clear()
+      .type('Test version name');
+
+    registerVersionPage.findSubmitButton().should('be.enabled');
     registerVersionPage.findSubmitButton().click();
 
     cy.wait('@createModelVersion').then((interception) => {
@@ -373,7 +382,7 @@ describe('Register model page with no preselected model', () => {
     });
     cy.wait('@createModelArtifact').then((interception) => {
       expect(interception.request.body).to.containSubset({
-        name: 'Test model 1-Test version name-artifact',
+        name: 'Test version name',
         description: 'Test version description',
         customProperties: {},
         state: ModelArtifactState.LIVE,
@@ -428,7 +437,7 @@ describe('Register model page with no preselected model', () => {
     });
     cy.wait('@createModelArtifact').then((interception) => {
       expect(interception.request.body).to.containSubset({
-        name: 'Test model 1-Test version name-artifact',
+        name: 'Test version name',
         description: 'Test version description',
         customProperties: {},
         state: ModelArtifactState.LIVE,
@@ -513,7 +522,7 @@ describe('Register model page with preselected model', () => {
     });
     cy.wait('@createModelArtifact').then((interception) => {
       expect(interception.request.body).to.containSubset({
-        name: 'Test model 1-Test version name-artifact',
+        name: 'Test version name',
         description: 'Test version description',
         customProperties: {},
         state: ModelArtifactState.LIVE,
@@ -568,7 +577,7 @@ describe('Register model page with preselected model', () => {
     });
     cy.wait('@createModelArtifact').then((interception) => {
       expect(interception.request.body).to.containSubset({
-        name: 'Test model 1-Test version name-artifact',
+        name: 'Test version name',
         description: 'Test version description',
         customProperties: {},
         state: ModelArtifactState.LIVE,

diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx
@@ -4,6 +4,9 @@ import {
   BreadcrumbItem,
   Form,
   FormGroup,
+  FormHelperText,
+  HelperText,
+  HelperTextItem,
   PageSection,
   Stack,
   StackItem,
@@ -17,7 +20,7 @@ import FormSection from '~/components/pf-overrides/FormSection';
 import ApplicationsPage from '~/pages/ApplicationsPage';
 import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/screens/routeUtils';
 import { useRegisterModelData } from './useRegisterModelData';
-import { isRegisterModelSubmitDisabled, registerModel } from './utils';
+import { isNameValid, isRegisterModelSubmitDisabled, registerModel } from './utils';
 import RegistrationCommonFormSections from './RegistrationCommonFormSections';
 import { useRegistrationCommonState } from './useRegistrationCommonState';
 import PrefilledModelRegistryField from './PrefilledModelRegistryField';
@@ -31,7 +34,13 @@ const RegisterModel: React.FC = () => {
     useRegistrationCommonState();
 
   const [formData, setData] = useRegisterModelData();
-  const isSubmitDisabled = isSubmitting || isRegisterModelSubmitDisabled(formData);
+  const isVersionNameValid = isNameValid(formData.versionName);
+  const isModelNameValid = isNameValid(formData.modelName);
+  const isSubmitDisabled =
+    isSubmitting ||
+    isRegisterModelSubmitDisabled(formData) ||
+    !isVersionNameValid ||
+    !isModelNameValid;
   const { modelName, modelDescription } = formData;
 
   const onSubmit = () =>
@@ -75,7 +84,17 @@ const RegisterModel: React.FC = () => {
                     name="model-name"
                     value={modelName}
                     onChange={(_e, value) => setData('modelName', value)}
+                    validated={isModelNameValid ? 'default' : 'error'}
                   />
+                  {!isModelNameValid && (
+                    <FormHelperText>
+                      <HelperText>
+                        <HelperTextItem variant="error">
+                          Cannot exceed 128 characters
+                        </HelperTextItem>
+                      </HelperText>
+                    </FormHelperText>
+                  )}
                 </FormGroup>
                 <FormGroup label="Model description" fieldId="model-description">
                   <TextArea
@@ -91,6 +110,7 @@ const RegisterModel: React.FC = () => {
                 formData={formData}
                 setData={setData}
                 isFirstVersion
+                isVersionNameValid={isVersionNameValid}
               />
             </StackItem>
           </Stack>

diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterVersion.tsx
@@ -17,7 +17,7 @@ import { modelRegistryUrl, registeredModelUrl } from '~/pages/modelRegistry/scre
 import useRegisteredModels from '~/concepts/modelRegistry/apiHooks/useRegisteredModels';
 import { filterLiveModels } from '~/concepts/modelRegistry/utils';
 import { useRegisterVersionData } from './useRegisterModelData';
-import { isRegisterVersionSubmitDisabled, registerVersion } from './utils';
+import { isNameValid, isRegisterVersionSubmitDisabled, registerVersion } from './utils';
 import RegistrationCommonFormSections from './RegistrationCommonFormSections';
 import { useRegistrationCommonState } from './useRegistrationCommonState';
 import PrefilledModelRegistryField from './PrefilledModelRegistryField';
@@ -34,7 +34,9 @@ const RegisterVersion: React.FC = () => {
     useRegistrationCommonState();
 
   const [formData, setData] = useRegisterVersionData(prefilledRegisteredModelId);
-  const isSubmitDisabled = isSubmitting || isRegisterVersionSubmitDisabled(formData);
+  const isVersionNameValid = isNameValid(formData.versionName);
+  const isSubmitDisabled =
+    isSubmitting || isRegisterVersionSubmitDisabled(formData) || !isVersionNameValid;
   const { registeredModelId } = formData;
 
   const [allRegisteredModels, loadedRegisteredModels, loadRegisteredModelsError] =
@@ -121,6 +123,7 @@ const RegisterVersion: React.FC = () => {
                 setData={setData}
                 isFirstVersion={false}
                 latestVersion={latestVersion}
+                isVersionNameValid={isVersionNameValid}
               />
             </StackItem>
           </Stack>

diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationCommonFormSections.tsx b/frontend/src/pages/modelRegistry/screens/RegisterModel/RegistrationCommonFormSections.tsx
@@ -27,13 +27,15 @@ type RegistrationCommonFormSectionsProps<D extends RegistrationCommonFormData> =
   setData: UpdateObjectAtPropAndValue<D>;
   isFirstVersion: boolean;
   latestVersion?: ModelVersion;
+  isVersionNameValid?: boolean;
 };
 
 const RegistrationCommonFormSections = <D extends RegistrationCommonFormData>({
   formData,
   setData,
   isFirstVersion,
   latestVersion,
+  isVersionNameValid,
 }: RegistrationCommonFormSectionsProps<D>): React.ReactNode => {
   const [isAutofillModalOpen, setAutofillModalOpen] = React.useState(false);
 
@@ -86,14 +88,22 @@ const RegistrationCommonFormSections = <D extends RegistrationCommonFormData>({
             name="version-name"
             value={versionName}
             onChange={(_e, value) => setData('versionName', value)}
+            validated={isVersionNameValid ? 'default' : 'error'}
           />
-          {latestVersion && (
-            <FormHelperText>
+          <FormHelperText>
+            {latestVersion && (
               <HelperText>
-                <HelperTextItem>Current version is {latestVersion.name}</HelperTextItem>
+                <HelperTextItem variant="indeterminate">
+                  Current version is {latestVersion.name}
+                </HelperTextItem>
               </HelperText>
-            </FormHelperText>
-          )}
+            )}
+            {!isVersionNameValid && (
+              <HelperText>
+                <HelperTextItem variant="error">Cannot exceed 128 characters</HelperTextItem>
+              </HelperText>
+            )}
+          </FormHelperText>
         </FormGroup>
         <FormGroup label="Version description" fieldId="version-description">
           <TextArea

diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/const.ts
@@ -0,0 +1 @@
+export const NAME_CHARACTER_LIMIT = 128;
diff --git a/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts b/frontend/src/pages/modelRegistry/screens/RegisterModel/utils.ts
@@ -13,6 +13,7 @@ import {
   RegisterVersionFormData,
   RegistrationCommonFormData,
 } from './useRegisterModelData';
+import { NAME_CHARACTER_LIMIT } from './const';
 
 export type RegisterModelCreatedResources = RegisterVersionCreatedResources & {
   registeredModel: RegisteredModel;
@@ -66,7 +67,7 @@ export const registerVersion = async (
     },
   );
   const modelArtifact = await apiState.api.createModelArtifactForModelVersion({}, modelVersion.id, {
-    name: `${registeredModel.name}-${formData.versionName}-artifact`,
+    name: `${formData.versionName}`,
     description: formData.versionDescription,
     customProperties: {},
     state: ModelArtifactState.LIVE,
@@ -112,3 +113,5 @@ export const isRegisterModelSubmitDisabled = (formData: RegisterModelFormData):
 
 export const isRegisterVersionSubmitDisabled = (formData: RegisterVersionFormData): boolean =>
   !formData.registeredModelId || isSubmitDisabledForCommonFields(formData);
+
+export const isNameValid = (name: string): boolean => name.length <= NAME_CHARACTER_LIMIT;