Fix nested message object in API responses#12097
Conversation
This comment has been minimized.
This comment has been minimized.
pdurbin
left a comment
There was a problem hiding this comment.
A couple quick comments.
Also, @poikilotherm noticed this double message bug, which lead to this API v2 proposal: https://docs.google.com/document/d/1XtXPF_PZCuhPbm4HCu28yRge_4_--ah604NhigPdjPk/edit?usp=sharing . And these related issues:
src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java
Outdated
Show resolved
Hide resolved
|
@pdurbin Thanks for the quick review and the context about the API v2 proposal! I was a bit hesitant to make this change since it could break existing API consumers that have implemented workarounds for the nested message object. However, I think it's worth fixing. The current behavior is clearly a bug, and the affected endpoints are relatively niche (the bug was actually discovered through duplicate file uploads, while regular file additions work fine). That said, I can see the argument for keeping this for API v2 if you think the breaking change risk is too high. Let me know your thoughts - happy to go either way. I've fixed the integration tests and added the changelog entry as requested. The docs build should pass now too (had a small RST syntax issue with the reference). |
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
While the commits look good overall, I am reluctant to give this a pass as-is.
I discovered more problems with inconsistencies in #11667 and as @pdurbin pointed out, this led to the v2 API proposal.
I agree with @ErykKul that changing this is very useful. And for 6 of the 7 affected endpoints I am agreeing that changing them may cause minor inconveniences for administrators (fine, just deal with it).
Yet with the Dataset endpoint to add files warning about duplicates and which are duplicated - this may seriously affect some integration with Dataverse out there. This may actually be a breaking change causing trouble for users. IMHO we shouldn't do this.
Also, if we fix that, let's do it all in one sweep and fix any other inconsistencies as well.
There may be some middle ground here if we make this opt-in. Using a feature flag marking it as experimental, we can leave a choice to the admin. They can test with their integrations and see what happens and return to the default at any time. (This reminds me of mentioning https://openfeature.dev somewhere - making feature flags activate based on more criteria than mere configuration...)
A way forward may be to make this experimental for now, then deprecate it in a future version, then drop it. This way this may not even need an API v2 (only regarding this message - the mileage for the other inconsistencies may vary!)
Adds API_MESSAGE_FIELD_LEGACY feature flag that allows admins to revert to the old (buggy) nested message format if they have integrations that depend on it. The fix is now the default behavior. Set dataverse.feature.api-message-field-legacy=true to use the legacy format. This flag will be removed in a future version.
This comment has been minimized.
This comment has been minimized.
|
Hi @pdurbin, @poikilotherm, I've addressed the feedback in this PR:
I ran the full test suite locally (unit tests + integration tests) and everything passes. However, I'm unable to access the Jenkins server from here — it seems to be restricted to internal networks. @pdurbin, if Jenkins fails again on this latest commit, could you share the relevant log output or a screenshot? That would help me figure out what's going on. Thanks! |
This comment has been minimized.
This comment has been minimized.
|
Overall, Oliver’s code changes look good to me and the direction is solid. I only added a few follow-up tweaks after merge/testing:
Targeted tests pass:
|
… low-level details from being exposed to clients
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the ok(String msg, JsonObjectBuilder bld) method in AbstractApiBean.java that incorrectly returned nested message objects ({"message":{"message":"..."}}) instead of plain strings ({"message":"..."}). The bug has existed since Dataverse 5.0 (May 2020) and affects 7 API endpoints. The fix includes backward compatibility options and introduces comprehensive test infrastructure for feature flags and JVM settings.
Changes:
- Fixed the nested message object bug in API responses with opt-out legacy mode via
dataverse.legacy.api-response-message-style - Refactored all
ok()response methods inAbstractApiBeanto use a unified implementation with NullSafeJsonBuilder - Added experimental opt-in feature flag
UNIFY_API_RESPONSE_MESSAGE_STYLEfor aligning ~230 additional API responses - Introduced test infrastructure for manipulating feature flags and JVM settings in unit tests
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/edu/harvard/iq/dataverse/api/AbstractApiBean.java |
Core fix for nested message bug, refactored ok() methods to use unified implementation |
src/main/java/edu/harvard/iq/dataverse/api/ApiConstants.java |
Moved API constants from AbstractApiBean for better organization |
src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java |
Added LEGACY_API_RESPONSE_MESSAGE_STYLE setting for opt-out backward compatibility |
src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java |
Added UNIFY_API_RESPONSE_MESSAGE_STYLE feature flag and getScopedKey() method |
src/main/java/edu/harvard/iq/dataverse/engine/command/DataverseRequest.java |
Updated to use ApiConstants instead of AbstractApiBean constants |
src/main/java/edu/harvard/iq/dataverse/api/BatchImport.java |
Improved error handling (unrelated to main PR purpose) |
src/main/java/edu/harvard/iq/dataverse/api/ldn/COARNotifyRelationshipAnnouncement.java |
Enhanced JSON-LD IRI handling (unrelated to main PR purpose) |
src/test/java/edu/harvard/iq/dataverse/api/AbstractApiBeanTest.java |
Added tests for default, legacy, and unified message styles |
src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java |
Updated constant reference from AbstractApiBean to ApiConstants |
src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java |
Updated test assertions to expect plain string messages instead of nested objects |
src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java |
Updated constant reference from AbstractApiBean to ApiConstants |
src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java |
Updated test assertion to expect plain string message instead of nested object |
src/test/java/edu/harvard/iq/dataverse/util/cache/CacheFactoryBeanTest.java |
Replaced Hazelcast with ConcurrentHashMap (unrelated to main PR purpose) |
src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlag.java |
New test annotation for setting feature flags during tests |
src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagBroker.java |
Interface for accessing and manipulating feature flags in tests |
src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagExtension.java |
JUnit5 extension for feature flag test support |
src/test/java/edu/harvard/iq/dataverse/util/testing/LocalFeatureFlags.java |
Annotation for tests that manipulate local feature flags via system properties |
doc/sphinx-guides/source/installation/config.rst |
Documentation for new legacy setting and feature flag |
doc/sphinx-guides/source/api/changelog.rst |
API changelog entry for v6.10 breaking change |
doc/release-notes/12096-fix-ok-message-nested-object.md |
Release notes explaining the fix and migration path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/edu/harvard/iq/dataverse/util/cache/CacheFactoryBeanTest.java
Outdated
Show resolved
Hide resolved
| private String extractField(JsonObject msgObject, String key) { | ||
| return msgObject.containsKey(key) ? msgObject.getString(key) : null; | ||
| if (!msgObject.containsKey(key)) { | ||
| return null; | ||
| } | ||
|
|
||
| JsonValue value = msgObject.get(key); | ||
| if (value.getValueType().equals(JsonValue.ValueType.STRING)) { | ||
| return msgObject.getString(key); | ||
| } | ||
|
|
||
| // JSON-LD expansion may represent IRI values as {"@id":"..."} objects. | ||
| if (value.getValueType().equals(JsonValue.ValueType.OBJECT)) { | ||
| JsonObject objectValue = msgObject.getJsonObject(key); | ||
| JsonValue id = objectValue.get("@id"); | ||
| if (id != null && id.getValueType().equals(JsonValue.ValueType.STRING)) { | ||
| return objectValue.getString("@id"); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
The changes to extractField() method in this file appear unrelated to the PR's stated purpose of fixing nested message objects in API responses. These changes handle JSON-LD expansion for IRI values. While the implementation looks correct, this should ideally be in a separate PR focused on JSON-LD handling improvements, not bundled with an API response format fix. This makes it harder to review, test, and potentially revert if issues arise with either change.
There was a problem hiding this comment.
I will wait to see if all tests pass (Jenkins is still running at this point), and then revert it.
There was a problem hiding this comment.
All tests passed, reverted.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…e local only, e.g., passes on github?
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
…e-nested-object' into 12096-fix-ok-message-nested-object
|
📦 Pushed preview images as 🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
|
All tests passed, I reverted the unrelated changes so tests fail again. However, the fix is in #11983 |
What this PR does / why we need it:
Fixes the
ok(String msg, JsonObjectBuilder bld)method inAbstractApiBean.javawhich incorrectly wraps the message in a nested object:The bug was introduced in commit f311312c34 on May 12, 2020, as part of #4813.
Which issue(s) this PR closes:
Special notes for your reviewer:
This is a one-line fix in
AbstractApiBean.javaline 1000:The following 7 API endpoints are affected:
Datasets.javaPOST /api/datasets/{id}/add(duplicate file warning)Admin.javaPUT /api/admin/settingsDataverses.javaPUT /api/dataverses/{id}Dataverses.javaPUT /api/dataverses/{id}/inputLevelsSavedSearches.javaPOST /api/admin/savedsearchesHarvestingClients.javaPUT /api/harvest/clients/{nickName}HarvestingServer.javaPUT /api/harvest/server/oaisets/{specname}Suggestions on how to test this:
messagefield is a string, not a nested object: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?:
Yes, included in this PR:
doc/release-notes/12096-fix-ok-message-nested-object.mdAdditional documentation:
N/A