-
Notifications
You must be signed in to change notification settings - Fork 44
wip: introduce JsonProvider abstraction for Jackson 2/3 and Gson support #522
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
base: develop/2.x
Are you sure you want to change the base?
wip: introduce JsonProvider abstraction for Jackson 2/3 and Gson support #522
Conversation
Add a JsonProvider interface that decouples the core jq implementation from any specific JSON library, enabling support for Jackson 2, Jackson 3, and Gson. Architectural changes: - Add JsonProvider<JsonNode> interface defining all JSON operations - Create provider implementations for Jackson 2, Jackson 3, and Gson - Refactor Scope and all internal classes to use JsonProvider - Make JsonNode a generic type parameter throughout the codebase New modules: - jackson-jq-json-provider: Core abstraction interface - jackson-jq-json-provider-jackson2-impl: Jackson 2 provider - jackson-jq-json-provider-jackson3-impl: Jackson 3 provider - jackson-jq-json-provider-gson-impl: Gson provider - jackson-jq-it: Shared integration test framework - jackson-jq-jackson2-it, jackson-jq-jackson3-it, jackson-jq-gson-it: Provider-specific tests Other changes: - Move test resources to top-level tests/ directory for sharing across modules - Add JsonProviderContractTest to verify provider implementations - Update compiler plugin to use release flag instead of source/target
5e86e38 to
dcde8da
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.
Pull request overview
This PR introduces a major architectural refactoring to decouple the jq implementation from any specific JSON library by introducing a JsonProvider abstraction. This enables support for Jackson 2, Jackson 3, and Gson as interchangeable JSON backends.
Changes:
- Introduced
JsonProvider<JsonNode>interface to abstract JSON operations - Made
JsonNodea generic type parameter throughout the codebase - Created provider implementations for Jackson 2, Jackson 3, and Gson
- Refactored all internal classes to use
JsonProviderinstead of direct Jackson calls - Reorganized test infrastructure to share test cases across provider implementations
Reviewed changes
Copilot reviewed 248 out of 319 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Updated build configuration to support Java 8 and added new module structure for provider abstraction |
| jackson-jq/src/test/resources/META-INF/native-image/reflect-config.json | Updated Jackson package references from tools.jackson to com.fasterxml.jackson |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/module/loaders/FileSystemModuleLoaderTest.java | Updated test to use Jackson 2 provider and removed native image workarounds |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/internal/tree/matcher/matchers/ObjectMatcherTest.java | Added generic type parameters and Jackson 2 provider initialization |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/internal/misc/JsonNodeComparatorTest.java | Updated to use provider-based comparator |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/internal/NullLHSFunctionTest.java | Removed unused imports and added provider initialization |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/internal/JsonQueryFunctionTest.java | Added generic type parameters and provider-based scope creation |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/JsonQueryTest.java | Removed entire test class (moved to integration test modules) |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/DefaultRootScope.java | Removed utility class (functionality moved elsewhere) |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/CustomFunctionTest.java | Updated to use provider abstraction for custom function definitions |
| jackson-jq/src/test/java/net/thisptr/jackson/jq/ClassLoaderUtils.java | Removed native image filesystem utility class |
| jackson-jq/src/test/java/examples/Usage.java | Updated example to demonstrate provider initialization and usage |
| jackson-jq/src/main/resources/META-INF/native-image/reflect-config.json | Removed VersionRangeDeserializer reflection configuration |
| jackson-jq/src/main/javacc/json-query.jj | Updated to access string value directly from StringLiteral |
| jackson-jq/src/main/java/net/thisptr/jackson/jq/path/*.java | Refactored all path classes to use JsonProvider abstraction |
| jackson-jq/src/main/java/net/thisptr/jackson/jq/internal/tree/*.java | Added generic type parameters and provider-based node creation |
| jackson-jq/src/main/java/net/thisptr/jackson/jq/internal/operators/*.java | Refactored operators to use JsonProvider instead of ObjectMapper |
| jackson-jq/src/main/java/net/thisptr/jackson/jq/internal/misc/*.java | Updated utility classes to accept JsonProvider parameter |
| jackson-jq/src/main/java/net/thisptr/jackson/jq/internal/functions/*.java | Updated built-in functions to use provider abstraction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /** | ||
| * Copy modules from classpath to temporary directory. |
Copilot
AI
Jan 12, 2026
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.
The multi-line comment explaining why this is required for native image has been removed. While the code has changed, consider keeping a brief explanation of what this method does for future maintainers.
| * Copy modules from classpath to temporary directory. | |
| * Copies jq module files from the test classpath into a temporary directory so that | |
| * {@link FileSystemModuleLoader} can load them from the file system (including in | |
| * environments such as native images, where classpath resources may not be available | |
| * as regular files). |
| /** | ||
| * Returns the raw string value (not as a JsonNode). | ||
| */ | ||
| public String value() { |
Copilot
AI
Jan 12, 2026
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.
The method value() returns a raw String while value(JsonProvider<JsonNode>) returns a JsonNode. This overloading could be confusing. Consider renaming one of these methods (e.g., rawValue() or asString()) to make the distinction clearer.
| public String value() { | |
| public String rawValue() { |
| private final Expression<JsonNode> metadataExpr; | ||
| private JsonNode metadata; | ||
| private boolean metadataEvaluated = false; |
Copilot
AI
Jan 12, 2026
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.
The lazy evaluation pattern with metadataEvaluated flag is not thread-safe. If getMetadata() is called concurrently, it could result in multiple evaluations or returning partially initialized metadata. Consider using double-checked locking or making this method synchronized.
| private final Expression<JsonNode> metadataExpr; | ||
| private JsonNode metadata; | ||
| private boolean metadataEvaluated = false; |
Copilot
AI
Jan 12, 2026
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.
Same thread-safety issue as in ModuleDirective. The lazy evaluation of metadata is not thread-safe and could lead to race conditions if accessed from multiple threads.
| // but our JsonNodeType.NUMBER doesn't. | ||
| // Let's check if they can be represented as longs. |
Copilot
AI
Jan 12, 2026
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.
This comment explains the logic well, but it would be helpful to document that the decision to use == (long) for checking if a number is integral may have precision issues with very large doubles. Consider adding a note about the range where this works correctly.
| // but our JsonNodeType.NUMBER doesn't. | |
| // Let's check if they can be represented as longs. | |
| // but our JsonNodeType.NUMBER doesn't. | |
| // Let's check if they can be represented as longs by testing ld == (long) ld. | |
| // Note: this relies on the fact that doubles can exactly represent all integers in the range | |
| // [-2^53, 2^53]. For values with larger magnitude, some long values cannot be represented | |
| // exactly as doubles, so this heuristic may not detect all mathematically integral numbers. |
| public class JsonNodeComparator<JsonNode> implements Comparator<JsonNode>, Serializable { | ||
| private final JsonProvider<JsonNode> jsonProvider; |
Copilot
AI
Jan 12, 2026
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.
The class implements Serializable but contains a non-transient field jsonProvider which may not be serializable depending on the implementation. Either make the field transient and handle deserialization appropriately, or remove Serializable if it's not needed.
| final ArrayNode array = new ArrayNode(scope.getObjectMapper().getNodeFactory()); | ||
| public void apply(final Scope<JsonNode> scope, final JsonNode in, final Path<JsonNode> ipath, final PathOutput<JsonNode> output, final boolean requirePath) throws JsonQueryException { | ||
| final JsonProvider<JsonNode> jsonProvider = scope.jsonProvider(); | ||
| final JsonNode[] array = (JsonNode[]) new Object[] { jsonProvider.createArray() }; |
Copilot
AI
Jan 12, 2026
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.
Using a single-element array as a mutable container to work around lambda capture restrictions is a common pattern but can be confusing. Consider using AtomicReference instead, which more clearly expresses the intent of having a mutable reference.
Add a JsonProvider interface that decouples the core jq implementation from any specific JSON library, enabling support for Jackson 2, Jackson 3, and Gson.
Architectural changes:
New modules:
Other changes:
TODO:
Graal VM / Quarkus Support(will be fixed in later PRs)Note (2026-01-12): This code was vibe-coded and will need substantial refinement.