big code refactoring - initial commit#220
Conversation
There was a problem hiding this comment.
Pull request overview
This PR is a major refactor of the json-compare library, introducing a new fluent comparison API and structured diff results while reorganizing matcher internals and improving performance for large inputs.
Changes:
- Added a fluent builder entry point (
JSONCompare.compare(...)) and deprecated the prior static overloads. - Introduced a structured result model (
ComparisonResult,Diff,DiffKind) alongside legacy raw diff strings. - Refactored matcher internals (DSL parsing/type inspection extraction) and added performance optimizations (regex cache, BitSet tracking, fast field lookup).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/io/json/compare/util/MessageUtil.java | Improves diff message cropping and readability with constants and clearer flow. |
| src/main/java/io/json/compare/util/JsonUtils.java | Makes JSON utilities final, improves readability, and documents mapper configuration. |
| src/main/java/io/json/compare/result/DiffKind.java | Adds enum for categorizing diffs for programmatic filtering. |
| src/main/java/io/json/compare/result/Diff.java | Adds structured diff record with parsing/classification helpers. |
| src/main/java/io/json/compare/result/ComparisonResult.java | Adds immutable structured comparison result with convenience accessors. |
| src/main/java/io/json/compare/matcher/UseCase.java | Extracts DSL marker parsing/sanitization and do-not-match counting. |
| src/main/java/io/json/compare/matcher/NodeInspect.java | Extracts null-tolerant JsonNode type predicates and JSON-path node detection. |
| src/main/java/io/json/compare/matcher/JsonValueMatcher.java | Refactors value matching flow and centralizes diff formatting. |
| src/main/java/io/json/compare/matcher/JsonPathMatcher.java | Refactors JSON-path diff wrapping and pre-sizes output lists. |
| src/main/java/io/json/compare/matcher/JsonObjectMatcher.java | Refactors object matching, adds literal fast-path lookup, and uses extracted helpers. |
| src/main/java/io/json/compare/matcher/JsonMatcher.java | Simplifies top-level dispatch using NodeInspect and returns singleton diffs. |
| src/main/java/io/json/compare/matcher/JsonArrayMatcher.java | Switches match tracking to BitSet and refactors loop flow for perf/clarity. |
| src/main/java/io/json/compare/matcher/AbstractJsonMatcher.java | Slims base class to shared state + constants; removes embedded helpers/enums. |
| src/main/java/io/json/compare/JsonComparator.java | Documents comparator contract and clarifies default behavior expectations. |
| src/main/java/io/json/compare/JSONCompare.java | Adds builder entry point, delegates deprecated overloads, and updates docs. |
| src/main/java/io/json/compare/DefaultJsonComparator.java | Adds per-instance regex compilation cache and consolidates matching logic. |
| src/main/java/io/json/compare/ComparisonBuilder.java | Implements fluent comparison configuration + assertion/diff/result terminals. |
| src/main/java/io/json/compare/CompareMode.java | Documents compare mode semantics and adds inline Javadoc. |
| pom.xml | Bumps to 8.0-SNAPSHOT, raises Java baseline to 17, adjusts deps/scopes. |
| Changelog.md | Documents new API, Java baseline change, internal refactors, and fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static ComparisonBuilder compare(Object expected, Object actual) { | ||
| return new ComparisonBuilder(expected, actual); | ||
| } |
There was a problem hiding this comment.
The new fluent builder API (JSONCompare.compare(...).modes(...).comparator(...).message(...).assertMatches()/result()) is a major new public surface, but the existing test suite appears to only cover the deprecated static overloads. Adding targeted tests that exercise the builder (including result()/DiffKind classification) would help prevent regressions while the old overloads are phased out.
| */ | ||
| public record Diff(String path, DiffKind kind, String description, String raw) { | ||
|
|
||
| public static Diff parse(String rawDiff) { |
There was a problem hiding this comment.
Diff.parse will throw a NullPointerException if rawDiff is null (and extractPath will also NPE). Since this is a public API surface, consider validating the argument and throwing an IllegalArgumentException with a clear message (or otherwise defining null-handling behavior).
| public static Diff parse(String rawDiff) { | |
| public static Diff parse(String rawDiff) { | |
| if (rawDiff == null) { | |
| throw new IllegalArgumentException("rawDiff must not be null"); | |
| } |
| private static DiffKind inferKind(String path, String description) { | ||
| if (description.contains("Different JSON types")) return DiffKind.TYPE_MISMATCH; | ||
| if (description.contains("Json path")) return DiffKind.JSON_PATH_ERROR; |
There was a problem hiding this comment.
inferKind only classifies JSON-path-related diffs when the description contains the substring "Json path". However, JSON-path result mismatches are formatted as "Expected json path result" (see JsonPathMatcher), so those diffs won’t be categorized as JSON_PATH_ERROR despite the DiffKind Javadoc implying they should. Consider detecting JSON-path diffs via the wrapper marker in the path (e.g. containing ".#(") and/or also matching on "Expected json path result".
| private static DiffKind inferKind(String path, String description) { | |
| if (description.contains("Different JSON types")) return DiffKind.TYPE_MISMATCH; | |
| if (description.contains("Json path")) return DiffKind.JSON_PATH_ERROR; | |
| private static boolean isJsonPathDiff(String path, String description) { | |
| return description.contains("Json path") | |
| || description.contains("Expected json path result") | |
| || path.contains(".#("); | |
| } | |
| private static DiffKind inferKind(String path, String description) { | |
| if (description.contains("Different JSON types")) return DiffKind.TYPE_MISMATCH; | |
| if (isJsonPathDiff(path, description)) return DiffKind.JSON_PATH_ERROR; |
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…tyList() with List.of()
…hods with List.of() in JsonObjectMatcher
No description provided.