Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review

Overall: This is a correct and well-explained bug fix. The root cause analysis is accurate and the regression tests are solid. A few items worth discussing before merge.

What the PR does

writeExternal was writing properties.size() as the property count, then silently skipping null-valued properties and properties with unrecognized types (type == -1) without writing their size/payload bytes. Because readExternal always reads readUTF() + readInt() + read(array) for each counted entry, a single skipped property desynced every subsequent read, producing garbage values or EOFException.

The fix pre-serializes all valid properties into a staging ByteArrayOutputStream, counts only those that were actually serialized, then writes validCount followed by the staged bytes. The written count now matches the wire bytes exactly.

Issues

1. docs/ file should be removed

docs/4328-javabinaryserializer-property-count-mismatch.md is a per-issue design doc that duplicates the PR description. Per project convention, per-issue analysis docs are not committed - the fix lives in the code and the context lives in the PR/commit message. This file will become stale and confusing once the PR is merged. Please drop it.

2. Pre-existing in.read(array) may not read all bytes (not introduced here, but worth flagging)

In readExternal at line 131:

in.read(array);

ObjectInput.read(byte[]) is not guaranteed to fill the buffer in one call (it returns the number of bytes actually read, just like InputStream.read). For large property payloads this can silently truncate values. Should be:

in.readFully(array);

This is a pre-existing bug and out of scope for this PR, but since you are touching this class it would be a good opportunity to fix it.

3. Minor: extra heap buffer allocation

The staging ByteArrayOutputStream allocates and grows a heap buffer holding a copy of all serialized properties. A two-pass approach avoids this at the cost of iterating the in-memory map twice:

// Pass 1: count valid entries
int validCount = 0;
for (final Map.Entry<String, Object> prop : properties.entrySet()) {
  if (prop.getValue() == null) continue;
  final byte type = BinaryTypes.getTypeFromValue(prop.getValue(), documentType.getPropertyIfExists(prop.getKey()));
  if (type != -1) validCount++;
}
out.writeInt(validCount);

// Pass 2: serialize
for (final Map.Entry<String, Object> prop : properties.entrySet()) { ... }

For documents with many large properties this halves peak heap pressure. Since serialization is not on the query hot path this is a minor concern, but it is simpler code too - no DataOutputStream wrapper, no staged.toByteArray() copy.

What is good

• Root cause is correct. The count/bytes mismatch is exactly the bug and the fix closes the gap cleanly.
• Regression tests are well-designed. documentWithNullPropertyDoesNotDesync specifically tests the desync scenario (null property in the middle of non-null ones), and documentAllNullPropertiesDeserializesEmpty covers the degenerate all-null case. Both use AssertJ style matching project conventions.
• The try-with-resources on DataOutputStream is correct even though ByteArrayOutputStream.close() is a no-op; good habit.
• Backward compatibility is not a concern. Any previously serialized document with null properties was already undeserializable due to the bug, so there is no valid old format to support.
• The Javadoc on line 39 was updated to match the new behavior (Null properties are excluded from serialization).

Summary

Fix the in.read(array) → in.readFully(array) issue while you are here (even as a separate commit), drop the docs/ file, and either merge as-is or consider the two-pass approach if you want to avoid the staging buffer. The core logic is correct and the tests are solid.

Thanks for the thorough review. Addressed point 1 (dropped docs/ file) in 2cf6346.

On point 2 (in.read -> in.readFully): Already being fixed in #4348 (currently open). The bot's own note flags it as out of scope, and pulling it in here would create a conflict with that PR. Leaving for #4348.

On point 3 (two-pass instead of staging buffer): The staging-buffer shape came from gemini's review in the first round (5a3e679 -> 3053f8b). A two-pass approach would re-invoke BinaryTypes.getTypeFromValue and serializer.serializeValue per property to avoid the staging copy. Since serialization is not on the query hot path - as the review notes - I prefer one allocation over duplicate compute. Keeping the staging buffer.

Code Review

Overview

