Skip to content

Commit 303dbad

Browse files
mengweiericclaude
andcommitted
Address PR review: fix hash truncation, antlrVersion API, immutability, and tests
- Hash full 32-bit ints in grammarHash to avoid collisions with ANTLR 4.13.2 ATN serialization - Use RuntimeMetaData.getRuntimeVersion() instead of unreliable JAR manifest lookup - Make GrammarBundle immutable with @value instead of @DaTa - Update THIRD-PARTY to reflect ANTLR 4.13.2 - Harden tests with JSON parsing and add antlrVersion assertion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 7dce44d commit 303dbad

5 files changed

Lines changed: 53 additions & 30 deletions

File tree

THIRD-PARTY

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -467,15 +467,15 @@ DAMAGE.
467467

468468
------
469469

470-
** ANTLR; version 4.7.1 -- https://github.com/antlr/antlr4
470+
** ANTLR; version 4.13.2 -- https://github.com/antlr/antlr4
471471
/*
472-
* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
472+
* Copyright (c) 2012-2024 The ANTLR Project. All rights reserved.
473473
* Use of this file is governed by the BSD 3-clause license that
474474
* can be found in the LICENSE.txt file in the project root.
475475
*/
476476

477477
[The "BSD 3-clause license"]
478-
Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
478+
Copyright (c) 2012-2024 The ANTLR Project. All rights reserved.
479479

480480
Redistribution and use in source and binary forms, with or without
481481
modification, are permitted provided that the following conditions

plugin/src/test/java/org/opensearch/sql/plugin/rest/RestPPLGrammarActionTest.java

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import java.io.IOException;
1414
import java.util.concurrent.atomic.AtomicInteger;
15+
import org.json.JSONObject;
1516
import org.junit.Before;
1617
import org.junit.Test;
1718
import org.opensearch.common.io.stream.BytesStreamOutput;
@@ -67,16 +68,31 @@ public void testGetGrammar_ReturnsBundle() throws Exception {
6768
assertEquals("Should return 200 OK", RestStatus.OK, response.status());
6869

6970
String content = response.content().utf8ToString();
70-
assertTrue(content.contains("\"bundleVersion\":\"1.0\""));
71-
assertTrue(content.contains("\"antlrVersion\":"));
72-
assertTrue(content.contains("\"grammarHash\":\"sha256:"));
73-
assertTrue(content.contains("\"startRuleIndex\":0"));
74-
assertTrue(content.contains("\"lexerSerializedATN\":"));
75-
assertTrue(content.contains("\"parserSerializedATN\":"));
76-
assertTrue(content.contains("\"lexerRuleNames\":"));
77-
assertTrue(content.contains("\"parserRuleNames\":"));
78-
assertTrue(content.contains("\"literalNames\":"));
79-
assertTrue(content.contains("\"symbolicNames\":"));
71+
JSONObject json = new JSONObject(content);
72+
73+
// Identity & versioning
74+
assertEquals("1.0", json.getString("bundleVersion"));
75+
assertTrue(
76+
"antlrVersion should be a version string",
77+
json.getString("antlrVersion").matches("\\d+\\.\\d+.*"));
78+
assertTrue(
79+
"grammarHash should start with sha256:",
80+
json.getString("grammarHash").startsWith("sha256:"));
81+
assertEquals(0, json.getInt("startRuleIndex"));
82+
83+
// Lexer ATN & metadata (non-empty arrays)
84+
assertTrue(json.getJSONArray("lexerSerializedATN").length() > 0);
85+
assertTrue(json.getJSONArray("lexerRuleNames").length() > 0);
86+
assertTrue(json.getJSONArray("channelNames").length() > 0);
87+
assertTrue(json.getJSONArray("modeNames").length() > 0);
88+
89+
// Parser ATN & metadata (non-empty arrays)
90+
assertTrue(json.getJSONArray("parserSerializedATN").length() > 0);
91+
assertTrue(json.getJSONArray("parserRuleNames").length() > 0);
92+
93+
// Vocabulary (non-empty arrays)
94+
assertTrue(json.getJSONArray("literalNames").length() > 0);
95+
assertTrue(json.getJSONArray("symbolicNames").length() > 0);
8096
}
8197

