Skip to content

Commit e9fa6c1

Browse files
Philip-CarneiroPhilip-Carneiro
authored
Add name field validation with maxLength to registry creation (#6869)
* fix the code for the actual problem Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix nitpicks issues Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * fix nitpicks issues Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> * add changes requested Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com> --------- Signed-off-by: Philip Colares Carneiro <philip.colares@gmail.com>
1 parent d7e30b8 commit e9fa6c1

5 files changed

Lines changed: 105 additions & 29 deletions

File tree

‎frontend/src/concepts/k8s/K8sNameDescriptionField/utils.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,14 @@ export enum LimitNameResourceType {
3333
/** Model deployments create routes */
3434
MODEL_DEPLOYMENT,
3535
PVC,
36+
/** Model Registry names are capped at 40 characters */
37+
MODEL_REGISTRY,
3638
}
3739
/** K8s max DNS subdomain name length */
3840
const MAX_RESOURCE_NAME_LENGTH = 253;
3941

4042
const MAX_PVC_NAME_LENGTH = 63;
43+
const MAX_MODEL_REGISTRY_NAME_LENGTH = 40;
4144

4245
/** KServe InferenceService names must start with a lowercase letter */
4346
export const INFERENCE_SERVICE_NAME_REGEX = /^[a-z]([-a-z0-9]*[a-z0-9])?$/;
@@ -49,6 +52,7 @@ export const resourceTypeLimits: Record<LimitNameResourceType, number> = {
4952
[LimitNameResourceType.WORKBENCH]: ROUTE_BASED_NAME_LENGTH,
5053
[LimitNameResourceType.MODEL_DEPLOYMENT]: ROUTE_BASED_NAME_LENGTH,
5154
[LimitNameResourceType.PVC]: MAX_PVC_NAME_LENGTH,
55+
[LimitNameResourceType.MODEL_REGISTRY]: MAX_MODEL_REGISTRY_NAME_LENGTH,
5256
};
5357

5458
export const isK8sNameDescriptionType = (

‎frontend/src/pages/modelRegistrySettings/CreateModal.tsx‎

Lines changed: 42 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -11,26 +11,26 @@ import {
1111
Stack,
1212
Spinner,
1313
TextInput,
14-
Modal,
15-
ModalBody,
16-
ModalHeader,
17-
ModalFooter,
1814
} from '@patternfly/react-core';
1915
import SimpleSelect, { SimpleSelectOption } from '#~/components/SimpleSelect';
20-
import DashboardModalFooter from '#~/concepts/dashboard/DashboardModalFooter';
16+
import ContentModal from '#~/components/modals/ContentModal';
2117
import { ModelRegistryKind } from '#~/k8sTypes';
2218
import { ModelRegistryModel } from '#~/api';
2319
import {
2420
createModelRegistryBackend,
2521
updateModelRegistryBackend,
2622
} from '#~/services/modelRegistrySettingsService';
27-
import { isValidK8sName, kindApiVersion, translateDisplayNameForK8s } from '#~/concepts/k8s/utils';
23+
import { kindApiVersion } from '#~/concepts/k8s/utils';
2824
import FormSection from '#~/components/pf-overrides/FormSection';
2925
import { AreaContext } from '#~/concepts/areas/AreaContext';
3026
import { SupportedArea, useIsAreaAvailable } from '#~/concepts/areas';
3127
import K8sNameDescriptionField, {
3228
useK8sNameDescriptionFieldData,
3329
} from '#~/concepts/k8s/K8sNameDescriptionField/K8sNameDescriptionField';
30+
import {
31+
isK8sNameDescriptionDataValid,
32+
LimitNameResourceType,
33+
} from '#~/concepts/k8s/K8sNameDescriptionField/utils';
3434
import useModelRegistryCertificateNames from '#~/concepts/modelRegistrySettings/useModelRegistryCertificateNames';
3535
import {
3636
buildDatabaseSpec,
@@ -54,6 +54,7 @@ import {
5454
DEFAULT_DATABASE_NAME,
5555
DEFAULT_MYSQL_PORT,
5656
DEFAULT_POSTGRES_PORT,
57+
MAX_MODEL_REGISTRY_NAME_LENGTH,
5758
ResourceType,
5859
SecureDBRType,
5960
} from './const';
@@ -71,6 +72,7 @@ const CreateModal: React.FC<CreateModalProps> = ({ onClose, refresh, modelRegist
7172
const [error, setError] = React.useState<Error>();
7273
const { data: nameDesc, onDataChange: setNameDesc } = useK8sNameDescriptionFieldData({
7374
initialData: mr,
75+
limitNameResourceType: LimitNameResourceType.MODEL_REGISTRY,
7476
});
7577
const [databaseSource, setDatabaseSource] = React.useState<DatabaseSource>(
7678
DatabaseSource.DEFAULT,
@@ -323,7 +325,7 @@ const CreateModal: React.FC<CreateModalProps> = ({ onClose, refresh, modelRegist
323325
apiVersion: kindApiVersion(ModelRegistryModel),
324326
kind: 'ModelRegistry',
325327
metadata: {
326-
name: nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name),
328+
name: nameDesc.k8sName.value,
327329
namespace: modelRegistryNamespace,
328330
annotations: {
329331
'openshift.io/description': nameDesc.description,
@@ -390,16 +392,12 @@ const CreateModal: React.FC<CreateModalProps> = ({ onClose, refresh, modelRegist
390392
const hasContent = (value: string): boolean => !!value.trim().length;
391393

392394
const canSubmit = () => {
393-
const isValidName = isValidK8sName(
394-
nameDesc.k8sName.value || translateDisplayNameForK8s(nameDesc.name),
395-
);
395+
const isValidName = isK8sNameDescriptionDataValid(nameDesc);
396396

397397
if (databaseSource === DatabaseSource.DEFAULT) {
398-
// For default database, only name is required
399398
return !isSubmitting && isValidName;
400399
}
401400

402-
// For external database, all connection fields are required
403401
return (
404402
!isSubmitting &&
405403
isValidName &&
@@ -419,11 +417,37 @@ const CreateModal: React.FC<CreateModalProps> = ({ onClose, refresh, modelRegist
419417
];
420418

421419
return (
422-
<Modal isOpen onClose={onCancelClose} variant="medium">
423-
<ModalHeader title={`${mr ? 'Edit' : 'Create'} model registry`} />
424-
<ModalBody>
420+
<ContentModal
421+
onClose={onCancelClose}
422+
variant="medium"
423+
title={`${mr ? 'Edit' : 'Create'} model registry`}
424+
error={error}
425+
alertTitle={`Error ${mr ? 'updating' : 'creating'} model registry`}
426+
buttonActions={[
427+
{
428+
label: mr ? 'Update' : 'Create',
429+
onClick: onSubmit,
430+
variant: 'primary',
431+
isLoading: isSubmitting,
432+
isDisabled: !canSubmit(),
433+
dataTestId: 'modal-submit-button',
434+
},
435+
{
436+
label: 'Cancel',
437+
onClick: onCancelClose,
438+
variant: 'link',
439+
isDisabled: isSubmitting,
440+
dataTestId: 'modal-cancel-button',
441+
},
442+
]}
443+
contents={
425444
<Form>
426-
<K8sNameDescriptionField dataTestId="mr" data={nameDesc} onDataChange={setNameDesc} />
445+
<K8sNameDescriptionField
446+
dataTestId="mr"
447+
data={nameDesc}
448+
onDataChange={setNameDesc}
449+
maxLength={MAX_MODEL_REGISTRY_NAME_LENGTH}
450+
/>
427451
<FormSection title="Database" description="Choose where to store model data.">
428452
<FormGroup role="radiogroup" fieldId="mr-database-source">
429453
<Stack hasGutter>
@@ -632,19 +656,8 @@ const CreateModal: React.FC<CreateModalProps> = ({ onClose, refresh, modelRegist
632656
)}
633657
</FormSection>
634658
</Form>
635-
</ModalBody>
636-
<ModalFooter>
637-
<DashboardModalFooter
638-
onCancel={onCancelClose}
639-
onSubmit={onSubmit}
640-
submitLabel={mr ? 'Update' : 'Create'}
641-
isSubmitLoading={isSubmitting}
642-
isSubmitDisabled={!canSubmit()}
643-
error={error}
644-
alertTitle={`Error ${mr ? 'updating' : 'creating'} model registry`}
645-
/>
646-
</ModalFooter>
647-
</Modal>
659+
}
660+
/>
648661
);
649662
};
650663

‎frontend/src/pages/modelRegistrySettings/const.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
export const MAX_MODEL_REGISTRY_NAME_LENGTH = 40;
2+
13
export const ODH_TRUSTED_BUNDLE = 'odh-trusted-ca-bundle';
24
export const CA_BUNDLE_CRT = 'ca-bundle.crt';
35
export const ODH_CA_BUNDLE_CRT = 'odh-ca-bundle.crt';

‎packages/cypress/cypress/pages/modelRegistrySettings.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ import { Contextual } from './components/Contextual';
33
import { K8sNameDescriptionField } from './components/subComponents/K8sNameDescriptionField';
44
import { SearchSelector } from './components/subComponents/SearchSelector';
55

6+
export const MAX_MODEL_REGISTRY_NAME_LENGTH = 40;
7+
68
export enum FormFieldSelector {
79
NAME = '#mr-name',
810
DESCRIPTION = '#mr-description',

‎packages/cypress/cypress/tests/mocked/modelRegistrySettings/modelRegistrySettings.cy.ts‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { mockRoleBindingK8sResource } from '@odh-dashboard/internal/__mocks__/mo
1212
import {
1313
DatabaseType,
1414
FormFieldSelector,
15+
MAX_MODEL_REGISTRY_NAME_LENGTH,
1516
modelRegistrySettings,
1617
} from '../../../pages/modelRegistrySettings';
1718
import { pageNotfound } from '../../../pages/pageNotFound';
@@ -336,6 +337,60 @@ describe('CreateModal', () => {
336337
modelRegistrySettings.shouldHaveAllErrors();
337338
});
338339

340+
it('should enforce maxLength on the name input field', () => {
341+
modelRegistrySettings.visit(true);
342+
modelRegistrySettings.findCreateButton().click();
343+
modelRegistrySettings
344+
.findFormField(FormFieldSelector.NAME)
345+
.should('have.attr', 'maxLength', String(MAX_MODEL_REGISTRY_NAME_LENGTH));
346+
});
347+
348+
it('should disable submit when resource name exceeds the character limit', () => {
349+
modelRegistrySettings.visit(true);
350+
modelRegistrySettings.findCreateButton().click();
351+
modelRegistrySettings.findDatabaseSourceDefaultRadio().should('be.checked');
352+
353+
modelRegistrySettings
354+
.findFormField(FormFieldSelector.NAME)
355+
.type('a'.repeat(MAX_MODEL_REGISTRY_NAME_LENGTH));
356+
modelRegistrySettings.findSubmitButton().should('be.enabled');
357+
358+
modelRegistrySettings.k8sNameDescription.findResourceEditLink().click();
359+
modelRegistrySettings.k8sNameDescription
360+
.findResourceNameInput()
361+
.clear()
362+
.type('a'.repeat(MAX_MODEL_REGISTRY_NAME_LENGTH + 1));
363+
modelRegistrySettings.findSubmitButton().should('be.disabled');
364+
});
365+
366+
it('should show character count warning when name approaches the limit', () => {
367+
modelRegistrySettings.visit(true);
368+
modelRegistrySettings.findCreateButton().click();
369+
370+
const nearLimitName = 'a'.repeat(MAX_MODEL_REGISTRY_NAME_LENGTH - 9);
371+
modelRegistrySettings.findFormField(FormFieldSelector.NAME).type(nearLimitName);
372+
cy.contains(`Cannot exceed ${MAX_MODEL_REGISTRY_NAME_LENGTH} characters`).should('be.visible');
373+
cy.contains('remaining').should('be.visible');
374+
});
375+
376+
it('should allow creation with name at exactly the character limit using default database', () => {
377+
modelRegistrySettings.visit(true);
378+
modelRegistrySettings.findCreateButton().click();
379+
modelRegistrySettings.findDatabaseSourceDefaultRadio().should('be.checked');
380+
381+
const exactName = 'a'.repeat(MAX_MODEL_REGISTRY_NAME_LENGTH);
382+
modelRegistrySettings.findFormField(FormFieldSelector.NAME).type(exactName);
383+
modelRegistrySettings.findSubmitButton().should('be.enabled');
384+
modelRegistrySettings.findSubmitButton().click();
385+
386+
cy.wait('@createModelRegistry').then((interception) => {
387+
expect(interception.request.body.modelRegistry.metadata.name).to.equal(exactName);
388+
expect(interception.request.body.modelRegistry.metadata.name).to.have.length.at.most(
389+
MAX_MODEL_REGISTRY_NAME_LENGTH,
390+
);
391+
});
392+
});
393+
339394
it('should enable submit button if fields are valid', () => {
340395
modelRegistrySettings.visit(true);
341396
modelRegistrySettings.findCreateButton().click();

0 commit comments

Comments
 (0)