From 667e3e3011a80656c96c7c8a207f1a27d9af2da9 Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Wed, 29 Jul 2026 12:13:20 +0200 Subject: [PATCH 1/2] Make message clear() O(1) with presence-guarded getters parseFrom() begins with clear(), which physically reset every field: 13-23% of deserialization time in CPU profiles (a generated MessageMetadata.clear() executed ~60 field stores per parse). clear() now only resets presence state (bitfields, oneof cases, repeated counts, _cachedSize, _parsedBuffer): - Presence bits are tracked for all non-repeated, non-oneof fields; proto3 implicit-presence fields get an internal bit (no has() method) marking "written since last clear". - Getters return the field default while presence is unset, so stale values from previous parse/set cycles are unobservable. - String/bytes fields invalidate their cached decoded value in parse() instead of clear(), paying the cost only for fields present on the wire. - equals()/hashCode() mask bitfields to explicit-presence bits so that proto3 "set to default" remains indistinguishable from "never set". - copyFrom() guards implicit-presence fields on the source's presence bit (protobuf merge semantics). - Repeated message and map clear() reset only the element count; the parse path adds elements without clearing (parseFrom clears the child itself), while the public addX() clears reused pooled instances. This also fixes two latent bugs, now covered by InstanceReuseTest: - clearX() on a message field cleared the presence bit before the has()-guarded child clear, leaving a stale child reachable via getX(). - copyFrom() overwrote target fields with unset implicit-presence source values. Deserialization throughput (JMH, Apple M-series): AddressBook 19.4 -> 26.3 ops/us (+35%) Pulsar MessageMetadata 14.3 -> 17.4 ops/us (+22%) Simple + readString 38.6 -> 51.3 ops/us (+33%) Pulsar BaseCommand 33.2 -> 34.2 ops/us (+3%) --- .../generator/LightProtoBooleanField.java | 27 +- .../generator/LightProtoBytesField.java | 18 +- .../generator/LightProtoEnumField.java | 19 +- .../lightproto/generator/LightProtoField.java | 35 ++- .../generator/LightProtoMapField.java | 44 +-- .../generator/LightProtoMessage.java | 44 ++- .../generator/LightProtoNumberField.java | 46 ++- .../LightProtoRepeatedBytesField.java | 10 +- .../LightProtoRepeatedMessageField.java | 21 +- .../LightProtoRepeatedStringField.java | 10 +- .../generator/LightProtoStringField.java | 24 +- .../lightproto/tests/InstanceReuseTest.java | 296 ++++++++++++++++++ 12 files changed, 490 insertions(+), 104 deletions(-) create mode 100644 tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java index 365fc82..05af7a5 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java @@ -27,17 +27,32 @@ public LightProtoBooleanField(ProtoFieldDescriptor field, int index) { public void getter(PrintWriter w) { w.format(" /** Returns the value of the {@code %s} field. */\n", field.getName()); w.format(" public %s %s() {\n", field.getJavaType(), Util.camelCase("is", ccName)); + w.format(" if (!(%s)) {\n", presenceCondition()); + w.format(" return %s;\n", defaultValueExpr()); + w.format(" }\n"); w.format(" return %s;\n", ccName); w.format(" }\n"); } @Override - public void clear(PrintWriter w) { + protected String defaultValueExpr() { if (field.isDefaultValueSet()) { - w.format("%s = %s;\n", ccName, field.getDefaultValueAsString()); - } else { - w.format("%s = false;\n", ccName); + return field.getDefaultValueAsString(); + } + return "false"; + } + + @Override + protected String cmpValueExpr(String qualifier) { + if (field.hasImplicitPresence()) { + return qualifier + Util.camelCase("is", ccName) + "()"; } + return qualifier + ccName; + } + + @Override + public void clear(PrintWriter w) { + // No value reset needed: the getter is guarded by the presence condition. } @Override @@ -65,12 +80,12 @@ public void parseTextFormat(PrintWriter w) { @Override public void equalsCode(PrintWriter w) { - w.format("if (%s != _other.%s) return false;\n", ccName, ccName); + w.format("if (%s != %s) return false;\n", cmpValueExpr(""), cmpValueExpr("_other.")); } @Override public void hashCodeCode(PrintWriter w) { - w.format("_h = 31 * _h + (%s ? 1231 : 1237);\n", ccName); + w.format("_h = 31 * _h + (%s ? 1231 : 1237);\n", cmpValueExpr("")); } @Override diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java index c32b240..ee6a741 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java @@ -32,6 +32,9 @@ public void declaration(PrintWriter w) { @Override public void parse(PrintWriter w) { + // Invalidate any stale buffer reference from a previous parse or set: clear() + // no longer resets it, so it must be dropped when the field is on the wire. + w.format("%s = null;\n", ccName); w.format("_%sLen = LightProtoCodec.readVarInt(_buffer);\n", ccName); w.format("_%sIdx = _buffer.readerIndex();\n", ccName); w.format("_buffer.skipBytes(_%sLen);\n", ccName); @@ -65,13 +68,13 @@ public void setter(PrintWriter w, String enclosingType) { public void getter(PrintWriter w) { w.format("/** Returns the size in bytes of the {@code %s} field. */\n", field.getName()); w.format("public int %s() {\n", Util.camelCase("get", ccName, "size")); - w.format(" if (_%sLen < 0) { return 0; }\n", ccName); + w.format(" if (!(%s)) { return 0; }\n", presenceCondition()); w.format(" return _%sLen;\n", ccName); w.format("}\n"); w.format("/** Returns the {@code %s} field as a byte array. */\n", field.getName()); w.format("public byte[] %s() {\n", Util.camelCase("get", ccName)); - w.format(" if (_%sLen < 0) { return new byte[0]; }\n", ccName); + w.format(" if (!(%s)) { return new byte[0]; }\n", presenceCondition()); w.format(" io.netty.buffer.ByteBuf _b = %s();\n", Util.camelCase("get", ccName, "slice")); w.format(" byte[] res = new byte[_b.readableBytes()];\n"); w.format(" _b.getBytes(0, res);\n"); @@ -80,7 +83,7 @@ public void getter(PrintWriter w) { w.format("/** Returns the {@code %s} field as a ByteBuf slice. */\n", field.getName()); w.format("public io.netty.buffer.ByteBuf %s() {\n", Util.camelCase("get", ccName, "slice")); - w.format(" if (_%sLen < 0) { return io.netty.buffer.Unpooled.EMPTY_BUFFER; }\n", ccName); + w.format(" if (!(%s)) { return io.netty.buffer.Unpooled.EMPTY_BUFFER; }\n", presenceCondition()); w.format(" if (%s == null) {\n", ccName); w.format(" return _parsedBuffer.slice(_%sIdx, _%sLen);\n", ccName, ccName); w.format(" } else {\n"); @@ -96,9 +99,8 @@ protected String nonDefaultCondition() { @Override public void clear(PrintWriter w) { - w.format("%s = null;\n", ccName); - w.format("_%sIdx = -1;\n", ccName); - w.format("_%sLen = -1;\n", ccName); + // No value reset needed: the getters are guarded by the presence condition, and + // parse() invalidates the stale buffer reference for fields present on the wire. } @Override @@ -159,7 +161,9 @@ public void serialize(PrintWriter w) { @Override public void materialize(PrintWriter w) { - w.format("if (_%sIdx >= 0) {\n", ccName); + // The presence guard is required: an absent field may hold a stale buffer + // index from an earlier parse of a buffer that has since been released. + w.format("if ((%s) && _%sIdx >= 0) {\n", presenceCondition(), ccName); w.format(" byte[] _tmp = new byte[_%sLen];\n", ccName); w.format(" _parsedBuffer.getBytes(_%sIdx, _tmp);\n", ccName); w.format(" %s = io.netty.buffer.Unpooled.wrappedBuffer(_tmp);\n", ccName); diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java index f02ada8..abf44f4 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java @@ -36,17 +36,24 @@ public void declaration(PrintWriter w) { public void getter(PrintWriter w) { w.format(" /** Returns the value of the {@code %s} field. */\n", field.getName()); w.format(" public %s %s() {\n", field.getJavaType(), Util.camelCase("get", field.getName())); + w.format(" if (!(%s)) {\n", presenceCondition()); + w.format(" return %s;\n", defaultValueExpr()); + w.format(" }\n"); w.format(" return %s;\n", ccName); w.format(" }\n"); } @Override - public void clear(PrintWriter w) { + protected String defaultValueExpr() { if (field.isDefaultValueSet()) { - w.format("%s = %s;\n", ccName, field.getDefaultValueAsString()); - } else { - w.format("%s = %s.valueOf(0);\n", ccName, field.getJavaType()); + return field.getDefaultValueAsString(); } + return String.format("%s.valueOf(0)", field.getJavaType()); + } + + @Override + public void clear(PrintWriter w) { + // No value reset needed: the getter is guarded by the presence condition. } @Override @@ -86,12 +93,12 @@ public void parseTextFormat(PrintWriter w) { @Override public void equalsCode(PrintWriter w) { - w.format("if (%s != _other.%s) return false;\n", ccName, ccName); + w.format("if (%s != %s) return false;\n", cmpValueExpr(""), cmpValueExpr("_other.")); } @Override public void hashCodeCode(PrintWriter w) { - w.format("_h = 31 * _h + %s.getValue();\n", ccName); + w.format("_h = 31 * _h + %s.getValue();\n", cmpValueExpr("")); } @Override diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java index c6d13c2..b095891 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java @@ -91,7 +91,11 @@ public void tags(PrintWriter w) { w.format(" private static final int %s = %d;\n", fieldNumber(), field.getNumber()); w.format(" private static final int %s = (%s << LightProtoCodec.TAG_TYPE_BITS) | %s;\n", tagName(), fieldNumber(), typeTag()); w.format(" private static final int %s_SIZE = LightProtoCodec.computeVarIntSize(%s);\n", tagName(), tagName()); - if (!field.isRepeated() && !field.isOneofMember() && !field.hasImplicitPresence()) { + // Presence bits are tracked for all non-repeated, non-oneof fields. For proto3 + // implicit-presence fields the bit is internal (no has() method): it marks that the + // field was written since the last clear(), so clear() does not need to physically + // reset field values. + if (!field.isRepeated() && !field.isOneofMember()) { w.format(" private static final int %s = 1 << (%d %% 32);\n", fieldMask(), index); } } @@ -115,16 +119,16 @@ public void has(PrintWriter w) { public void fieldClear(PrintWriter w, String enclosingType) { w.format(" /** Clear the {@code %s} field. */\n", field.getName()); w.format(" public %s %s() {\n", enclosingType, Util.camelCase("clear", field.getName())); + // The value clear must run while presence is still set: message-field clear() + // is guarded by has(), so clearing the presence first would leave a stale child. if (field.isOneofMember()) { w.format(" if (_%sCase == %s) {\n", Util.camelCase(field.getOneofName()), fieldNumber()); - w.format(" _%sCase = 0;\n", Util.camelCase(field.getOneofName())); clear(w); + w.format(" _%sCase = 0;\n", Util.camelCase(field.getOneofName())); w.format(" }\n"); - } else if (!field.hasImplicitPresence()) { - w.format(" _bitField%d &= ~%s;\n", bitFieldIndex(), fieldMask()); - clear(w); } else { clear(w); + w.format(" _bitField%d &= ~%s;\n", bitFieldIndex(), fieldMask()); } w.format(" return this;\n"); w.format(" }\n"); @@ -219,13 +223,24 @@ protected int bitFieldIndex() { return index / 32; } - protected void writeSetPresence(PrintWriter w) { - if (field.hasImplicitPresence()) { - return; // No presence tracking for proto3 implicit presence + /** + * Returns the Java expression that is true when this (non-repeated) field is present: + * the oneof case check for oneof members, otherwise the presence bit check. Getters use + * this to return defaults for absent fields, since clear() no longer resets field values. + */ + protected String presenceCondition() { + if (field.isOneofMember()) { + return String.format("_%sCase == %s", Util.camelCase(field.getOneofName()), fieldNumber()); } + return String.format("(_bitField%d & %s) != 0", bitFieldIndex(), fieldMask()); + } + + protected void writeSetPresence(PrintWriter w) { if (field.isOneofMember()) { w.format(" _%sCase = %s;\n", Util.camelCase(field.getOneofName()), fieldNumber()); } else { + // Also set for proto3 implicit presence: the internal bit guards against + // stale values surviving an O(1) clear(). w.format(" _bitField%d |= %s;\n", bitFieldIndex(), fieldMask()); } } @@ -239,7 +254,9 @@ public String serializeCondition() { return null; } if (field.hasImplicitPresence()) { - return nonDefaultCondition(); + // The presence bit guards against stale values after an O(1) clear(); + // the non-default check preserves proto3 semantics (defaults not emitted). + return presenceCondition() + " && " + nonDefaultCondition(); } return Util.camelCase("has", field.getName()) + "()"; } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMapField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMapField.java index cdb2d38..9293b55 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMapField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMapField.java @@ -384,6 +384,15 @@ public void parse(PrintWriter w) { // Ensure capacity before parsing (message values parse directly into the array) w.format("_ensure%sCapacity();\n", Util.camelCaseFirstUpper(ccName)); + if (isMessageValue()) { + // Reset the pooled slot up front: if the entry's value field is absent from + // the wire, the slot must yield defaults, not data from a previous cycle. + w.format("if (_%sValues[_%sCount] == null) {\n", ccName, ccName); + w.format(" _%sValues[_%sCount] = new %s();\n", ccName, ccName, valueField.getJavaType()); + w.format("} else {\n"); + w.format(" _%sValues[_%sCount].clear();\n", ccName, ccName); + w.format("}\n"); + } // Parse entry fields w.format("while (_buffer.readerIndex() < _%sEntryEnd) {\n", ccName); @@ -450,10 +459,8 @@ private void generateValueTempParse(PrintWriter w) { w.format(" _%sValueIdx = _buffer.readerIndex();\n", ccName); w.format(" _buffer.skipBytes(_%sValueLen);\n", ccName); } else if (isMessageValue()) { + // The slot was already allocated and reset right after _ensureCapacity(). w.format(" int _%sMsgSize = LightProtoCodec.readVarInt(_buffer);\n", ccName); - w.format(" if (_%sValues[_%sCount] == null) {\n", ccName, ccName); - w.format(" _%sValues[_%sCount] = new %s();\n", ccName, ccName, valueField.getJavaType()); - w.format(" }\n"); w.format(" _%sValues[_%sCount].parseFrom(_buffer, _%sMsgSize);\n", ccName, ccName, ccName); } else { w.format(" _%sValue = %s;\n", ccName, LightProtoNumberField.parseNumber(valueField)); @@ -970,33 +977,10 @@ public void copy(PrintWriter w) { @Override public void clear(PrintWriter w) { - if (isStringKey()) { - w.format("for (int _i = 0; _i < _%sCount; _i++) {\n", ccName); - w.format(" LightProtoCodec.StringHolder _sh = _%sKeys[_i];\n", ccName); - w.format(" _sh.s = null;\n"); - w.format(" _sh.idx = -1;\n"); - w.format(" _sh.len = -1;\n"); - w.format("}\n"); - } - if (isStringValue()) { - w.format("for (int _i = 0; _i < _%sCount; _i++) {\n", ccName); - w.format(" LightProtoCodec.StringHolder _sh = _%sValues[_i];\n", ccName); - w.format(" _sh.s = null;\n"); - w.format(" _sh.idx = -1;\n"); - w.format(" _sh.len = -1;\n"); - w.format("}\n"); - } else if (isBytesValue()) { - w.format("for (int _i = 0; _i < _%sCount; _i++) {\n", ccName); - w.format(" LightProtoCodec.BytesHolder _bh = _%sValues[_i];\n", ccName); - w.format(" _bh.b = null;\n"); - w.format(" _bh.idx = -1;\n"); - w.format(" _bh.len = -1;\n"); - w.format("}\n"); - } else if (isMessageValue()) { - w.format("for (int _i = 0; _i < _%sCount; _i++) {\n", ccName); - w.format(" _%sValues[_i].clear();\n", ccName); - w.format("}\n"); - } + // Entries beyond the count are unreachable. Both parse() and put() write the + // complete key/value holder state (including nulling stale s/b references), + // and message values are cleared by parseFrom()/put() when slots are reused, + // so no per-entry reset is needed here. w.format("_%sCount = 0;\n", ccName); w.format("_%sIndex = null;\n", ccName); } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java index 83356c4..9d645ed 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java @@ -150,7 +150,9 @@ private void generateParseFrom(PrintWriter w) { if (field.isOneofMember()) { w.format(" _%sCase = %s;\n", Util.camelCase(field.field.getOneofName()), field.fieldNumber()); - } else if (!field.field.hasImplicitPresence()) { + } else { + // Also set for implicit-presence fields: the internal bit guards + // against stale values surviving the O(1) clear(). w.format(" _bitField%d |= %s;\n", field.bitFieldIndex(), field.fieldMask()); } } @@ -209,8 +211,11 @@ private void generateCopyFrom(PrintWriter w) { if (f.isRepeated()) { f.copy(w); } else if (f.field.hasImplicitPresence()) { - // Always copy for proto3 implicit presence — no has() to check + // No public has() — guard on the other message's internal presence bit, + // since its field value may be stale when the bit is unset. + w.format(" if ((_other._bitField%d & %s) != 0) {\n", f.bitFieldIndex(), f.fieldMask()); f.copy(w); + w.format(" }\n"); } else { w.format(" if (_other.%s()) {\n", Util.camelCase("has", f.ccName)); f.copy(w); @@ -481,9 +486,28 @@ private void generateBitFields(PrintWriter w) { } }); w.println(";"); + if (hasExplicitPresenceFieldsInWord(i)) { + // Bits of explicit-presence fields only: implicit-presence bits are an + // internal detail (set-since-clear) and must not affect equals/hashCode. + w.format("private static final int _PRESENCE_CMP_MASK%d = 0", i); + fields.forEach(f -> { + if (isExplicitPresence(f) && f.index() / 32 == idx) { + w.format(" | %s", f.fieldMask()); + } + }); + w.println(";"); + } } } + private static boolean isExplicitPresence(LightProtoField f) { + return !f.isRepeated() && !f.isOneofMember() && !f.field.hasImplicitPresence(); + } + + private boolean hasExplicitPresenceFieldsInWord(int word) { + return fields.stream().anyMatch(f -> isExplicitPresence(f) && f.index() / 32 == word); + } + private void generateCheckRequiredFields(PrintWriter w) { if (!hasRequiredFields()) { return; @@ -543,9 +567,14 @@ private void generateEquals(PrintWriter w) { w.format(" if (!(_o instanceof %s)) return false;\n", message.getName()); w.format(" %s _other = (%s) _o;\n", message.getName(), message.getName()); - // Fast-path: compare bitfields (explicit presence) + // Fast-path: compare bitfields, masked to explicit-presence fields. Implicit + // bits are excluded: "never set" and "explicitly set to the default" must + // compare equal under proto3 semantics. for (int i = 0; i < bitFieldsCount(); i++) { - w.format(" if (_bitField%d != _other._bitField%d) return false;\n", i, i); + if (hasExplicitPresenceFieldsInWord(i)) { + w.format(" if ((_bitField%d & _PRESENCE_CMP_MASK%d) != (_other._bitField%d & _PRESENCE_CMP_MASK%d)) return false;\n", + i, i, i, i); + } } // Compare oneof cases @@ -584,9 +613,12 @@ private void generateHashCode(PrintWriter w) { w.format(" @Override public int hashCode() {\n"); w.format(" int _h = 0;\n"); - // Include bitfields in hash + // Include bitfields in hash, masked to explicit-presence fields (implicit + // bits must not affect the hash — see equals()). for (int i = 0; i < bitFieldsCount(); i++) { - w.format(" _h = 31 * _h + _bitField%d;\n", i); + if (hasExplicitPresenceFieldsInWord(i)) { + w.format(" _h = 31 * _h + (_bitField%d & _PRESENCE_CMP_MASK%d);\n", i, i); + } } // Include oneof cases in hash diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java index da164db..f9883ce 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java @@ -168,10 +168,20 @@ public void tags(PrintWriter w) { public void getter(PrintWriter w) { w.format(" /** Returns the value of the {@code %s} field. */\n", field.getName()); w.format(" public %s %s() {\n", field.getJavaType(), Util.camelCase("get", field.getName())); + w.format(" if (!(%s)) {\n", presenceCondition()); + w.format(" return %s;\n", defaultValueExpr()); + w.format(" }\n"); w.format(" return %s;\n", ccName); w.format(" }\n"); } + protected String defaultValueExpr() { + if (field.isDefaultValueSet()) { + return field.getDefaultValueAsString(); + } + return "0"; + } + @Override public void parse(PrintWriter w) { w.format("%s = %s;\n", ccName, parseNumber(field)); @@ -279,11 +289,8 @@ public void copy(PrintWriter w) { @Override public void clear(PrintWriter w) { - if (field.isDefaultValueSet()) { - w.format("%s = %s;\n", ccName, field.getDefaultValueAsString()); - } else { - w.format("%s = 0;\n", ccName); - } + // No value reset needed: the getter returns the default while the presence + // bit (or oneof case) is unset, and parse/set always overwrite the value. } @Override @@ -313,30 +320,45 @@ public void serializedSize(PrintWriter w) { } } + /** + * Expression for this field's value in equals/hashCode. Implicit-presence fields may + * hold stale values when their internal presence bit is unset, so their effective + * value is read through the getter (which applies the presence guard). + */ + protected String cmpValueExpr(String qualifier) { + if (field.hasImplicitPresence()) { + return qualifier + Util.camelCase("get", field.getName()) + "()"; + } + return qualifier + ccName; + } + @Override public void equalsCode(PrintWriter w) { String type = field.getProtoType(); + String v = cmpValueExpr(""); + String o = cmpValueExpr("_other."); if (type.equals("float")) { - w.format("if (Float.floatToIntBits(%s) != Float.floatToIntBits(_other.%s)) return false;\n", ccName, ccName); + w.format("if (Float.floatToIntBits(%s) != Float.floatToIntBits(%s)) return false;\n", v, o); } else if (type.equals("double")) { - w.format("if (Double.doubleToLongBits(%s) != Double.doubleToLongBits(_other.%s)) return false;\n", ccName, ccName); + w.format("if (Double.doubleToLongBits(%s) != Double.doubleToLongBits(%s)) return false;\n", v, o); } else { - w.format("if (%s != _other.%s) return false;\n", ccName, ccName); + w.format("if (%s != %s) return false;\n", v, o); } } @Override public void hashCodeCode(PrintWriter w) { String type = field.getProtoType(); + String v = cmpValueExpr(""); if (type.equals("float")) { - w.format("_h = 31 * _h + Float.floatToIntBits(%s);\n", ccName); + w.format("_h = 31 * _h + Float.floatToIntBits(%s);\n", v); } else if (type.equals("double")) { - w.format("_h = 31 * _h + Long.hashCode(Double.doubleToLongBits(%s));\n", ccName); + w.format("_h = 31 * _h + Long.hashCode(Double.doubleToLongBits(%s));\n", v); } else if (type.equals("int64") || type.equals("uint64") || type.equals("sint64") || type.equals("fixed64") || type.equals("sfixed64")) { - w.format("_h = 31 * _h + Long.hashCode(%s);\n", ccName); + w.format("_h = 31 * _h + Long.hashCode(%s);\n", v); } else { - w.format("_h = 31 * _h + %s;\n", ccName); + w.format("_h = 31 * _h + %s;\n", v); } } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedBytesField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedBytesField.java index 12bd124..0edf064 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedBytesField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedBytesField.java @@ -37,6 +37,8 @@ public void declaration(PrintWriter w) { @Override public void parse(PrintWriter w) { w.format("LightProtoCodec.BytesHolder _%sBh = _%sBytesHolder();\n", ccName, Util.camelCase("new", singularName)); + // The holder may be pooled from a previous parse: drop its stale buffer reference. + w.format("_%sBh.b = null;\n", ccName); w.format("_%sBh.len = LightProtoCodec.readVarInt(_buffer);\n", ccName); w.format("_%sBh.idx = _buffer.readerIndex();\n", ccName); w.format("_buffer.skipBytes(_%sBh.len);\n", ccName); @@ -205,12 +207,8 @@ public void serializedSize(PrintWriter w) { @Override public void clear(PrintWriter w) { - w.format("for (int i = 0; i < _%sCount; i++) {\n", pluralName); - w.format(" LightProtoCodec.BytesHolder _bh = %s[i];\n", pluralName); - w.format(" _bh.b = null;\n"); - w.format(" _bh.idx = -1;\n"); - w.format(" _bh.len = -1;\n"); - w.format("}\n"); + // Holders beyond the count are unreachable, and both parse() and add() write + // the complete holder state, so no per-element reset is needed. w.format("_%sCount = 0;\n", pluralName); } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedMessageField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedMessageField.java index 513c720..8b8b584 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedMessageField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedMessageField.java @@ -65,7 +65,11 @@ public void getter(PrintWriter w) { @Override public void parse(PrintWriter w) { w.format("int _%sSize = LightProtoCodec.readVarInt(_buffer);\n", ccName); - w.format("%s().parseFrom(_buffer, _%sSize);\n", Util.camelCase("add", singularName), ccName); + w.format("%s().parseFrom(_buffer, _%sSize);\n", addForParseName(), ccName); + } + + private String addForParseName() { + return "_" + Util.camelCase("add", singularName, "for", "parse"); } @Override @@ -129,6 +133,15 @@ public void copy(PrintWriter w) { public void setter(PrintWriter w, String enclosingType) { w.format("/** Adds a new element to the {@code %s} list, returning the sub-message for population. */\n", field.getName()); w.format("public %s %s() {\n", field.getJavaType(), Util.camelCase("add", singularName)); + w.format(" %s _item = %s();\n", field.getJavaType(), addForParseName()); + // clear() only resets the count, so a pooled instance from a previous cycle + // must be cleared before being handed out for population. + w.format(" _item.clear();\n"); + w.format(" _cachedSize = -1;\n"); + w.format(" return _item;\n"); + w.format("}\n"); + + w.format("private %s %s() {\n", field.getJavaType(), addForParseName()); w.format(" if (%s == null) {\n", pluralName); w.format(" %s = new %s[4];\n", pluralName, field.getJavaType()); w.format(" }\n"); @@ -138,7 +151,6 @@ public void setter(PrintWriter w, String enclosingType) { w.format(" if (%s[_%sCount] == null) {\n", pluralName, pluralName); w.format(" %s[_%sCount] = new %s();\n", pluralName, pluralName, field.getJavaType()); w.format(" }\n"); - w.format(" _cachedSize = -1;\n"); w.format(" return %s[_%sCount++];\n", pluralName, pluralName); w.format("}\n"); @@ -168,9 +180,8 @@ public void serializedSize(PrintWriter w) { @Override public void clear(PrintWriter w) { - w.format("for (int i = 0; i < _%sCount; i++) {\n", pluralName); - w.format(" %s[i].clear();\n", pluralName); - w.format("}\n"); + // Pooled elements are NOT cleared here: parseFrom() clears them itself when + // reused by parsing, and addX() clears them when handed out for population. w.format("_%sCount = 0;\n", pluralName); } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedStringField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedStringField.java index eae88d4..7a5ae8d 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedStringField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoRepeatedStringField.java @@ -37,6 +37,8 @@ public void declaration(PrintWriter w) { @Override public void parse(PrintWriter w) { w.format("LightProtoCodec.StringHolder _%sSh = _%sStringHolder();\n", ccName, Util.camelCase("new", singularName)); + // The holder may be pooled from a previous parse: drop its stale decoded String. + w.format("_%sSh.s = null;\n", ccName); w.format("_%sSh.len = LightProtoCodec.readVarInt(_buffer);\n", ccName); w.format("_%sSh.idx = _buffer.readerIndex();\n", ccName); w.format("_buffer.skipBytes(_%sSh.len);\n", ccName); @@ -204,12 +206,8 @@ public void serializedSize(PrintWriter w) { @Override public void clear(PrintWriter w) { - w.format("for (int i = 0; i < _%sCount; i++) {\n", pluralName); - w.format(" LightProtoCodec.StringHolder _sh = %s[i];\n", pluralName); - w.format(" _sh.s = null;\n"); - w.format(" _sh.idx = -1;\n"); - w.format(" _sh.len = -1;\n"); - w.format("}\n"); + // Holders beyond the count are unreachable, and both parse() and add() write + // the complete holder state, so no per-element reset is needed. w.format("_%sCount = 0;\n", pluralName); } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java index a02a1d1..53b3113 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java @@ -58,15 +58,13 @@ public void copy(PrintWriter w) { public void getter(PrintWriter w) { w.format("/** Returns the value of the {@code %s} field. */\n", field.getName()); w.format("public %s %s() {\n", field.getJavaType(), Util.camelCase("get", field.getName())); - if (field.hasImplicitPresence()) { - w.format(" if (_%sBufferLen < 0) {\n", ccName); - w.format(" return \"\";\n"); - w.format(" }\n"); - } else if (!field.isDefaultValueSet()) { - w.format(" if (!%s()) {\n", Util.camelCase("has", ccName)); + w.format(" if (!(%s)) {\n", presenceCondition()); + if (field.isDefaultValueSet()) { + w.format(" return \"%s\";\n", field.getDefaultValue()); + } else { w.format(" return \"\";\n"); - w.format(" }\n"); } + w.format(" }\n"); w.format(" if (%s == null) {\n", camelCase(field.getName())); w.format(" %s = LightProtoCodec.readString(_parsedBuffer, _%sBufferIdx, _%sBufferLen);\n", ccName, ccName, ccName); w.format(" }\n"); @@ -81,9 +79,8 @@ protected String nonDefaultCondition() { @Override public void clear(PrintWriter w) { - w.format("%s = %s;\n", ccName, field.getDefaultValue()); - w.format("_%sBufferIdx = -1;\n", ccName); - w.format("_%sBufferLen = -1;\n", ccName); + // No value reset needed: the getter is guarded by the presence condition, and + // parse() invalidates the cached decoded String for fields present on the wire. } @Override @@ -139,6 +136,9 @@ public void parseTextFormat(PrintWriter w) { @Override public void parse(PrintWriter w) { + // Invalidate any stale decoded String from a previous parse or set: clear() + // no longer resets it, so it must be dropped when the field is on the wire. + w.format("%s = null;\n", ccName); w.format("_%sBufferLen = LightProtoCodec.readVarInt(_buffer);\n", ccName); w.format("_%sBufferIdx = _buffer.readerIndex();\n", ccName); w.format("_buffer.skipBytes(_%sBufferLen);\n", ccName); @@ -146,7 +146,9 @@ public void parse(PrintWriter w) { @Override public void materialize(PrintWriter w) { - w.format("if (_%sBufferIdx >= 0) {\n", ccName); + // The presence guard is required: an absent field may hold a stale buffer + // index from an earlier parse of a buffer that has since been released. + w.format("if ((%s) && _%sBufferIdx >= 0) {\n", presenceCondition(), ccName); w.format(" %s = LightProtoCodec.readString(_parsedBuffer, _%sBufferIdx, _%sBufferLen);\n", ccName, ccName, ccName); w.format(" _%sBufferIdx = -1;\n", ccName); w.format("}\n"); diff --git a/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java b/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java new file mode 100644 index 0000000..4d0cbfd --- /dev/null +++ b/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java @@ -0,0 +1,296 @@ +/** + * Copyright 2026 StreamNative + * + * 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 io.streamnative.lightproto.tests; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** + * Verifies that reusing a message instance across parse/clear cycles never exposes stale + * data. Since clear() only resets presence state (not field values), every read path must + * be guarded by presence: getters, equals/hashCode, copyFrom, serialization and materialize. + */ +public class InstanceReuseTest { + + private static byte[] bytes(AddressBook ab) { + return ab.toByteArray(); + } + + private static AddressBook wideBook() { + AddressBook ab = new AddressBook(); + Person p = ab.addPerson(); + p.setName("wide-name"); + p.setId(1); + p.setEmail("wide@example.com"); + Person.PhoneNumber pn1 = p.addPhone(); + pn1.setNumber("111-111"); + pn1.setType(Person.PhoneType.WORK); + Person.PhoneNumber pn2 = p.addPhone(); + pn2.setNumber("222-222"); + return ab; + } + + private static AddressBook narrowBook() { + AddressBook ab = new AddressBook(); + Person p = ab.addPerson(); + p.setName("narrow-name"); + p.setId(2); + return ab; + } + + @Test + public void testWideThenNarrowParseReuse() { + byte[] wide = bytes(wideBook()); + byte[] narrow = bytes(narrowBook()); + + AddressBook reused = new AddressBook(); + reused.parseFrom(wide); + // Force lazy string decode so a stale String is cached in the instance + assertEquals("wide@example.com", reused.getPersonAt(0).getEmail()); + + reused.parseFrom(narrow); + Person p = reused.getPersonAt(0); + assertEquals("narrow-name", p.getName()); + assertEquals(2, p.getId()); + assertFalse(p.hasEmail()); + assertEquals("", p.getEmail()); + assertEquals(0, p.getPhonesCount()); + + // Re-serialization must be byte-identical to the narrow message + assertArrayEquals(narrow, bytes(reused)); + + // And the reused instance must be indistinguishable from a fresh parse + AddressBook fresh = new AddressBook(); + fresh.parseFrom(narrow); + assertEquals(fresh, reused); + assertEquals(fresh.hashCode(), reused.hashCode()); + } + + @Test + public void testEnumDefaultAfterReuse() { + byte[] wide = bytes(wideBook()); + + AddressBook reused = new AddressBook(); + reused.parseFrom(wide); + assertEquals(Person.PhoneType.WORK, reused.getPersonAt(0).getPhoneAt(0).getType()); + + // Second phone had no type: default (HOME) applies + assertFalse(reused.getPersonAt(0).getPhoneAt(1).hasType()); + assertEquals(Person.PhoneType.HOME, reused.getPersonAt(0).getPhoneAt(1).getType()); + + // Re-parse: first phone slot is pooled from the WORK phone; a phone without + // a type must not inherit the stale WORK value + AddressBook oneDefaultPhone = new AddressBook(); + Person p = oneDefaultPhone.addPerson(); + p.setName("n"); + p.setId(3); + p.addPhone().setNumber("333-333"); + + reused.parseFrom(bytes(oneDefaultPhone)); + Person.PhoneNumber pn = reused.getPersonAt(0).getPhoneAt(0); + assertFalse(pn.hasType()); + assertEquals(Person.PhoneType.HOME, pn.getType()); + } + + @Test + public void testProto3ImplicitPresenceReuse() { + Proto3Message wide = new Proto3Message(); + wide.setIntField(42); + wide.setLongField(43L); + wide.setFloatField(1.5f); + wide.setDoubleField(2.5); + wide.setBoolField(true); + wide.setStringField("hello"); + wide.setBytesField(new byte[] {1, 2, 3}); + wide.setEnumField(Proto3Enum.VALUE_B); + byte[] wideBytes = wide.toByteArray(); + + Proto3Message reused = new Proto3Message(); + reused.parseFrom(wideBytes); + assertEquals(42, reused.getIntField()); + assertEquals("hello", reused.getStringField()); + + // Parse an empty message into the same instance: every implicit field + // must read back as its default and nothing must be serialized + reused.parseFrom(new byte[0]); + assertEquals(0, reused.getIntField()); + assertEquals(0L, reused.getLongField()); + assertEquals(0.0f, reused.getFloatField()); + assertEquals(0.0, reused.getDoubleField()); + assertFalse(reused.isBoolField()); + assertEquals("", reused.getStringField()); + assertEquals(0, reused.getBytesFieldSize()); + assertArrayEquals(new byte[0], reused.getBytesField()); + assertEquals(Proto3Enum.DEFAULT, reused.getEnumField()); + assertEquals(0, reused.toByteArray().length); + + Proto3Message fresh = new Proto3Message(); + assertEquals(fresh, reused); + assertEquals(fresh.hashCode(), reused.hashCode()); + } + + @Test + public void testProto3SetToDefaultEqualsUnset() { + // proto3: explicitly setting an implicit-presence field to its default must be + // indistinguishable from never setting it (not serialized, equals/hashCode agree) + Proto3Message a = new Proto3Message(); + a.setIntField(0); + a.setStringField(""); + Proto3Message b = new Proto3Message(); + + assertEquals(0, a.toByteArray().length); + assertEquals(b, a); + assertEquals(a, b); + assertEquals(b.hashCode(), a.hashCode()); + } + + @Test + public void testStaleDecodedStringDropped() { + S first = new S(); + first.setId("hello"); + S second = new S(); + second.setId("world"); + + S reused = new S(); + reused.parseFrom(first.toByteArray()); + assertEquals("hello", reused.getId()); // caches decoded String + + reused.parseFrom(second.toByteArray()); + assertEquals("world", reused.getId()); + + reused.parseFrom(new byte[0]); + assertFalse(reused.hasId()); + assertEquals("", reused.getId()); + } + + @Test + public void testClearFieldClearsChildMessage() { + Frame f = new Frame(); + f.setName("f"); + f.setPoint().setX(5).setY(6).setZ(7); + assertEquals(5, f.getPoint().getX()); + + f.clearPoint(); + assertFalse(f.hasPoint()); + // The child returned after clearing the field must be fully cleared + assertEquals(0, f.getPoint().getX()); + assertEquals(0, f.getPoint().getY()); + assertFalse(f.getPoint().hasZ()); + assertEquals(0, f.getPoint().getZ()); + } + + @Test + public void testPooledRepeatedMessageClearedOnAdd() { + AddressBook ab = wideBook(); + byte[] wide = bytes(ab); + ab.parseFrom(wide); // pool holds fully-populated Person instances + + ab.clear(); + // addPerson() must hand out a cleared instance even when pooled + Person p = ab.addPerson(); + assertFalse(p.hasName()); + assertEquals("", p.getName()); + assertFalse(p.hasEmail()); + assertEquals(0, p.getPhonesCount()); + } + + @Test + public void testOneofReuse() { + OneofMsg withString = new OneofMsg().setOneofString("str-value"); + OneofMsg withInt = new OneofMsg().setOneofInt(99); + + OneofMsg reused = new OneofMsg(); + reused.parseFrom(withString.toByteArray()); + assertEquals(OneofMsg.TestOneofCase.ONEOF_STRING, reused.getTestOneofCase()); + assertEquals("str-value", reused.getOneofString()); + + reused.parseFrom(withInt.toByteArray()); + assertEquals(OneofMsg.TestOneofCase.ONEOF_INT, reused.getTestOneofCase()); + assertEquals(99, reused.getOneofInt()); + assertFalse(reused.hasOneofString()); + assertEquals("", reused.getOneofString()); + + reused.parseFrom(new byte[0]); + assertEquals(OneofMsg.TestOneofCase.NOT_SET, reused.getTestOneofCase()); + assertEquals(0, reused.getOneofInt()); + assertEquals("", reused.getOneofString()); + } + + @Test + public void testMaterializeSkipsAbsentFields() { + S withId = new S(); + withId.setId("some-id-value"); + + S reused = new S(); + reused.parseFrom(withId.toByteArray()); + // Parse an empty message: the stale buffer index of `id` points into the old + // (conceptually released) buffer and must not be dereferenced by materialize() + reused.parseFrom(new byte[0]); + reused.materialize(); + assertFalse(reused.hasId()); + assertEquals("", reused.getId()); + } + + @Test + public void testMapReuse() { + MapMessage m1 = new MapMessage(); + m1.putStringToInt("a", 1); + m1.putStringToInt("b", 2); + m1.putStringToInt("c", 3); + MapNestedValue v = m1.putStringToMsg("k"); + v.setId(10); + v.setName("nested-name"); + + MapMessage reused = new MapMessage(); + reused.parseFrom(m1.toByteArray()); + assertEquals(3, reused.getStringToIntCount()); + assertEquals(2, reused.getStringToInt("b")); + assertEquals("nested-name", reused.getStringToMsg("k").getName()); + + // Smaller map into the same instance; the pooled nested value message must not + // retain the stale `name` sub-field + MapMessage m2 = new MapMessage(); + m2.putStringToInt("z", 26); + m2.putStringToMsg("k").setId(20); + + reused.parseFrom(m2.toByteArray()); + assertEquals(1, reused.getStringToIntCount()); + assertEquals(26, reused.getStringToInt("z")); + assertThrows(IllegalArgumentException.class, () -> reused.getStringToInt("a")); + assertEquals(20, reused.getStringToMsg("k").getId()); + assertFalse(reused.getStringToMsg("k").hasName()); + assertEquals("", reused.getStringToMsg("k").getName()); + } + + @Test + public void testCopyFromReusedInstance() { + Proto3Message src = new Proto3Message(); + src.parseFrom(new Proto3Message().setIntField(5).setStringField("x").toByteArray()); + src.parseFrom(new byte[0]); // src now holds stale-but-guarded values + + Proto3Message target = new Proto3Message(); + target.setLongField(7); // must survive: copyFrom merges present fields + target.copyFrom(src); + assertEquals(0, target.getIntField()); + assertEquals("", target.getStringField()); + assertEquals(7L, target.getLongField()); + } +} From e4a02aa91e77a99e53ffa27559a333ff769c42ca Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Wed, 29 Jul 2026 12:25:36 +0200 Subject: [PATCH 2/2] Treat set-to-default as unset in proto3 copyFrom Review feedback: copyFrom() guarded implicit-presence fields only on the source's internal presence bit, so a field explicitly set to its default (e.g. setIntField(0)) would overwrite a non-default target value. proto3 merge semantics treat default == unset (protobuf-java mergeFrom checks `other.getX() != 0`), and the field would not survive a serialize/parse roundtrip either. The copy condition now also requires the source value to be non-default, matching the serialization condition. --- .../generator/LightProtoBooleanField.java | 4 +-- .../generator/LightProtoBytesField.java | 4 +-- .../generator/LightProtoEnumField.java | 4 +-- .../lightproto/generator/LightProtoField.java | 7 ++--- .../generator/LightProtoMessage.java | 9 ++++--- .../generator/LightProtoNumberField.java | 4 +-- .../generator/LightProtoStringField.java | 4 +-- .../lightproto/tests/InstanceReuseTest.java | 26 +++++++++++++++++++ 8 files changed, 46 insertions(+), 16 deletions(-) diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java index 05af7a5..533785a 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBooleanField.java @@ -89,7 +89,7 @@ public void hashCodeCode(PrintWriter w) { } @Override - protected String nonDefaultCondition() { - return ccName; + protected String nonDefaultCondition(String qualifier) { + return qualifier + ccName; } } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java index ee6a741..50f69c2 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoBytesField.java @@ -93,8 +93,8 @@ public void getter(PrintWriter w) { } @Override - protected String nonDefaultCondition() { - return "_" + ccName + "Len > 0"; + protected String nonDefaultCondition(String qualifier) { + return qualifier + "_" + ccName + "Len > 0"; } @Override diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java index abf44f4..d6105ae 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoEnumField.java @@ -102,7 +102,7 @@ public void hashCodeCode(PrintWriter w) { } @Override - protected String nonDefaultCondition() { - return ccName + ".getValue() != 0"; + protected String nonDefaultCondition(String qualifier) { + return qualifier + ccName + ".getValue() != 0"; } } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java index b095891..83bfe8d 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoField.java @@ -256,16 +256,17 @@ public String serializeCondition() { if (field.hasImplicitPresence()) { // The presence bit guards against stale values after an O(1) clear(); // the non-default check preserves proto3 semantics (defaults not emitted). - return presenceCondition() + " && " + nonDefaultCondition(); + return presenceCondition() + " && " + nonDefaultCondition(""); } return Util.camelCase("has", field.getName()) + "()"; } /** - * Returns the Java expression that is true when this field has a non-default value. + * Returns the Java expression that is true when this field's value, qualified by the + * given prefix (e.g. "_other."), is not the proto3 default. * Only called for proto3 implicit presence fields. */ - protected String nonDefaultCondition() { + protected String nonDefaultCondition(String qualifier) { throw new UnsupportedOperationException("nonDefaultCondition not implemented for " + getClass().getSimpleName()); } diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java index 9d645ed..4acc498 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoMessage.java @@ -211,9 +211,12 @@ private void generateCopyFrom(PrintWriter w) { if (f.isRepeated()) { f.copy(w); } else if (f.field.hasImplicitPresence()) { - // No public has() — guard on the other message's internal presence bit, - // since its field value may be stale when the bit is unset. - w.format(" if ((_other._bitField%d & %s) != 0) {\n", f.bitFieldIndex(), f.fieldMask()); + // No public has() — guard on the other message's internal presence bit + // (its field value may be stale when the bit is unset) AND on the value + // being non-default: proto3 merge semantics treat set-to-default as + // unset, so it must not overwrite a non-default target value. + w.format(" if ((_other._bitField%d & %s) != 0 && %s) {\n", + f.bitFieldIndex(), f.fieldMask(), f.nonDefaultCondition("_other.")); f.copy(w); w.format(" }\n"); } else { diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java index f9883ce..cc2ffce 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoNumberField.java @@ -294,8 +294,8 @@ public void clear(PrintWriter w) { } @Override - protected String nonDefaultCondition() { - return ccName + " != 0"; + protected String nonDefaultCondition(String qualifier) { + return qualifier + ccName + " != 0"; } static int fixedDataSize(ProtoFieldDescriptor field) { diff --git a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java index 53b3113..6d3557e 100644 --- a/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java +++ b/code-generator/src/main/java/io/streamnative/lightproto/generator/LightProtoStringField.java @@ -73,8 +73,8 @@ public void getter(PrintWriter w) { } @Override - protected String nonDefaultCondition() { - return "_" + ccName + "BufferLen > 0"; + protected String nonDefaultCondition(String qualifier) { + return qualifier + "_" + ccName + "BufferLen > 0"; } @Override diff --git a/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java b/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java index 4d0cbfd..72ea7da 100644 --- a/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java +++ b/tests/src/test/java/io/streamnative/lightproto/tests/InstanceReuseTest.java @@ -280,6 +280,32 @@ public void testMapReuse() { assertEquals("", reused.getStringToMsg("k").getName()); } + @Test + public void testCopyFromTreatsSetToDefaultAsUnset() { + // proto3 merge semantics: a source field explicitly set to its default must not + // overwrite a non-default target value (same as protobuf-java mergeFrom, and + // consistent with the field not being serialized on the wire) + Proto3Message src = new Proto3Message(); + src.setIntField(0); + src.setStringField(""); + src.setBoolField(false); + + Proto3Message target = new Proto3Message(); + target.setIntField(7); + target.setStringField("keep"); + target.setBoolField(true); + target.copyFrom(src); + assertEquals(7, target.getIntField()); + assertEquals("keep", target.getStringField()); + assertTrue(target.isBoolField()); + + // Non-default source values do overwrite + src.setIntField(5); + target.copyFrom(src); + assertEquals(5, target.getIntField()); + assertEquals("keep", target.getStringField()); + } + @Test public void testCopyFromReusedInstance() { Proto3Message src = new Proto3Message();