8298
@Test

ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/GrammarBundle.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
package org.opensearch.sql.ppl.autocomplete;
77

88
import lombok.Builder;
9-
import lombok.Data;
109
import lombok.NonNull;
10+
import lombok.Value;
1111

1212
/** Serialized ANTLR grammar data served by {@code GET /_plugins/_ppl/_grammar}. */
13-
@Data
13+
@Value
1414
@Builder
1515
public class GrammarBundle {
1616

ppl/src/main/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilder.java

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,10 @@
1717

1818
/** Builds the {@link GrammarBundle} for the PPL language from the generated ANTLR lexer/parser. */
1919
public class PPLGrammarBundleBuilder {
20-
private static final String ANTLR_VERSION = getAntlrVersion();
20+
private static final String ANTLR_VERSION =
21+
org.antlr.v4.runtime.RuntimeMetaData.getRuntimeVersion();
2122
private static final String BUNDLE_VERSION = "1.0";
2223

23-
private static String getAntlrVersion() {
24-
Package antlrPackage = org.antlr.v4.runtime.RuntimeMetaData.class.getPackage();
25-
String version = antlrPackage.getImplementationVersion();
26-
return version != null ? version : "unknown";
27-
}
28-
2924
public GrammarBundle build() {
3025
OpenSearchPPLLexer lexer = new OpenSearchPPLLexer(CharStreams.fromString(""));
3126
CommonTokenStream tokens = new CommonTokenStream(lexer);
@@ -62,14 +57,8 @@ public GrammarBundle build() {
6257
private static String computeGrammarHash(int[] lexerATN, int[] parserATN) {
6358
try {
6459
MessageDigest digest = MessageDigest.getInstance("SHA-256");
65-
for (int v : lexerATN) {
66-
digest.update((byte) (v >> 8));
67-
digest.update((byte) v);
68-
}
69-
for (int v : parserATN) {
70-
digest.update((byte) (v >> 8));
71-
digest.update((byte) v);
72-
}
60+
updateDigest(digest, lexerATN);
61+
updateDigest(digest, parserATN);
7362
digest.update(ANTLR_VERSION.getBytes(StandardCharsets.UTF_8));
7463
byte[] hash = digest.digest();
7564
// Output is always "sha256:" (7 chars) + 64 hex chars = 71 chars.
@@ -83,4 +72,13 @@ private static String computeGrammarHash(int[] lexerATN, int[] parserATN) {
8372
throw new IllegalStateException("SHA-256 not available", e);
8473
}
8574
}
75+
76+
private static void updateDigest(MessageDigest digest, int[] data) {
77+
for (int v : data) {
78+
digest.update((byte) (v >>> 24));
79+
digest.update((byte) (v >>> 16));
80+
digest.update((byte) (v >>> 8));
81+
digest.update((byte) (v));
82+
}
83+
}
8684
}

ppl/src/test/java/org/opensearch/sql/ppl/autocomplete/PPLGrammarBundleBuilderTest.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,15 @@ public void bundleVersionIsSet() {
3131
assertEquals("1.0", bundle.getBundleVersion());
3232
}
3333

34+
@Test
35+
public void antlrVersionIsSet() {
36+
String version = bundle.getAntlrVersion();
37+
assertNotNull("antlrVersion should not be null", version);
38+
assertTrue(
39+
"antlrVersion should look like a version string, got: " + version,
40+
version.matches("\\d+\\.\\d+.*"));
41+
}
42+
3443
@Test
3544
public void grammarHashHasExpectedFormat() {
3645
String hash = bundle.getGrammarHash();

0 commit comments

Comments
 (0)