Introduce factory method for Json-P Builders - #12541
Open
TillJan wants to merge 13 commits into
Open
Conversation
poikilotherm
previously requested changes
Jul 17, 2026
poikilotherm
added
Component: Code Infrastructure
TillJan
force-pushed
the
#12362-factory-method-for-JSONbuilders
branch
2 times, most recently
from
July 17, 2026 12:45
04f565f to
ac66224
Compare
poikilotherm
changed the title
Contributor
|
@TillJan you're on to present your work during the next tech hour (2026-07-28). |
poikilotherm
moved this from Ready for Triage
to Needs Sizing 📏
in IQSS Dataverse Project
poikilotherm
marked this pull request as ready for review
July 21, 2026 17:08
Contributor
|
@TillJan we should specifically take another look at the Also, as discussed during tech hour, lets move the |
pdurbin
added
Size: 3
cmbz
added
FY27 Sprint 2
poikilotherm
force-pushed
the
#12362-factory-method-for-JSONbuilders
branch
from
July 31, 2026 13:04
c1d1d91 to
5842cfa
Compare
Member
|
This came up in standup today. Do you need any help? Are you blocked? Thanks for the PR! |
Member
|
(just anxious to get it through review/QA and it's in In Progress right now) |
Contributor
|
I think this was done before Till went on his vacation. Let me rebase and set correct status. |
poikilotherm
moved this from In Progress 💻
to Ready for Review ⏩
in IQSS Dataverse Project
use JsonUtil.createObjectBuilder and JsonUtil.createArrayBuilder to reuse JsonUtil.builderFactory and avoid inefficient usage of JSON providers/builders.
refactored the start import to specific imports
replaced all Json.createObjectBuilder() and Json.createArrayBuilder() calls with the matching JsonUtil methods.
For now, enable less invasive bundled checks. In addition, adding first rules to disallow usage of JSON-P Json.createX() API.
Disallow more methods for object and array builders using copy-style arguments.
…sonProvider. replaced all remaining Json.createObjectBuilder() and Json.createArrayBuilder() calls with the matching JsonUtil methods.
use JsonUtil.createValue to reuse JsonUtil.provider and avoid usage of Json.createValue. Replaced all Json.createValue to JsonUtil.createValue.
…ch fixes the forbiddenapis signature parsing failure.
…is plugin usage While using the source code analysis config files from a Config JAR is a great idea in general, it will not work in our codebase. As we do not use a parent POM at the root of our project, we cannot make the Maven Reactor pull in the dataverse-sca-config module during a build. We'd need every developer to switch to using `mvn -f modules/dataverse-parent` all the time, which seems unlikely. Bad design choices in the past, not easy to resolve. While we could push a built version to Maven Central, that would not help when modifying the signature file. At the same time, using there is no reliable way to have an absolute path injected into a Maven property. We were using project.basedir within the parent POM before. This will be resolved at build time and always points to the directory where the current POM is running from. The reusability of this centralized plugin definition is rather limited, as it would fail to lookup the file in a different module. The only way out of this: 1. Move the plugin into the dataverse module build chain. 2. This way, the resolved path is always correct, as it is model-bound, not half-parent-bound. 3. To still satisfy the request to avoid clutter in the project root, create a src/maven folder, ready to collect all these configurations.
Cleaning up the root directory, a single directory will now contain any configs related to Maven plugins. This will (mostly) be for Static Code Analysis plugins.
pdurbin
force-pushed
the
#12362-factory-method-for-JSONbuilders
branch
from
August 7, 2026 01:44
9b41ec9 to
9ddbf5c
Compare
Member
|
Checks were failing so I rebased. Hopefully they pass! |
github-project-automation
Bot
moved this from Ready for Review ⏩
to Ready for QA ⏩
in IQSS Dataverse Project
Member
|
@poikilotherm thanks for approving. I unassigned you and @TillJan because the "ready" columns like "Ready for QA" shouldn't have anyone assigned. We do this so anyone can pick it up, drag it to the next column, and assign themselves. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The PR adds methods in the JsonUtil for creating a JsonObjectBuilder and a JsonArrayBuilder. JsonUtil caches a JsonBuilderFactory which is shared by both methods.
All usage of Json.createArrayBuilder() and Json.createObjectBuilder has been replaced by the new methods.
This avoids repeatedly loading a Jakarta JSON-P implementation through the ServiceLoader which caused inefficiency.
Which issue(s) this PR closes:
Special notes for your reviewer:
The JsonBuilderFactory is shared and only created one time, while the methods are returning new Builder everytime.
Suggestions on how to test this:
run existing tests
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
no
Is there a release notes update needed for this change?:
no
Additional documentation: