[
https://issues.apache.org/jira/browse/GROOVY-12273?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18105730#comment-18105730
]
ASF GitHub Bot commented on GROOVY-12273:
-----------------------------------------
Copilot commented on code in PR #2810:
URL: https://github.com/apache/groovy/pull/2810#discussion_r3809063798
##########
src/main/java/groovy/util/ConfigObject.java:
##########
@@ -251,20 +253,20 @@ private void writeConfig(String prefix, ConfigObject map,
BufferedWriter out, in
if (configSize == 1 ||
DefaultGroovyMethods.asBoolean(dotsInKeys)) {
if (firstSize == 1 && firstValue instanceof
ConfigObject) {
- key = KEYWORDS.contains(key) ?
FormatHelper.inspect(key) : key;
+ key = renderKey(key);
String writePrefix = prefix + key + "." + firstKey
+ ".";
writeConfig(writePrefix, (ConfigObject)
firstValue, out, tab, true);
Review Comment:
In the single-entry ConfigObject flattening branch, `firstKey` is appended
into `writePrefix` without being rendered. That means a nested key like `a
b`/`a.b` can still be written as source (and often won’t parse back), even
though other paths now use `renderKey`. Also, if the rendered outer key starts
with a quote and `prefix` is empty, `writePrefix` will start with a string
literal and assignments emitted under it need a receiver (`this.`) to parse.
##########
src/main/java/groovy/util/ConfigObject.java:
##########
@@ -273,30 +275,100 @@ private void writeConfig(String prefix, ConfigObject
map, BufferedWriter out, in
}
}
} else {
- writeValue(key, space, prefix, v, out);
+ writeValue(renderKey(key), space, prefix, v, out);
}
}
}
- private static void writeValue(String key, String space, String prefix,
Object value, BufferedWriter out) throws IOException {
-// key = key.indexOf('.') > -1 ? InvokerHelper.inspect(key) : key;
- boolean isKeyword = KEYWORDS.contains(key);
- key = isKeyword ? FormatHelper.inspect(key) : key;
-
- if (!StringGroovyMethods.asBoolean(prefix) && isKeyword) prefix =
"this.";
-
out.append(space).append(prefix).append(key).append('=').append(FormatHelper.inspect(value));
+ /**
+ * Writes one entry, given a key path whose components have already been
rendered.
+ *
+ * @param keyPath the rendered key path, such as {@code foo} or {@code
foo.'a b'}
+ */
+ private static void writeValue(String keyPath, String space, String
prefix, Object value, BufferedWriter out) throws IOException {
+ // A quoted key cannot open a statement on its own, so it needs a
receiver, exactly as a
+ // keyword key has always done.
+ if (!StringGroovyMethods.asBoolean(prefix) && keyPath.startsWith("'"))
prefix = "this.";
+
out.append(space).append(prefix).append(keyPath).append('=').append(renderValue(value));
out.newLine();
}
private void writeNode(String key, String space, int tab, ConfigObject
value, BufferedWriter out) throws IOException {
- key = KEYWORDS.contains(key) ? FormatHelper.inspect(key) : key;
- out.append(space).append(key).append(" {");
+ out.append(space).append(renderKey(key)).append(" {");
out.newLine();
writeConfig("", value, out, tab + 1, true);
out.append(space).append('}');
out.newLine();
}
+ /**
+ * Renders a key as it must appear in the written configuration: bare when
it is a plain
+ * identifier, and as a quoted literal otherwise. A key which is not an
identifier would
+ * otherwise be written as though it were source, and read back as
whatever it happened to
+ * parse as.
+ *
+ * @param key the key to render
+ * @return the key as it should be written
+ */
+ private static String renderKey(String key) {
+ return isIdentifier(key) ? key : FormatHelper.inspect(key);
+ }
+
+ private static boolean isIdentifier(String key) {
+ if (key == null || key.isEmpty() || KEYWORDS.contains(key)) return
false;
+ if (!Character.isJavaIdentifierStart(key.charAt(0))) return false;
+ for (int i = 1, n = key.length(); i < n; i += 1) {
+ if (!Character.isJavaIdentifierPart(key.charAt(i))) return false;
+ }
+ return true;
+ }
+
+ /**
+ * Renders a value as a literal which reads back as the same data.
+ *
+ * @param value the value to render
+ * @return the value as it should be written
+ */
+ private static String renderValue(Object value) {
+ return FormatHelper.inspect(asWritableData(value));
+ }
+
+ /**
+ * Converts a value into something {@link FormatHelper#inspect} renders as
inert data.
+ * <p>
+ * A {@link CharSequence} which is not a {@code String} is rendered as a
double quoted
+ * literal, in which a dollar is live, so its text is carried over to a
{@code String} and
+ * rendered single quoted instead. A value of any other type without a
literal form would be
+ * written as a bare {@code toString()}, which is not data at all, so its
text is carried
+ * over in the same way. Numbers and booleans already write as themselves.
+ *
+ * @param value the value to convert
+ * @return a value whose rendering is data
+ */
+ private static Object asWritableData(Object value) {
+ if (value == null || value instanceof String || value instanceof
Number || value instanceof Boolean) {
+ return value;
+ }
+ if (value instanceof CharSequence) {
+ return value.toString();
+ }
+ if (value instanceof Map<?, ?> map) {
+ Map<Object, Object> converted = new LinkedHashMap<>(map.size());
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
+ converted.put(asWritableData(entry.getKey()),
asWritableData(entry.getValue()));
+ }
+ return converted;
+ }
+ if (value instanceof Collection<?> collection) {
+ List<Object> converted = new ArrayList<>(collection.size());
+ for (Object element : collection) {
+ converted.add(asWritableData(element));
+ }
+ return converted;
+ }
+ return value.toString();
+ }
Review Comment:
`asWritableData` currently treats Java arrays as “other type” and falls back
to `value.toString()`. For arrays this is the JVM identity form (e.g.
`[Ljava.lang.String;@...` / `[I@...`), which loses the actual elements and
changes `writeTo` behavior vs `FormatHelper.inspect(array)` (which renders a
list-like literal). Consider converting arrays to a `List` and recursively
sanitizing their elements, similar to the `Collection` branch.
> ConfigObject.writeTo breaks its round-trip contract
> ---------------------------------------------------
>
> Key: GROOVY-12273
> URL: https://issues.apache.org/jira/browse/GROOVY-12273
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
