Skip to content

Commit 64e321a

Browse files
Address Copilot code review suggestions
- Remove s += initstr on max-retries and parse-error paths in read() (accumulator should not include default when returning it) - Add dict key validation: throw if key is not a non-null String - Use explicit UTF-8 charset in parseFile() and sparams reading - Align sparams parsing with Python parse_params logic: try literalEval first if input looks like dict literal - Update test comment to reflect actual coverage - Remove unused list creation in testRoundTripSerialization() - Add test for non-string dict key validation All 50 tests pass.
1 parent 003540c commit 64e321a

2 files changed

Lines changed: 46 additions & 25 deletions

File tree

TestLiteralEval.java

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* Test suite for concoredocker.literalEval() recursive descent parser.
55
* Covers: dicts, lists, tuples, numbers, strings, booleans, None,
66
* nested structures, escape sequences, scientific notation,
7-
* toPythonLiteral serialization, and fractional simtime.
7+
* fractional simtime, and related round-trip parsing behavior.
88
*/
99
public class TestLiteralEval {
1010
static int passed = 0;
@@ -36,6 +36,7 @@ public static void main(String[] args) {
3636
testUnterminatedList();
3737
testUnterminatedDict();
3838
testUnterminatedTuple();
39+
testNonStringDictKey();
3940

4041
System.out.println("\n=== Results: " + passed + " passed, " + failed + " failed out of " + (passed + failed) + " tests ===");
4142
if (failed > 0) {
@@ -198,15 +199,7 @@ static void testFractionalSimtime() {
198199
// --- Round-trip serialization tests ---
199200

200201
static void testRoundTripSerialization() {
201-
// Serialize a list with mixed types, then re-parse and verify
202-
List<Object> original = new ArrayList<>();
203-
original.add(1);
204-
original.add(2.5);
205-
original.add(true);
206-
original.add(false);
207-
original.add(null);
208-
original.add("hello");
209-
202+
// Conceptually: a list with mixed types [1, 2.5, true, false, null, "hello"]
210203
// Use reflection-free approach: build the Python literal manually
211204
// and verify round-trip through literalEval
212205
String serialized = "[1, 2.5, True, False, None, 'hello']";
@@ -263,4 +256,15 @@ static void testUnterminatedTuple() {
263256
check("unterminated tuple throws", true, e.getMessage().contains("Unterminated tuple"));
264257
}
265258
}
259+
260+
static void testNonStringDictKey() {
261+
// Dict keys must be strings - numeric keys should throw
262+
try {
263+
concoredocker.literalEval("{123: 'value'}");
264+
System.out.println("FAIL: numeric dict key should throw");
265+
failed++;
266+
} catch (IllegalArgumentException e) {
267+
check("numeric dict key throws", true, e.getMessage().contains("Dict keys must be non-null strings"));
268+
}
269+
}
266270
}

concoredocker.java

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -41,25 +41,39 @@ public static void main(String[] args) {
4141
}
4242

4343
try {
44-
String sparams = new String(Files.readAllBytes(Paths.get(inpath + "1/concore.params")));
44+
String sparams = new String(Files.readAllBytes(Paths.get(inpath + "1/concore.params")), java.nio.charset.StandardCharsets.UTF_8);
4545
if (sparams.length() > 0 && sparams.charAt(0) == '"') { // windows keeps "" need to remove
4646
sparams = sparams.substring(1);
4747
sparams = sparams.substring(0, sparams.indexOf('"'));
4848
}
49-
if (!sparams.equals("{")) {
49+
// Try parsing as dict literal first (matches Python parse_params logic)
50+
sparams = sparams.trim();
51+
if (sparams.startsWith("{") && sparams.endsWith("}")) {
52+
try {
53+
Object parsed = literalEval(sparams);
54+
if (parsed instanceof Map) {
55+
@SuppressWarnings("unchecked")
56+
Map<String, Object> parsedMap = (Map<String, Object>) parsed;
57+
params = parsedMap;
58+
}
59+
} catch (Exception e) {
60+
System.out.println("bad params: " + sparams);
61+
}
62+
} else if (!sparams.isEmpty()) {
63+
// Fallback: convert key=value,key=value format to dict
5064
System.out.println("converting sparams: " + sparams);
5165
sparams = "{'" + sparams.replaceAll(",", ",'").replaceAll("=", "':").replaceAll(" ", "") + "}";
5266
System.out.println("converted sparams: " + sparams);
53-
}
54-
try {
55-
Object parsed = literalEval(sparams);
56-
if (parsed instanceof Map) {
57-
@SuppressWarnings("unchecked")
58-
Map<String, Object> parsedMap = (Map<String, Object>) parsed;
59-
params = parsedMap;
67+
try {
68+
Object parsed = literalEval(sparams);
69+
if (parsed instanceof Map) {
70+
@SuppressWarnings("unchecked")
71+
Map<String, Object> parsedMap = (Map<String, Object>) parsed;
72+
params = parsedMap;
73+
}
74+
} catch (Exception e) {
75+
System.out.println("bad params: " + sparams);
6076
}
61-
} catch (Exception e) {
62-
System.out.println("bad params: " + sparams);
6377
}
6478
} catch (IOException e) {
6579
params = new HashMap<>();
@@ -73,7 +87,7 @@ public static void main(String[] args) {
7387
* Returns empty map if file is empty or malformed (matches Python safe_literal_eval).
7488
*/
7589
private static Map<String, Object> parseFile(String filename) throws IOException {
76-
String content = new String(Files.readAllBytes(Paths.get(filename)));
90+
String content = new String(Files.readAllBytes(Paths.get(filename)), java.nio.charset.StandardCharsets.UTF_8);
7791
content = content.trim();
7892
if (content.isEmpty()) {
7993
return new HashMap<>();
@@ -182,7 +196,6 @@ private static List<Object> read(int port, String name, String initstr) {
182196

183197
if (ins.length() == 0) {
184198
System.out.println("Max retries reached for " + filePath + ", using default value.");
185-
s += initstr;
186199
return defaultVal;
187200
}
188201

@@ -197,7 +210,6 @@ private static List<Object> read(int port, String name, String initstr) {
197210
} catch (Exception e) {
198211
System.out.println("Error parsing " + ins + ": " + e.getMessage());
199212
}
200-
s += initstr;
201213
return defaultVal;
202214
}
203215

@@ -404,7 +416,12 @@ Map<String, Object> parseDict() {
404416
pos++; // skip ':'
405417
skipWhitespace();
406418
Object value = parseExpression();
407-
map.put(key.toString(), value);
419+
if (!(key instanceof String)) {
420+
throw new IllegalArgumentException(
421+
"Dict keys must be non-null strings, but got: "
422+
+ (key == null ? "null" : key.getClass().getSimpleName()));
423+
}
424+
map.put((String) key, value);
408425
skipWhitespace();
409426
if (pos >= input.length()) {
410427
throw new IllegalArgumentException("Unterminated dict: missing '}'");

0 commit comments

Comments
 (0)