Repository: gocd/gocd
Issue: `runOnAllAgents="true"` added to jobs at GoCD startup

##### Issue Type

<!--- Please specify the issue type to help us categorize the issue, mention any one of the below types -->

- Bug Report

##### Summary

<!--- Provide a brief summary of the issue -->

GoCD surprisingly adds `runOnAllAgents="true"` to jobs.

##### Environment

<!--- 
Describe the environment in which you encountered this issue.
Details of the environment GoCD is running in can be fetched from the GoCD support api - http://<go-server-host>/go/api/support.
If you are pasting plain text, please surround it with 3 backticks on each side

```plain
paste any text output
to prevent formatting
```
-->

👋  Let me know if you need this and I can share this individually, as it might contain sensitive information.

###### Basic environment details

<!--- We recommend providing the for us to reproduce the issue quicker -->

* Go Version: `23.1.0 (16080-54a6971915ff8d402c9fea8cd2ceeb6e31c8cdc8)`
* JAVA Version: `17.0.6`
* OS: `Linux 5.10.176+`

###### Additional Environment Details

<!-- More environment details captured from the support API or other sources can be shared here -->

##### Steps to Reproduce

1. Restarting GoCD

##### Expected Results

1. GoCD config behaving like before.

##### Actual Results

1. GoCD surprisingly adds `runOnAllAgents="true"` to jobs.

##### Possible Fix
<!--- Not obligatory, but suggest a fix or reason for the bug -->

🤷 

##### Log snippets
<!--- 
If you could find any information/exceptions from the logs do provide it here. 
Do mask any confidential information which you don't want to be shared.
Remember to surround them in 3 backticks like this —

```
long log lines go here
```
-->

##### Code snippets/Screenshots

Here is an example of a commit GoCD makes:

```diff
commit a29ab7eddd8a8d864f33a656f96ad1291abe53b7
Author: Upgrade <go-cd-dev@googlegroups.com>
Date:   Fri Aug 11 05:11:37 2023 +0000

    user:Upgrade|timestamp:1691730697644|schema_version:139|go_edition:OpenSource|go_version:23.1.0 (16080-54a6971915ff8d402c9fea8cd2ceeb6e31c8cdc8)|md5:536dedf85183254cabe2192eb1f7c5e2

diff --git a/cruise-config.xml b/cruise-config.xml
index 5234ae934..65a8023b5 100644
--- a/cruise-config.xml
+++ b/cruise-config.xml
@@ -34838,7 +34838,7 @@ fi</arg>
               <artifact type="test" src="some-tests/target/logs/docker" dest="logs" />
             </artifacts>
           </job>
-          <job name="some-tests">
+          <job name="some-tests" runOnAllAgents="true">
             <tasks>
               <fetchartifact artifactOrigin="gocd" srcfile="tag" stage="build" job="defaultJob" />
               <exec command="/bin/bash">
```


##### Any other info
<!--- Provide any other information you would like to share to explain this issue more better -->


Changed files:
config/config-server/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldLoaderTest.java
config/config-api/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverter.java
config/config-api/src/main/java/com/thoughtworks/go/config/GoConfigFieldWriter.java
config/config-api/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverterTest.java
config/config-server/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldLoader.java

diff --git a/config/config-api/src/main/java/com/thoughtworks/go/config/GoConfigFieldWriter.java b/config/config-api/src/main/java/com/thoughtworks/go/config/GoConfigFieldWriter.java
index 3087dc0b1f..de6c0c0a5f 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/config/GoConfigFieldWriter.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/config/GoConfigFieldWriter.java
@@ -19,7 +19,6 @@ import com.thoughtworks.go.config.parser.GoConfigFieldTypeConverter;
 import com.thoughtworks.go.config.registry.ConfigElementImplementationRegistry;
 import com.thoughtworks.go.util.ConfigUtil;
 import org.jdom2.Element;
-import org.springframework.beans.SimpleTypeConverter;
 import org.springframework.beans.TypeMismatchException;
 
 import java.lang.reflect.Field;