This PR fixes a real data-corruption bug in JavaBinarySerializer.writeExternal: the old code wrote properties.size() as the count but silently skipped null/unrecognized-type entries, causing readExternal to desync on every subsequent property. The fix pre-serializes valid entries into a staging buffer, then writes the true count before the buffered bytes. The root cause analysis and fix are both correct.

Code Quality

writeExternal logic is sound. Collecting entries into a ByteArrayOutputStream before writing the count is the simplest correct approach given that ObjectOutput does not extend OutputStream, so staged.writeTo(out) is not directly callable.

One avoidable extra copy - staged.toByteArray() allocates a new byte array on every call. Since writeExternal is always called from MutableDocument.writeExternal with an ObjectOutputStream, you could cast and use staged.writeTo((OutputStream) out) to skip the extra allocation:

// instead of: out.write(staged.toByteArray())
staged.writeTo((OutputStream) out);

Minor, but worth it for large documents.

Comment style - CLAUDE.md says comments should explain the why rather than the what, capped at one line. The two-line staging comment and // PROPERTY COUNT - matches exactly what will be written below both describe what the code does. Either drop them or condense to a single non-obvious constraint note.

Pre-existing issue surfaced by the fix (not introduced here)

In readExternal (line 131):

in.read(array);   // may return fewer bytes than array.length

ObjectInput.read(byte[]) follows the InputStream.read contract and is not guaranteed to fill the buffer in one call. For payloads spanning multiple network segments this can silently truncate a property. The correct replacement is:

in.readFully(array);

ObjectInputStream already implements DataInput, so readFully is available. Since this sits right next to the fixed code, fixing it in the same pass would be clean.

Tests

Two regression tests are included, confirmed to fail before the fix and pass after - good practice.

Assertion style - line 164 uses:

assertThat(result.containsKey("nullProp")).isFalse();

The more idiomatic AssertJ form (better failure message, matches project style) is:

assertThat(result).doesNotContainKey("nullProp");

Summary

The fix is correct and the tests cover the important desync scenario. Suggested follow-ups:

1. Replace in.read(array) with in.readFully(array) in readExternal - correctness gap, pre-existing.
2. Optionally: staged.writeTo((OutputStream) out) to avoid the extra byte-array copy.
3. assertThat(result.containsKey(...)).isFalse() -> assertThat(result).doesNotContainKey(...).
4. Trim redundant comments per CLAUDE.md conventions.

Applied in 5188a4e:

• assertThat(result).doesNotContainKey("nullProp") (idiomatic AssertJ)
• Trimmed the what-style comments per CLAUDE.md

Declined:

