Repository navigation
Optimize protobuf and Jackson field access - #985
Conversation
Cache source-specific accessors for generated protobuf messages and Jackson 2/3 properties. Convert CEL maps directly to protobuf map entries while retaining descriptor-based fallbacks and regression coverage for representative value types.
| IteratorT iterator = value.iterator(); | ||
| while (iterator.hasNext() == True) { | ||
| Val key = iterator.next(); | ||
| Val mapValue = value.find(key); |
There was a problem hiding this comment.
codex finding:
[P2] Preserve last-value-wins semantics when copying protobuf maps
A DynamicMessage can retain duplicate map-entry messages. The protobuf map specification states:
When parsing from the wire or when merging, if there are duplicate map keys the last key seen is used.
For entries ("k", "old") followed by ("k", "new"), this loop calls value.find(key) for both occurrences. However, ProtoMapT.find() retains the first value through both its linear scan and its index populated with putIfAbsent(). The resulting map therefore contains "old".
Previously, conversion through toJavaMap() used Map.put(), correctly retaining "new".
There was a problem hiding this comment.
Good point, added a commit to fix this.
| case INT64, SINT64, SFIXED64 -> value.convertToNative(Long.class); | ||
| case UINT64, FIXED64 -> value.convertToNative(ULong.class).longValue(); | ||
| case INT32, SINT32, SFIXED32 -> value.convertToNative(Integer.class); | ||
| case UINT32, FIXED32 -> value.convertToNative(ULong.class).intValue(); |
There was a problem hiding this comment.
codex finding:
[P2] Reject out-of-range unsigned 32-bit map entries
The new UINT32/FIXED32 conversion calls ULong.intValue(), which performs an unchecked cast to int. For example, 4294967296u becomes 0. For map keys, this can also collapse distinct entries such as 0u and 4294967296u.
Previously, map materialization used Val.value(), and the map conversion passed those values unchanged into MapEntry. That path also lacked range validation, but preserved the boxed Long, yielding an invalid native representation. The new path instead produces a serializable, silently truncated value.
Please validate the unsigned range against 0xffffffffL before narrowing, following the existing check in UintT.convertToNative() for UInt32Value.
There was a problem hiding this comment.
Good finding!
The next PR updates this code quite a bit, so I'd prefer to fix it in that next PR.
Make TypeAdapter the bidirectional conversion boundary. Keep Val.convertToNative() as the deprecated compatibility fallback, preserve primitive and range semantics, and route containers and environments through their configured adapters. Also rejects out-of-range uint32 protobuf map entries (see also projectnessie#985): Map-entry conversion narrowed CEL uint values to Java int without validating the uint32 range. Values above 4294967295 therefore silently changed value.\n\nValidate uint32 and fixed32 map keys and values before narrowing. Add coverage for a value immediately above the uint32 maximum.
Dynamic protobuf messages can retain duplicate entries for a map field. Protobuf map parsing and merge semantics use the final entry for a key.\n\nMake lookup, indexed lookup, iteration, and size observe the logical map rather than its physical entry list. Add a regression test for a dynamic message with duplicate map entries.
Make TypeAdapter the bidirectional conversion boundary. Keep Val.convertToNative() as the deprecated compatibility fallback, preserve primitive and range semantics, and route containers and environments through their configured adapters. Also rejects out-of-range uint32 protobuf map entries (see also projectnessie#985): Map-entry conversion narrowed CEL uint values to Java int without validating the uint32 range. Values above 4294967295 therefore silently changed value.\n\nValidate uint32 and fixed32 map keys and values before narrowing. Add coverage for a value immediately above the uint32 maximum.
Make TypeAdapter the bidirectional conversion boundary. Keep Val.convertToNative() as the deprecated compatibility fallback, preserve primitive and range semantics, and route containers and environments through their configured adapters. Also rejects out-of-range uint32 protobuf map entries (see also projectnessie#985): Map-entry conversion narrowed CEL uint values to Java int without validating the uint32 range. Values above 4294967295 therefore silently changed value.\n\nValidate uint32 and fixed32 map keys and values before narrowing. Add coverage for a value immediately above the uint32 maximum.
Make TypeAdapter the bidirectional conversion boundary. Keep Val.convertToNative() as the deprecated compatibility fallback, preserve primitive and range semantics, and route containers and environments through their configured adapters. Also rejects out-of-range uint32 protobuf map entries (see also projectnessie#985): Map-entry conversion narrowed CEL uint values to Java int without validating the uint32 range. Values above 4294967295 therefore silently changed value.\n\nValidate uint32 and fixed32 map keys and values before narrowing. Add coverage for a value immediately above the uint32 maximum.
Make `TypeAdapter` the bidirectional boundary for Java/CEL conversion, with primitive conversion helpers and shared default implementations. Retain deprecated `Val.convertToNative()` as the compatibility fallback for custom values. Route list and map exports through the calling adapter, including nested containers and protobuf-backed maps. Preserve CEL values during list concatenation so outbound conversion hooks cannot alter evaluation results. Preserve primitive range checks and boxed conversion targets for custom protobuf map entries. Correct unsigned protobuf map reads and reject out-of-range `uint32` and `fixed32` keys and values before narrowing (see #985). Add regression coverage for custom adapters, legacy conversions, nested containers, concatenation, and generated/dynamic protobuf round trips, plus conversion benchmarks.
Cache source-specific accessors for generated protobuf messages and Jackson 2/3 properties. Convert CEL maps directly to protobuf map entries while retaining descriptor-based fallbacks and regression coverage for representative value types.