@@ -33,23 +32,17 @@ public class GoConfigFieldWriter {
     private final ConfigUtil configUtil = new ConfigUtil("magic");
     private final Field configField;
     private final Object value;
-    private final SimpleTypeConverter typeConverter;
     private final ConfigCache configCache;
     private final ConfigElementImplementationRegistry registry;
 
-    public GoConfigFieldWriter(Field declaredField, Object value, SimpleTypeConverter converter, ConfigCache configCache, final ConfigElementImplementationRegistry registry) {
+    public GoConfigFieldWriter(Field declaredField, Object value, ConfigCache configCache, final ConfigElementImplementationRegistry registry) {
         this.configField = declaredField;
         this.value = value;
         this.configField.setAccessible(true);
-        this.typeConverter = converter;
         this.configCache = configCache;
         this.registry = registry;
     }
 
-    public GoConfigFieldWriter(Field declaredField, Object value, ConfigCache configCache, final ConfigElementImplementationRegistry registry) {
-        this(declaredField, value, new GoConfigFieldTypeConverter(), configCache, registry);
-    }
-
     public Field getConfigField() {
         return configField;
     }
@@ -78,7 +71,7 @@ public class GoConfigFieldWriter {
         try {
             setFieldIfNotNull(configField, o, val);
         } catch (TypeMismatchException e1) {
-            final String message = format("Could not set value [{0}] on Field [{1}] of type [{2}] ", val,
+            final String message = format("Could not set value [{0}] on field [{1}] of type [{2}] ", val,
                     configField.getName(), configField.getType());
             throw new RuntimeException(message, e1);
         }
@@ -87,9 +80,7 @@ public class GoConfigFieldWriter {
     private void bombIfNullAndNotAllowed(Field field, Object instance, ConfigAttribute attribute) {
         try {
             if (!attribute.allowNull()) {
-                bombIfNull(field.get(instance),
-                    () -> "Field '" + field.getName() + "' is still set to null. "
-                                + "Must give a default value.");
+                bombIfNull(field.get(instance), () -> "Field '" + field.getName() + "' is still set to null. Must give a default value.");
             }
         } catch (IllegalAccessException e) {
             throw bomb("Error getting configField: " + field.getName(), e);
@@ -99,7 +90,7 @@ public class GoConfigFieldWriter {
     private void setFieldIfNotNull(Field field, Object instance, Object val) {
         try {
             if (val != null) {
-                Object convertedValue = typeConverter.convertIfNecessary(val, configField.getType());
+                Object convertedValue = GoConfigFieldTypeConverter.forThread().convertIfNecessary(val, configField.getType());
                 field.set(instance, convertedValue);
             }
         } catch (IllegalAccessException e) {
@@ -107,15 +98,15 @@ public class GoConfigFieldWriter {
         }
     }
 
-@SuppressWarnings({"DataFlowIssue", "unchecked"})
-private Collection<?> parseCollection(Element e, Class<?> collectionType) {
+    @SuppressWarnings({"DataFlowIssue", "unchecked"})
+    private Collection<?> parseCollection(Element e, Class<?> collectionType) {
         ConfigCollection collection = collectionType.getAnnotation(ConfigCollection.class);
         Class<?> type = collection.value();
 
 
         Object o = newInstance(collectionType);
         bombUnless(o instanceof Collection,
-            () -> "Must be some sort of list. Was: " + collectionType.getName());
+                () -> "Must be some sort of list. Was: " + collectionType.getName());
 
         Collection<Object> baseCollection = (Collection<Object>) o;
         for (Element childElement : e.getChildren()) {
@@ -124,7 +115,7 @@ private Collection<?> parseCollection(Element e, Class<?> collectionType) {
             }
         }
         bombIf(baseCollection.size() < collection.minimum(),
-            () -> "Required at least " + collection.minimum() + " subelements to '" + e.getName() + "'. "
+                () -> "Required at least " + collection.minimum() + " subelements to '" + e.getName() + "'. "
                         + "Found " + baseCollection.size() + ".");
         return baseCollection;
     }
@@ -192,7 +183,7 @@ private Collection<?> parseCollection(Element e, Class<?> collectionType) {
         ConfigTag tag = configTag(field.getType());
         boolean optional = field.getAnnotation(ConfigSubtag.class).optional();
         boolean isMissingElement = !configUtil.hasChild(e, tag);
-        if(!optional && isMissingElement) {
+        if (!optional && isMissingElement) {
             throw bomb("Non optional tag '" + tag + "' is not in config file. Found: " + configUtil.elementOutput(e));
         }
         return optional && isMissingElement;
@@ -201,7 +192,7 @@ private Collection<?> parseCollection(Element e, Class<?> collectionType) {
     private boolean optionalAndMissingAttribute(Element e, ConfigAttribute attribute) {
         boolean optional = attribute.optional();
         boolean isMissingAttribute = !configUtil.hasAttribute(e, attribute.value());
-        if(!optional && isMissingAttribute) {
+        if (!optional && isMissingAttribute) {
             throw bomb("Non optional attribute '" + attribute.value() + "' is not in element: " + configUtil.elementOutput(e));
         }
         return optional && isMissingAttribute;
diff --git a/config/config-api/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverter.java b/config/config-api/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverter.java
index d7c9ff96b2..40b88dd33a 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverter.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverter.java
@@ -15,16 +15,25 @@
  */
 package com.thoughtworks.go.config.parser;
 
-import java.io.File;
-
+import org.apache.commons.lang3.StringUtils;
 import org.springframework.beans.SimpleTypeConverter;
+import org.springframework.beans.TypeConverter;
 import org.springframework.beans.propertyeditors.FileEditor;
-import org.apache.commons.lang3.StringUtils;
+
+import java.io.File;
 
 public class GoConfigFieldTypeConverter extends SimpleTypeConverter {
 
+    public static TypeConverter forThread() {
+        return typeConverter.get();
+    }
+
     private static final CustomizedFileEditor propertyEditor = new CustomizedFileEditor();
 
+    // Type converters are NOT thread safe. Should be OK to cache them per thread as not too large and don't think
+    // we have too many threads that will be doing so?
+    private static final ThreadLocal<TypeConverter> typeConverter = ThreadLocal.withInitial(GoConfigFieldTypeConverter::new);
+
     public GoConfigFieldTypeConverter() {
         super();
         this.registerCustomEditor(File.class, propertyEditor);
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverterTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverterTest.java
deleted file mode 100644
index 6a70e36071..0000000000
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldTypeConverterTest.java
+++ /dev/null
@@ -1,40 +0,0 @@
-/*
- * Copyright 2023 Thoughtworks, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package com.thoughtworks.go.config.parser;
-
-import java.beans.PropertyEditor;
-import java.io.File;
-import java.lang.reflect.Field;
-import java.lang.reflect.Modifier;
-
-import com.thoughtworks.go.util.ReflectionUtil;
-import org.junit.jupiter.api.Test;
-
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.MatcherAssert.assertThat;
-
-public class GoConfigFieldTypeConverterTest {
-    @Test
-    public void shouldOnlyCreateOneInstanceOfCustomizedFileEditorAndUseIt() throws Exception {
-        GoConfigFieldTypeConverter converter = new GoConfigFieldTypeConverter();
-        Field expectedField = converter.getClass().getDeclaredField("propertyEditor");
-        int modifier = expectedField.getModifiers();
-        assertThat(Modifier.isStatic(modifier), is(true));
-        assertThat(Modifier.isFinal(modifier), is(true));
-        PropertyEditor actual = converter.findCustomEditor(File.class, null);
-        assertThat(actual, is(ReflectionUtil.getStaticField(converter.getClass(), "propertyEditor")));
-    }
-}
diff --git a/config/config-server/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldLoader.java b/config/config-server/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldLoader.java
index 8891250284..e90d5fb7ba 100644
--- a/config/config-server/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldLoader.java
+++ b/config/config-server/src/main/java/com/thoughtworks/go/config/parser/GoConfigFieldLoader.java
@@ -20,14 +20,13 @@ import com.thoughtworks.go.config.registry.ConfigElementImplementationRegistry;
 import com.thoughtworks.go.security.GoCipher;
 import org.jdom2.Attribute;
 import org.jdom2.Element;
-import org.springframework.beans.SimpleTypeConverter;
 import org.springframework.beans.TypeMismatchException;
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
-import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import static com.thoughtworks.go.config.ConfigCache.isAnnotationPresent;
 import static com.thoughtworks.go.config.parser.GoConfigAttributeLoader.attributeParser;
@@ -38,8 +37,7 @@ import static com.thoughtworks.go.util.ExceptionUtils.bomb;
 import static java.text.MessageFormat.format;
 
 public class GoConfigFieldLoader<T> {
-    private static final Map<Field, Boolean> implicits = new HashMap<>();
-    private static final SimpleTypeConverter typeConverter = new GoConfigFieldTypeConverter();
+    private static final Map<Field, Boolean> implicits = new ConcurrentHashMap<>();
 
     private final Element e;
     private final T instance;
@@ -49,7 +47,7 @@ public class GoConfigFieldLoader<T> {
     private final ConfigElementImplementationRegistry registry;
 
     public static <T> GoConfigFieldLoader<T> fieldParser(Element e, T instance, Field field, ConfigCache configCache, final ConfigElementImplementationRegistry registry,
-                                                             ConfigReferenceElements configReferenceElements) {
+                                                         ConfigReferenceElements configReferenceElements) {
         return new GoConfigFieldLoader<>(e, instance, field, configCache, registry, configReferenceElements);
     }
 
@@ -93,11 +91,8 @@ public class GoConfigFieldLoader<T> {
     }
 
     private boolean isImplicitCollection() {
-        if (!implicits.containsKey(field)) {
-            implicits.put(field, ConfigCache.isAnnotationPresent(field, ConfigSubtag.class)
-                    && GoConfigClassLoader.isImplicitCollection(field.getType()));
-        }
-        return implicits.get(field);
+        return implicits.computeIfAbsent(field,
+                f -> ConfigCache.isAnnotationPresent(f, ConfigSubtag.class) && GoConfigClassLoader.isImplicitCollection(f.getType()));
     }
 
     private void setValue(Object val) {
@@ -109,13 +104,12 @@ public class GoConfigFieldLoader<T> {
                     field.set(instance, constructor.newInstance(val));
                 }
             } else if (val != null) {
-                Object convertedValue = typeConverter.convertIfNecessary(val, field.getType());
-                field.set(instance, convertedValue);
+                field.set(instance, GoConfigFieldTypeConverter.forThread().convertIfNecessary(val, field.getType()));
             }
         } catch (IllegalAccessException e) {
             throw bomb("Error setting configField: " + field.getName(), e);
         } catch (TypeMismatchException e) {
-            final String message = format("Could not set value [{0}] on Field [{1}] of type [{2}] ",
+            final String message = format("Could not set value [{0}] on field [{1}] of type [{2}] ",
                     val, field.getName(), field.getType());
             throw bomb(message, e);
         } catch (NoSuchMethodException e) {
diff --git a/config/config-server/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldLoaderTest.java b/config/config-server/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldLoaderTest.java
deleted file mode 100644
index 7d221b5f32..0000000000
--- a/config/config-server/src/test/java/com/thoughtworks/go/config/parser/GoConfigFieldLoaderTest.java
+++ /dev/null
@@ -1,39 +0,0 @@
-/*
- * Copyright 2023 Thoughtworks, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package com.thoughtworks.go.config.parser;
-
-import java.lang.reflect.Field;
-import java.lang.reflect.Modifier;
-
-import org.junit.jupiter.api.Test;
-
-import static org.hamcrest.Matchers.is;
-import static org.hamcrest.MatcherAssert.assertThat;
-
-public class GoConfigFieldLoaderTest {
-
-    @Test
-    public void shouldOnlyCreateOneInstanceOfSimpleTypeConverterAndUseIt() throws Exception {
-        Integer dummyValue = 0;
-        Field randomField = dummyValue.getClass().getDeclaredFields()[0]; // Random field because I can't mock java.lang.reflect.Field (Sachin)
-        GoConfigFieldLoader loader = GoConfigFieldLoader.fieldParser(null, null, randomField, null, null, new ConfigReferenceElements());
-        Field expectedField = loader.getClass().getDeclaredField("typeConverter");
-        int modifier = expectedField.getModifiers();
-        assertThat(Modifier.isStatic(modifier), is(true));
-        assertThat(Modifier.isFinal(modifier), is(true));
-    }
-}
-