• staged.writeTo((OutputStream) out) - the method signature is ObjectOutput, not ObjectOutputStream. The cast holds today only because MutableDocument happens to call us with ObjectOutputStream. If a future caller passes any other ObjectOutput implementation, the cast would throw ClassCastException. The single toByteArray() allocation is the price of respecting the interface.
• in.read(array) -> in.readFully(array) - same suggestion as the previous cycle; being addressed in fix(#4329): use readFully instead of read in JavaBinarySerializer.readExternal #4348.

Code Review

Overview

This PR fixes a desync bug in JavaBinarySerializer.writeExternal (closes #4328). The root cause: the original code wrote properties.size() as the property count up-front, then silently skipped null-valued and unrecognized-type properties during iteration - but readExternal unconditionally consumed exactly that many entries, causing the read cursor to fall out of sync for every subsequent property.

The fix stages all valid entries into a ByteArrayOutputStream in one pass, then writes validCount followed by the staged bytes - so the written count always matches what is on the wire.

The Fix is Correct

The two-pass approach cleanly solves the desync: null and unrecognized-type properties are filtered before the count is committed to the stream. The logic is easy to follow and the fix is minimal.

Findings

One pre-existing bug worth fixing alongside this PR (readExternal)

In readExternal at line 129:

in.read(array);

ObjectInput extends DataInput, which provides readFully. The plain read(byte[]) call is not guaranteed to fill the buffer in one shot - it may return a short read on slow streams or large payloads. This should be:

in.readFully(array);

Not introduced by this PR, but since the test suite is already exercising this path it is a good moment to close the gap.

Minor: unnecessary heap copy

staged.toByteArray() (line 87) allocates a full copy of the staged buffer. The allocation is harmless for correctness and for most use cases, but it is extra GC pressure on documents with many/large properties. One option is to write validCount into staged first (or use a separate 4-byte prefix), then call staged.writeTo(wrappedOutputStream) - but since ObjectOutput is not an OutputStream the current approach is the pragmatic choice. Worth a comment or a ticket if this path is hot.

One naming nit

The local variable staged is clear; the parallel variable tempOut vs out is a bit confusing since both are output streams in scope. stagedOut would be unambiguous.

Test Coverage

Both new tests are well-structured:

• documentWithNullPropertyDoesNotDesync covers the exact reported scenario (null in the middle of non-null properties).
• documentAllNullPropertiesDeserializesEmpty covers the degenerate all-null edge case.
• The assertion style (assertThat(...).containsEntry(...).doesNotContainKey(...)) matches the project convention.
• The PR description confirms both tests fail before the fix and pass after - good discipline.

One small suggestion: the second test calls doc2.toMap(false) to exclude @cat/@type metadata, but the comment explaining why is only in-line. A companion assertion verifying that the non-metadata map is indeed empty (e.g. hasSize(0)) would make intent even clearer, though isEmpty() already does this.

Summary

The fix is correct and well-tested. The only actionable suggestion is to change in.read(array) to in.readFully(array) in readExternal while this code is under review - it is a latent bug on the read side that mirrors the write-side bug this PR fixes.

Cycle 4 (final per --max-cycles=4):

• Applied: renamed tempOut -> stagedOut (317e730).
• Declined: in.readFully - still being addressed in fix(#4329): use readFully instead of read in JavaBinarySerializer.readExternal #4348.
• The toByteArray heap copy point is acknowledged but unavoidable while the signature is ObjectOutput.

Stopping the orchestrator here. Merge belongs to the maintainer.

Code Review

Overview

This PR fixes a real data-corruption bug in JavaBinarySerializer.writeExternal. The old code wrote properties.size() as the property count (which included null-valued and unserializable properties), but then skipped writing size+bytes for those entries. Since readExternal unconditionally reads name + int-size + bytes for each counted entry, any null or unserializable property caused complete stream desync.

The fix is correct and the root cause analysis in the PR description is accurate.

What works well

• Correct approach: Staging to a ByteArrayOutputStream first, then writing validCount before the payload, is clean and eliminates the count/bytes mismatch.
• Backward compatibility: Documents with no null properties and all valid types produce an identical wire format - the staged bytes contain the same name+size+data sequence as before. Only the (already-broken) null-property case changes.
• Test coverage: The two new tests directly reproduce the reported failure (EOFException on desync) and verify the fix. The TestHelper base class drops and recreates the database after each test, so both tests creating a type named "Doc" is safe.
• Logging preserved: The WARNING log for unserializable types is kept, which aids debugging.

Issues and suggestions

Pre-existing bug worth noting (out of scope but related): readExternal at line 129 uses in.read(array) which is not guaranteed to fill the buffer - ObjectInputStream.read(byte[]) may return fewer bytes than requested if the stream is chunked. This should be in.readFully(array) for correctness. This is pre-existing and not introduced by this PR, but it sits right next to the fix and could cause a similar class of silent data corruption on large property values.

Minor - extra allocation: staged.toByteArray() creates a copy of the already-buffered bytes. For Java serialization (not the hot binary path) this is acceptable. An alternative would be a two-pass approach (count valid entries first, then write), but that would either serialize values twice or require storing intermediate results - the staged-buffer approach is simpler and more readable.

Minor - validCount is redundant with staged: After the try-with-resources block, validCount equals the number of entries written to stagedOut. It's clear and correct as-is, but it's worth noting it could also be derived by tracking entries in a list. Not a problem.

Comment style: The CLAUDE.md guidance asks to avoid comments that explain what code does rather than why. The added comment // Count must be written before the payload, so stage the entries to know the final count up-front. documents a real non-obvious constraint (ordering of count vs. bytes on the wire), which satisfies the "WHY is non-obvious" bar.

Summary

The fix is correct, well-tested, and safe to merge. The one thing worth addressing before or shortly after merging is in.read(array) → in.readFully(array) in readExternal to close a related reliability gap on the read side.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.26%. Comparing base (61177a8) to head (cd51460).

@@             Coverage Diff              @@
##               main    #4349      +/-   ##
============================================
- Coverage     64.72%   58.26%   -6.46%     
+ Complexity      446      423      -23     
============================================
  Files          1647     1647              
  Lines        127719   127725       +6     
  Branches      27375    27375              
============================================
- Hits          82660    74422    -8238     
- Misses        33384    42643    +9259     
+ Partials      11675    10660    -1015     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review

Overview

This PR fixes a correctness bug in JavaBinarySerializer.writeExternal: the old code wrote properties.size() as the property count but silently skipped null-valued and unrecognized-type properties when emitting their payloads. Because readExternal always reads count entries, one skipped property desynced all subsequent reads, producing garbage values or EOFException.

The fix stages serializable entries into a ByteArrayOutputStream, counts them, then writes the count followed by the staged bytes. The fix is correct and the approach is sound.

Correctness - No Issues

The root cause is accurately diagnosed and the fix is logically correct. The written count now exactly matches the number of entries on the wire. Both null-skip and type=-1-skip paths are handled consistently.

Potential Issues

Pre-existing bug in readExternal (not introduced here, but worth fixing)

// JavaBinarySerializer.java:129
final byte[] array = new byte[propertySize];
in.read(array);  // <-- may read fewer bytes than requested

ObjectInput.read(byte[]) is not guaranteed to fill the array - it behaves like InputStream.read(). This should be in.readFully(array). Under normal ObjectInputStream use this rarely triggers, but it is a latent correctness hole worth fixing in a follow-up.

Double logging for unrecognized types

BinaryTypes.getTypeFromValue already logs a WARNING when it returns -1, and then writeExternal logs a second WARNING for the same event. This was present in the old code and is preserved here, but results in two log lines per unrecognized property. Minor, but worth noting.

Performance

The fix allocates a ByteArrayOutputStream and DataOutputStream wrapper on every writeExternal call, adding GC pressure. The project has a strong performance/GC-minimization mandate.

A lighter alternative: the thread-local db.getContext().getTemporaryBuffer2() (8 KB, pre-allocated) could stage the raw bytes, but encoding Java's writeUTF format manually into Binary is fragile. The current approach trades a small per-call allocation for clear, maintainable code - an acceptable trade-off given the fix is for correctness. If writeExternal ever becomes a hot path, a reusable staging buffer per thread-context would be the right next step.

Also, staged.toByteArray() at line 87 creates a defensive copy of the internal buffer. A micro-optimization: staged.writeTo((OutputStream) out) avoids that copy (valid since ObjectOutputStream extends OutputStream), though the cast is slightly fragile. Not a blocking concern.

Test Coverage

The two new tests are well-structured and use the correct AssertJ assertion style:

• documentWithNullPropertyDoesNotDesync - covers the primary bug scenario (null in the middle of valid properties)
• documentAllNullPropertiesDeserializesEmpty - covers the all-null edge case

Suggestion: Add a third test for the unrecognized-type (type == -1) path, analogous to the null test. The unrecognized-type branch is the other skip path and currently has no explicit regression coverage.

Minor Notes

• The // Count must be written before... comment is appropriate - it explains a non-obvious staging constraint.
• The removed inline // PROPERTY COUNT, // PROPERTY NAME, // PROPERTY VALUE comments are not missed; the code is self-explanatory without them.
• Wire format compatibility: data serialized with the old buggy code was already unreadable (desync), so the format change is not a real regression.

Summary

The fix is correct and the tests confirm the before/after behavior. The main actionable items are:

1. (This PR or follow-up) Fix in.read(array) to in.readFully(array) in readExternal
2. (Optional) Add a test for the unrecognized-type (type == -1) skip path
3. (Optional) Consider staged.writeTo((OutputStream) out) to avoid the defensive copy in toByteArray()