Copilot commented on code in PR #10513:
URL: https://github.com/apache/rocketmq/pull/10513#discussion_r3410842445
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/SerializeType.java:
##########
@@ -21,19 +21,17 @@ public enum SerializeType {
JSON((byte) 0),
ROCKETMQ((byte) 1);
+ private static final SerializeType[] BY_CODE = {JSON, ROCKETMQ};
+
private byte code;
SerializeType(byte code) {
this.code = code;
}
public static SerializeType valueOf(byte code) {
- for (SerializeType serializeType : SerializeType.values()) {
- if (serializeType.getCode() == code) {
- return serializeType;
- }
- }
- return null;
+ int idx = code & 0xFF;
+ return idx < BY_CODE.length ? BY_CODE[idx] : null;
Review Comment:
`BY_CODE` is manually ordered and implicitly assumes codes are contiguous
and start at 0 (and that JSON/ROCKETMQ codes won’t change). This is fragile: if
a new serialize type gets a non-contiguous code or an existing code changes,
the lookup can silently return the wrong enum or `null`. Prefer building
`BY_CODE` from `values()` (like `LanguageCode` does) and indexing by `getCode()
& 0xFF`, optionally sizing by max code.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/SerializeType.java:
##########
@@ -21,19 +21,17 @@ public enum SerializeType {
JSON((byte) 0),
ROCKETMQ((byte) 1);
+ private static final SerializeType[] BY_CODE = {JSON, ROCKETMQ};
Review Comment:
`BY_CODE` is manually ordered and implicitly assumes codes are contiguous
and start at 0 (and that JSON/ROCKETMQ codes won’t change). This is fragile: if
a new serialize type gets a non-contiguous code or an existing code changes,
the lookup can silently return the wrong enum or `null`. Prefer building
`BY_CODE` from `values()` (like `LanguageCode` does) and indexing by `getCode()
& 0xFF`, optionally sizing by max code.
##########
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/LanguageCode.java:
##########
@@ -38,19 +38,28 @@ public enum LanguageCode {
RUST((byte) 12),
NODE_JS((byte) 13);
+ private static final LanguageCode[] BY_CODE;
+ static {
+ LanguageCode[] all = values();
+ int max = 0;
+ for (LanguageCode lc : all) {
+ max = Math.max(max, lc.code & 0xFF);
+ }
+ BY_CODE = new LanguageCode[max + 1];
+ for (LanguageCode lc : all) {
+ BY_CODE[lc.code & 0xFF] = lc;
+ }
Review Comment:
This will silently overwrite entries if two enum constants are ever assigned
the same `code` value, changing behavior vs. the old implementation (which
returned the first match during iteration). Add a duplicate check during
initialization (e.g., if `BY_CODE[idx] != null` then fail fast with an
exception) to prevent ambiguous protocol decoding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
