-
-
Notifications
You must be signed in to change notification settings - Fork 98
fix(gh-1162): ensure NitriteId serialization is compatible with previous versions #1164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(gh-1162): ensure NitriteId serialization is compatible with previous versions #1164
Conversation
WalkthroughAdds a negated .gitignore entry to track Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as JUnit Test
participant FS as FileSystem (temp)
participant MV as MVStoreModule
participant DB as Nitrite DB
participant Coll as Collection
Note right of Test: testIssue1162 flow (loads bundled v4.3.0 DB)
Test->>FS: copy packaged `no2-v4.3.0.db` to temp
Test->>MV: configure MVStoreModule with temp DB path
Test->>DB: open Nitrite instance (via MVStoreModule)
DB->>Coll: load collection data (deserialize stored objects including NitriteId)
Coll-->>Test: return collection size and documents
alt deserialization compatible
Test-->>Test: assert expected size and first document ID
else incompatible (InvalidClassException)
DB-->>Test: exception thrown during deserialization
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ce59500 to
36df63a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (1)
49-62: Good fix for backward compatibility; consider hardening against accidental default serializationRenaming the field and adding the explicit warning is helpful. Given custom writeObject/readObject are now authoritative, consider marking the field transient to prevent accidental default serialization if someone later modifies writeObject. It’s safe (we set it explicitly in readObject/ctors).
- private long _idValue; + private transient long _idValue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
nitrite-mvstore-adapter/src/test/resources/no2-v4.3.0.dbis excluded by!**/*.db
📒 Files selected for processing (3)
.gitignore(1 hunks)nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java(3 hunks)nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Verify GraalVM 21 compatibility on macos-latest
- GitHub Check: Verify GraalVM 21 compatibility on windows-latest
- GitHub Check: Verify GraalVM 17 compatibility on windows-latest
- GitHub Check: Build with Java 17 in Windows
- GitHub Check: Build with Java 11 in MacOS
- GitHub Check: Build with Java 17 in MacOS
- GitHub Check: Build with Java 11 in Windows
- GitHub Check: Analyze (java)
- GitHub Check: Build with Java 17 in Ubuntu
- GitHub Check: Build with Java 11 in Ubuntu
🔇 Additional comments (4)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (4)
117-120: Explicit accessor restores the prior Lombok-generated APIMatches the previous getIdValue contract. Looks good.
123-123: compareTo uses the underlying value directly — OKComparison is consistent with equals/hashCode.
128-129: toString remains stable — OKPreserves the wire/text form ID_PREFIX + value + ID_SUFFIX.
132-137: Widen NitriteId.readObject signature to include ClassNotFoundException and preserve UTF serializationnitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java uses stream.writeUTF/stream.readUTF; keep that encoding and change readObject to the standard signature:
- private void readObject(ObjectInputStream stream) throws IOException { + private void readObject(ObjectInputStream stream) throws IOException, ClassNotFoundException { _idValue = Long.parseLong(stream.readUTF()); }Could not find tag v4.3.0 in this clone to confirm historical parity — verify that v4.3.0 used writeUTF/readUTF before merging.
6a39d0d to
251595a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (2)
123-129: Optional: add identity fast-path in equals.Micro-optimization; no behavior change.
@Override public boolean equals(Object o) { + if (this == o) { + return true; + } if (!(o instanceof NitriteId)) { return false; } return idValue == ((NitriteId) o).idValue; }
27-27: Avoid boxing in hashCode and drop the unused Objects import.Use Long.hashCode to prevent boxing; then the Objects import can be removed.
-import java.util.Objects;@Override public int hashCode() { - return Objects.hashCode(idValue); + return Long.hashCode(idValue); }Also applies to: 131-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
nitrite-mvstore-adapter/src/test/resources/no2-v4.3.0.dbis excluded by!**/*.db
📒 Files selected for processing (3)
.gitignore(1 hunks)nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java(3 hunks)nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- nitrite-mvstore-adapter/src/test/java/org/dizitart/no2/NitriteTest.java
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (2)
nitrite/src/main/java/org/dizitart/no2/exceptions/InvalidIdException.java (1)
InvalidIdException(28-47)nitrite-jackson-mapper/src/test/java/org/dizitart/no2/test/RepositoryJoinTest.java (1)
Data(216-222)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Verify GraalVM 17 compatibility on ubuntu-latest
- GitHub Check: Analyze (java)
- GitHub Check: Verify GraalVM 21 compatibility on windows-latest
- GitHub Check: Verify GraalVM 21 compatibility on macos-latest
- GitHub Check: Verify GraalVM 17 compatibility on macos-latest
- GitHub Check: Build with Java 11 in Ubuntu
- GitHub Check: Verify GraalVM 17 compatibility on windows-latest
- GitHub Check: Build with Java 17 in Windows
- GitHub Check: Verify GraalVM 21 compatibility on ubuntu-latest
- GitHub Check: Build with Java 11 in Windows
- GitHub Check: Build with Java 17 in MacOS
- GitHub Check: Build with Java 17 in Ubuntu
- GitHub Check: Build with Java 11 in MacOS
🔇 Additional comments (1)
nitrite/src/main/java/org/dizitart/no2/collection/NitriteId.java (1)
51-51: Marking idValue transient is the right compatibility fix.This aligns with custom writeObject/readObject and should resolve the InvalidClassException reported in gh-1162.
Please confirm this passes the linked reproducer and the new 4.3.0 DB test case on your side.
…h previous versions
251595a to
338595e
Compare
Fixes #1162
Summary by CodeRabbit
Bug Fixes
Tests
Chores