Merge bcf5eb2 into ddaadf3

services/orchest-webserver/client/src/components/ServiceForm.tsx

@@ -24,6 +24,7 @@ import Tooltip from "@mui/material/Tooltip";
 import Typography from "@mui/material/Typography";
 import { visuallyHidden } from "@mui/utils";
 import {
+  hasValue,
   makeCancelable,
   makeRequest,
   PromiseManager,
@@ -60,11 +61,12 @@ const FormSectionTitle: React.FC<{ title: string }> = ({ children, title }) => (
 
 const ServiceForm: React.FC<{
   service: Service;
+  services: Record<string, Service>;
+  serviceUuid: string;
   project_uuid: string;
   pipeline_uuid: string;
   run_uuid: string;
   updateService: (service: Service) => void;
-  nameChangeService: (oldName: string, newName: string) => void;
   disabled: boolean;
 }> = (props) => {
   const environmentPrefix = "environment@";
@@ -100,9 +102,24 @@ const ServiceForm: React.FC<{
     props.updateService(service);
   };
 
+  const [serviceName, setServiceName] = React.useState(props.service.name);
+  const [serviceNameError, setServiceNameError] = React.useState<string | null>(
+    null
+  );
+  const hasServiceNameError = hasValue(serviceNameError);
+
+  const allServiceNames = React.useMemo(() => {
+    return Object.values(props.services).map((s) => s.name);
+  }, [props.services]);
   const handleNameChange = (newName: string) => {
-    let oldName = props.service["name"];
-    props.nameChangeService(oldName, newName);
+    setServiceName(newName);
+
+    if (newName !== props.service.name && allServiceNames.includes(newName)) {
+      setServiceNameError("Same service name has been taken");
+      return;
+    }
+    setServiceNameError(null);
+    props.updateService({ ...props.service, name: newName });
   };
 
   const handleServiceBindsChange = (key: string, value: any) => {
@@ -194,19 +211,23 @@ const ServiceForm: React.FC<{
                   label="Name"
                   type="text"
                   disabled={props.disabled}
-                  value={props.service.name}
+                  error={hasServiceNameError}
+                  value={serviceName}
                   onChange={(e) => {
                     handleNameChange(e.target.value);
                   }}
                   fullWidth
-                  helperText="The name of the service. Up to 36 digits, letters or dashes are allowed."
+                  helperText={
+                    serviceNameError ||
+                    "The name of the service. Up to 36 digits, letters or dashes are allowed."
+                  }
                   aria-describedby="tooltip-name"
                   data-test-id={`service-${props.service.name}-name`}
                 />
                 <TextField
                   label="Image"
                   type="text"
-                  disabled={props.disabled}
+                  disabled={props.disabled || hasServiceNameError}
                   onClick={() => {
                     // TODO:  improve this
                     setShowImageDialog(true);
@@ -231,7 +252,7 @@ const ServiceForm: React.FC<{
                 <TextField
                   label="Entrypoint (optional)"
                   type="text"
-                  disabled={props.disabled}
+                  disabled={props.disabled || hasServiceNameError}
                   value={props.service.entrypoint}
                   onChange={(e) => {
                     handleServiceChange("entrypoint", e.target.value);
@@ -245,7 +266,7 @@ const ServiceForm: React.FC<{
                 <TextField
                   label="Command (optional)"
                   type="text"
-                  disabled={props.disabled}
+                  disabled={props.disabled || hasServiceNameError}
                   value={props.service.command}
                   onChange={(e) => {
                     handleServiceChange("command", e.target.value);
@@ -264,7 +285,7 @@ const ServiceForm: React.FC<{
                 <TextField
                   label="Project directory (optional)"
                   type="text"
-                  disabled={props.disabled}
+                  disabled={props.disabled || hasServiceNameError}
                   value={props.service.binds?.["/project-dir"]}
                   onChange={(e) => {
                     handleServiceBindsChange("/project-dir", e.target.value);
@@ -275,7 +296,7 @@ const ServiceForm: React.FC<{
                 <TextField
                   label="Data directory (optional)"
                   type="text"
-                  disabled={props.disabled}
+                  disabled={props.disabled || hasServiceNameError}
                   value={props.service?.binds?.["/data"]}
                   onChange={(e) => {
                     handleServiceBindsChange("/data", e.target.value);
@@ -299,7 +320,7 @@ const ServiceForm: React.FC<{
                           }))
                         : []
                     }
-                    disabled={props.disabled}
+                    disabled={props.disabled || hasServiceNameError}
                     onChange={(ports) => {
                       handleServiceChange(
                         "ports",
@@ -350,6 +371,7 @@ const ServiceForm: React.FC<{
                       control={
                         <Checkbox
                           checked={props.service?.preserve_base_path === true}
+                          disabled={props.disabled || hasServiceNameError}
                           onChange={(e) => {
                             handleServiceChange(
                               "preserve_base_path",
@@ -368,7 +390,7 @@ const ServiceForm: React.FC<{
                   <FormGroup>
                     <FormControlLabel
                       label="Authentication required"
-                      disabled={props.disabled}
+                      disabled={props.disabled || hasServiceNameError}
                       control={
                         <Checkbox
                           checked={
@@ -394,7 +416,7 @@ const ServiceForm: React.FC<{
                   <FormGroup>
                     <FormControlLabel
                       label="Interactive sessions"
-                      disabled={props.disabled}
+                      disabled={props.disabled || hasServiceNameError}
                       control={
                         <Checkbox
                           checked={
@@ -411,7 +433,7 @@ const ServiceForm: React.FC<{
                     />
                     <FormControlLabel
                       label="Job sessions"
-                      disabled={props.disabled}
+                      disabled={props.disabled || hasServiceNameError}
                       control={
                         <Checkbox
                           checked={
@@ -443,7 +465,7 @@ const ServiceForm: React.FC<{
                           value: env_variable.toString(),
                         }
                     )}
-                    disabled={props.disabled}
+                    disabled={props.disabled || hasServiceNameError}
                     onChange={(env_variables) => {
                       handleServiceChange(
                         "env_variables_inherit",

services/orchest-webserver/client/src/hooks/useFetchPipelineJson.ts

@@ -1,7 +1,7 @@
 import { getOrderValue } from "@/pipeline-settings-view/common";
 import { PipelineJson } from "@/types";
 import { getPipelineJSONEndpoint } from "@/utils/webserver-utils";
-import { fetcher } from "@orchest/lib-utils";
+import { fetcher, uuidv4 } from "@orchest/lib-utils";
 import React from "react";
 import useSWR from "swr";
 import { MutatorCallback } from "swr/dist/types";
@@ -46,6 +46,14 @@ export const useFetchPipelineJson = (props: FetchPipelineJsonProps | null) => {
           pipelineObj.services = {};
         }
 
+        // use temporary uuid for easier FE manipulation, will be cleaned up when saving
+        pipelineObj.services = Object.values(pipelineObj.services).reduce(
+          (all, curr) => {
+            return { ...all, [uuidv4()]: curr };
+          },
+          {}
+        );
+
         // Augment services with order key
         for (let service in pipelineObj.services) {
           pipelineObj.services[service].order = getOrderValue();

services/orchest-webserver/client/src/pipeline-settings-view/PipelineSettingsView.tsx

@@ -61,6 +61,7 @@ import {
   makeCancelable,
   makeRequest,
   PromiseManager,
+  uuidv4,
 } from "@orchest/lib-utils";
 import "codemirror/mode/javascript/javascript";
 import cloneDeep from "lodash.clonedeep";
@@ -214,42 +215,36 @@ const PipelineSettingsView: React.FC = () => {
   const addServiceFromTemplate = (service: ServiceTemplate["config"]) => {
     let clonedService = cloneDeep(service);
 
-    // Take care of service name collisions
-    let x = 1;
-    let baseServiceName = clonedService.name;
-    while (x < 100) {
-      if (pipelineJson.services[clonedService.name] == undefined) {
+    const services = pipelineJson?.services || {};
+
+    const allNames = new Set(Object.values(services).map((s) => s.name));
+
+    let count = 0;
+    // assuming that user won't have more than 100 instances of the same service
+    while (count < 100) {
+      const newName = `${clonedService.name}${count === 0 ? "" : count}`;
+      if (!allNames.has(newName)) {
+        clonedService.name = newName;
         break;
       }
-      clonedService.name = baseServiceName + x;
-      x++;
+      count += 1;
     }
 
-    onChangeService(clonedService);
+    onChangeService(uuidv4(), clonedService);
   };
 
-  const onChangeService = (service: Service) => {
+  const onChangeService = (uuid: string, service: Service) => {
     setPipelineJson((current) => {
       // Maintain client side order key
       if (service.order === undefined) service.order = getOrderValue();
-      current.services[service.name] = service;
+      current.services[uuid] = service;
       return current;
     });
 
     setServicesChanged(true);
     setAsSaved(false);
   };
 
-  const nameChangeService = (oldName: string, newName: string) => {
-    setPipelineJson((current) => {
-      current[newName] = current[oldName];
-      delete current.services[oldName];
-      return current;
-    });
-    setServicesChanged(true);
-    setAsSaved(false);
-  };
-
   const deleteService = async (serviceName: string) => {
     setPipelineJson((current) => {
       delete current.services[serviceName];
@@ -324,8 +319,13 @@ const PipelineSettingsView: React.FC = () => {
     pipelineJson: PipelineJson
   ): Omit<PipelineJson, "order"> => {
     let pipelineCopy = cloneDeep(pipelineJson);
-    for (let serviceName in pipelineCopy.services) {
-      delete pipelineCopy.services[serviceName].order;
+    for (let uuid in pipelineCopy.services) {
+      const serviceName = pipelineCopy.services[uuid].name;
+      delete pipelineCopy.services[uuid].order;
+      pipelineCopy.services[serviceName] = {
+        ...pipelineCopy.services[uuid],
+      };
+      delete pipelineCopy.services[uuid];
     }
     return pipelineCopy;
   };
@@ -521,18 +521,19 @@ const PipelineSettingsView: React.FC = () => {
         .map(([key, service]) => {
           return {
             uuid: key,
-            name: key,
+            name: service.name,
             scope: service.scope
               .map((scopeAsString) => scopeMap[scopeAsString])
               .join(", "),
             remove: key,
             details: (
               <ServiceForm
-                key={`ServiceForm-${key}`}
+                key={key}
+                serviceUuid={key}
                 service={service}
+                services={pipelineJson.services}
                 disabled={isReadOnly}
-                updateService={onChangeService}
-                nameChangeService={nameChangeService}
+                updateService={(updated) => onChangeService(key, updated)}
                 pipeline_uuid={pipelineUuid}
                 project_uuid={projectUuid}
                 run_uuid={runUuid}

services/orchest-webserver/client/src/utils/webserver-utils.ts

@@ -1,5 +1,5 @@
 import { EnvVarPair } from "@/components/EnvVarList";
-import { PipelineJson } from "@/types";
+import { PipelineJson, Service } from "@/types";
 import { pipelineSchema } from "@/utils/pipeline-schema";
 import { extensionFromFilename, makeRequest } from "@orchest/lib-utils";
 import Ajv from "ajv";
@@ -140,11 +140,7 @@ export function clearOutgoingConnections(steps: {
 }
 
 export function getServiceURLs(
-  service: {
-    ports: number[];
-    preserve_base_path: string;
-    name: string;
-  },
+  service: Pick<Service, "ports" | "preserve_base_path" | "name">,
   projectUuid: string,
   pipelineUuid: string,
   runUuid: string