diff --git a/doc/release-notes/12096-fix-ok-message-nested-object.md b/doc/release-notes/12096-fix-ok-message-nested-object.md new file mode 100644 index 00000000000..ab1e38df8b5 --- /dev/null +++ b/doc/release-notes/12096-fix-ok-message-nested-object.md @@ -0,0 +1,27 @@ +### API Response Format Fix for `message` Field + +The `message` field in API responses from certain endpoints was incorrectly returned as a nested object (`{"message": {"message": "..."}}`) instead of a plain string (`{"message": "..."}`). + +This has been fixed. The following endpoints now return the `message` field as a string, consistent with all other API responses: + +- `POST /api/datasets/{id}/add` (when uploading duplicate files) +- `PUT /api/admin/settings` +- `PUT /api/dataverses/{id}` +- `PUT /api/dataverses/{id}/inputLevels` +- `POST /api/admin/savedsearches` +- `PUT /api/harvest/clients/{nickName}` +- `PUT /api/harvest/server/oaisets/{specname}` + +**Note:** If you have integrations that implemented workarounds for the nested `message` object, you may need to update your code to expect a plain string instead. +If you need time to update your integrations, you can temporarily revert to the legacy behavior by setting this JVM option: + +``` +dataverse.legacy.api-response-message-style=true +``` + +This flag will be removed in a future version. + +**Note:** As of this version, there is also an experimental opt-in feature that will align API responses on about 230 more occassions. +In these responses, the message is embedded into the "data" field as a nested object. +If you want to test your integrations and clients, please enable the `dataverse.feature.unify-api-response-message-style` feature flag. +In a future version of Dataverse, this now experimental style is going to become the supported default. diff --git a/doc/sphinx-guides/source/api/changelog.rst b/doc/sphinx-guides/source/api/changelog.rst index 4c7a5914b1e..234566fd5b3 100644 --- a/doc/sphinx-guides/source/api/changelog.rst +++ b/doc/sphinx-guides/source/api/changelog.rst @@ -7,6 +7,19 @@ This API changelog is experimental and we would love feedback on its usefulness. :local: :depth: 1 +v6.10 +----- + +- Several API endpoints that return both a ``message`` and ``data`` field were incorrectly returning the message as a nested object (``{"message":{"message":"..."}}``). + This has been fixed so that the message is now a plain string (``{"message":"..."}``). + If you have integrations that depend on the old behavior, you can temporarily revert by setting ``dataverse.feature.api-message-field-legacy=true``. + This flag will be removed in a future version. + Affected endpoints: ``POST /api/datasets/{id}/add`` (duplicate file warning), ``PUT /api/admin/settings``, ``PUT /api/dataverses/{id}``, ``PUT /api/dataverses/{id}/inputLevels``, ``POST /api/admin/savedsearches``, ``PUT /api/harvest/clients/{nickName}``, ``PUT /api/harvest/server/oaisets/{specname}``. + See `#12096 `_. +- Most API endpoints that return a success notification but no actual data have it embedded into ``data``: ``{"data":{"message":"..."}}``. + For now, this style will remain the supported default. In a future version of Dataverse the ``message`` will always be a separate top field: ``{"data":{},"message":"..."}``. + Integrators and client vendors are welcome to opt-in to the new style and test thoroughly by enabling :ref:`dataverse.feature.unify-api-response-message-style`. + v6.9 ---- diff --git a/doc/sphinx-guides/source/installation/config.rst b/doc/sphinx-guides/source/installation/config.rst index 07084d8b126..988cb5cf588 100644 --- a/doc/sphinx-guides/source/installation/config.rst +++ b/doc/sphinx-guides/source/installation/config.rst @@ -3388,6 +3388,31 @@ Can also be set via any `supported MicroProfile Config API source`_, e.g. the en This setting will be ignored unless the :ref:`dataverse.api.blocked.policy` is set to ``unblock-key``. Otherwise the deprecated :ref:`:BlockedApiKey` will be used +.. _dataverse.legacy.api-response-message-style: + +dataverse.legacy.api-response-message-style ++++++++++++++++++++++++++++++++++++++++++++ + +Opt-out of no longer nesting an object in the "message" field, carrying the actual notification in its "message" field. +Enabling this will re-activate the legacy message style using ``{"message":{"message":"..."}}``, instead of the aligned format ``{"message": "..."}``. + +This option is provided as a temporary workaround for integrations that may have implemented +workarounds for the buggy behavior. The following endpoints are affected: + +- ``POST /api/datasets/{id}/add`` (just the duplicate file warning) +- ``PUT /api/admin/settings`` +- ``PUT /api/dataverses/{id}`` +- ``PUT /api/dataverses/{id}/inputLevels`` +- ``POST /api/admin/savedsearches`` +- ``PUT /api/harvest/clients/{nickName}`` +- ``PUT /api/harvest/server/oaisets/{specname}`` + +Please update your integrations to expect the corrected message format and deactivate this setting. +In a future version of Dataverse, the legacy format is expected to be removed completely. +See also :ref:`dataverse.feature.unify-api-response-message-style`. + +Can also be set via any `supported MicroProfile Config API source`_, e.g. the environment variable ``DATAVERSE_LEGACY_API_RESPONSE_MESSAGE_STYLE``. + .. _dataverse.ui.show-validity-label-when-published: dataverse.ui.show-validity-label-when-published @@ -3933,6 +3958,19 @@ dataverse.feature.api-bearer-auth-use-oauth-user-on-id-match Allows the use of an OAuth user account (GitHub, Google, or ORCID) when an identity match is found during API bearer authentication. This feature enables automatic association of an incoming IdP identity with an existing OAuth user account, bypassing the need for additional user registration steps. This feature only works when the feature flag ``api-bearer-auth`` is also enabled. **Caution: Enabling this flag could result in impersonation risks if (and only if) used with a misconfigured IdP.** +.. _dataverse.feature.unify-api-response-message-style: + +dataverse.feature.unify-api-response-message-style +++++++++++++++++++++++++++++++++++++++++++++++++++ + +When activated, the "message" in API responses will no longer be nested in the "data" field. +For any response carrying a notification, these will be found within a top-level "message" field of the JSON returned. +This affects about 230 endpoints and is likely to break existing integrations and clients. +It is mandatory to test instance clients and integrations thoroughly and it is not recommended to be used in production. +In a future Dataverse version, the (currently) experimental response message style will be made the only supported one. + +See also :ref:`dataverse.legacy.api-response-message-style`. + .. _dataverse.feature.avoid-expensive-solr-join: dataverse.feature.avoid-expensive-solr-join diff --git a/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java b/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java index 2ee5730153e..c8a11da06bd 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java @@ -32,6 +32,7 @@ import edu.harvard.iq.dataverse.metrics.MetricsServiceBean; import edu.harvard.iq.dataverse.search.savedsearch.SavedSearchServiceBean; import edu.harvard.iq.dataverse.settings.FeatureFlags; +import edu.harvard.iq.dataverse.settings.JvmSettings; import edu.harvard.iq.dataverse.settings.SettingsServiceBean; import edu.harvard.iq.dataverse.util.BundleUtil; import edu.harvard.iq.dataverse.util.DateUtil; @@ -76,12 +77,6 @@ public abstract class AbstractApiBean { private static final Logger logger = Logger.getLogger(AbstractApiBean.class.getName()); - private static final String DATAVERSE_KEY_HEADER_NAME = "X-Dataverse-key"; - private static final String PERSISTENT_ID_KEY=":persistentId"; - private static final String ALIAS_KEY=":alias"; - public static final String STATUS_WF_IN_PROGRESS = "WORKFLOW_IN_PROGRESS"; - public static final String DATAVERSE_WORKFLOW_INVOCATION_HEADER_NAME = "X-Dataverse-invocationID"; - public static final String RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED = "Only authenticated users can perform the requested operation"; /** * Utility class to convey a proper error response using Java's exceptions. @@ -307,7 +302,7 @@ protected String getRequestParameter( String key ) { } protected String getRequestApiKey() { - String headerParamApiKey = httpRequest.getHeader(DATAVERSE_KEY_HEADER_NAME); + String headerParamApiKey = httpRequest.getHeader(ApiConstants.DATAVERSE_KEY_HEADER_NAME); String queryParamApiKey = httpRequest.getParameter("key"); return headerParamApiKey!=null ? headerParamApiKey : queryParamApiKey; @@ -422,11 +417,11 @@ protected Dataset findDatasetOrDie(String id, boolean deep) throws WrappedRespon } } else { String persistentId = id; - if (id.equals(PERSISTENT_ID_KEY)) { - persistentId = getRequestParameter(PERSISTENT_ID_KEY.substring(1)); + if (id.equals(ApiConstants.PERSISTENT_ID_KEY)) { + persistentId = getRequestParameter(ApiConstants.PERSISTENT_ID_KEY.substring(1)); if (persistentId == null) { throw new WrappedResponse( - badRequest(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(PERSISTENT_ID_KEY.substring(1))))); + badRequest(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(ApiConstants.PERSISTENT_ID_KEY.substring(1))))); } } GlobalId globalId; @@ -447,7 +442,7 @@ protected Dataset findDatasetOrDie(String id, boolean deep) throws WrappedRespon fprLogService.logEntry(entry); } throw new WrappedResponse( - notFound(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(PERSISTENT_ID_KEY.substring(1))))); + notFound(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(ApiConstants.PERSISTENT_ID_KEY.substring(1))))); } } if (deep) { @@ -509,11 +504,11 @@ protected void validateInternalTimestampIsNotOutdated(DvObject dvObject, String protected DataFile findDataFileOrDie(String id) throws WrappedResponse { DataFile datafile; - if (id.equals(PERSISTENT_ID_KEY)) { - String persistentId = getRequestParameter(PERSISTENT_ID_KEY.substring(1)); + if (id.equals(ApiConstants.PERSISTENT_ID_KEY)) { + String persistentId = getRequestParameter(ApiConstants.PERSISTENT_ID_KEY.substring(1)); if (persistentId == null) { throw new WrappedResponse( - badRequest(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(PERSISTENT_ID_KEY.substring(1))))); + badRequest(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(ApiConstants.PERSISTENT_ID_KEY.substring(1))))); } datafile = fileService.findByGlobalId(persistentId); if (datafile == null) { @@ -541,8 +536,8 @@ protected DataFile findDataFileOrDie(String id) throws WrappedResponse { protected DataverseRole findRoleOrDie(String id) throws WrappedResponse { DataverseRole role; - if (id.equals(ALIAS_KEY)) { - String alias = getRequestParameter(ALIAS_KEY.substring(1)); + if (id.equals(ApiConstants.ALIAS_KEY)) { + String alias = getRequestParameter(ApiConstants.ALIAS_KEY.substring(1)); try { return em.createNamedQuery("DataverseRole.findDataverseRoleByAlias", DataverseRole.class) .setParameter("alias", alias) @@ -574,11 +569,11 @@ protected DatasetLinkingDataverse findDatasetLinkingDataverseOrDie(String datase DatasetLinkingDataverse dsld; Dataverse linkingDataverse = findDataverseOrDie(linkingDataverseId); - if (datasetId.equals(PERSISTENT_ID_KEY)) { - String persistentId = getRequestParameter(PERSISTENT_ID_KEY.substring(1)); + if (datasetId.equals(ApiConstants.PERSISTENT_ID_KEY)) { + String persistentId = getRequestParameter(ApiConstants.PERSISTENT_ID_KEY.substring(1)); if (persistentId == null) { throw new WrappedResponse( - badRequest(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(PERSISTENT_ID_KEY.substring(1))))); + badRequest(BundleUtil.getStringFromBundle("find.dataset.error.dataset_id_is_null", Collections.singletonList(ApiConstants.PERSISTENT_ID_KEY.substring(1))))); } Dataset dataset = datasetSvc.findByGlobalId(persistentId); @@ -948,71 +943,67 @@ private Response handleDataverseRequestHandlerException(Exception ex) { * HTTP Response methods * \* ====================== */ - protected Response ok( JsonArrayBuilder bld ) { - return Response.ok(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", bld).build()) - .type(MediaType.APPLICATION_JSON).build(); + protected Response ok(JsonValue value, JsonValue message, Long totalCount) { + return Response.status(Response.Status.OK) + .entity(NullSafeJsonBuilder.jsonObjectBuilder() + .add(ApiConstants.STATUS_FIELD, ApiConstants.STATUS_OK) + .add(ApiConstants.MESSAGE_FIELD, message) + .add(ApiConstants.TOTAL_COUNT_FIELD, totalCount) + .add(ApiConstants.DATA_FIELD, value) + .build()) + .type(MediaType.APPLICATION_JSON) + .build(); + + } + + protected Response ok(JsonArrayBuilder bld) { + return ok(bld.build(), null, null); } - protected Response ok( JsonArrayBuilder bld , long totalCount) { - return Response.ok(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("totalCount", totalCount) - .add("data", bld).build()) - .type(MediaType.APPLICATION_JSON).build(); + protected Response ok(JsonArrayBuilder bld, long totalCount) { + return ok(bld.build(), null, totalCount); } - protected Response ok( JsonArray ja ) { - return Response.ok(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", ja).build()) - .type(MediaType.APPLICATION_JSON).build(); + protected Response ok(JsonArray ja) { + return ok(ja, null, null); } - protected Response ok( JsonObjectBuilder bld ) { - return Response.ok( Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", bld).build() ) - .type(MediaType.APPLICATION_JSON) - .build(); + protected Response ok(JsonObjectBuilder bld) { + return ok(bld.build(), null, null); } - protected Response ok( JsonObject jo ) { - return Response.ok( Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", jo).build() ) - .type(MediaType.APPLICATION_JSON) - .build(); + protected Response ok(JsonObject jo) { + return ok(jo, null, null); } - protected Response ok( String msg ) { - return Response.ok().entity(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", Json.createObjectBuilder().add("message",msg)).build() ) - .type(MediaType.APPLICATION_JSON) - .build(); + protected Response ok(String msg) { + // An instance may opt out of using the old {data:{message:"$msg"}} way. + // This is a highly used response builder, which is why this is an experimental opt-in change! + // TODO: This will be removed in a future version. + if (FeatureFlags.UNIFY_API_RESPONSE_MESSAGE_STYLE.enabled()) { + return ok(null, Json.createValue(msg), null); + } else { + return ok(Json.createObjectBuilder().add("message", msg).build(), null, null); + } } - protected Response ok( String msg, JsonObjectBuilder bld ) { - return Response.ok().entity(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("message", Json.createObjectBuilder().add("message",msg)) - .add("data", bld).build()) - .type(MediaType.APPLICATION_JSON) - .build(); + protected Response ok(String msg, JsonObjectBuilder bld) { + // Legacy mode returns message as nested object for backward compatibility with integrations that worked around the bug. + // This is a scarcely used way to build a response, mostly relevant to admins, which is why we make it opt-out. + // TODO: This will be removed in a future version. + if (JvmSettings.LEGACY_API_RESPONSE_MESSAGE_STYLE.lookupOptional(Boolean.class).orElse(false)) { + return ok(bld.build(), Json.createObjectBuilder().add(ApiConstants.MESSAGE_FIELD, msg).build(), null); + } else { + return ok(bld.build(), Json.createValue(msg), null); + } } protected Response ok( boolean value ) { - return Response.ok().entity(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", value).build() ).build(); + return ok(value ? JsonValue.TRUE : JsonValue.FALSE, null, null); } protected Response ok(long value) { - return Response.ok().entity(Json.createObjectBuilder() - .add("status", ApiConstants.STATUS_OK) - .add("data", value).build()).build(); + return ok(Json.createValue(value), null, null); } /** @@ -1037,8 +1028,8 @@ protected Response ok(InputStream inputStream) { protected Response created( String uri, JsonObjectBuilder bld ) { return Response.created( URI.create(uri) ) .entity( Json.createObjectBuilder() - .add("status", "OK") - .add("data", bld).build()) + .add(ApiConstants.STATUS_FIELD, ApiConstants.STATUS_OK) + .add(ApiConstants.DATA_FIELD, bld).build()) .type(MediaType.APPLICATION_JSON) .build(); } @@ -1046,15 +1037,15 @@ protected Response created( String uri, JsonObjectBuilder bld ) { protected Response accepted(JsonObjectBuilder bld) { return Response.accepted() .entity(Json.createObjectBuilder() - .add("status", STATUS_WF_IN_PROGRESS) - .add("data",bld).build() + .add(ApiConstants.STATUS_FIELD, ApiConstants.STATUS_WF_IN_PROGRESS) + .add(ApiConstants.DATA_FIELD, bld).build() ).build(); } protected Response accepted() { return Response.accepted() .entity(Json.createObjectBuilder() - .add("status", STATUS_WF_IN_PROGRESS).build() + .add(ApiConstants.STATUS_FIELD, ApiConstants.STATUS_WF_IN_PROGRESS).build() ).build(); } @@ -1069,8 +1060,8 @@ protected Response badRequest( String msg ) { protected Response badRequest(String msg, Map fieldErrors) { return Response.status(Status.BAD_REQUEST) .entity(NullSafeJsonBuilder.jsonObjectBuilder() - .add("status", ApiConstants.STATUS_ERROR) - .add("message", msg) + .add(ApiConstants.STATUS_FIELD, ApiConstants.STATUS_ERROR) + .add(ApiConstants.MESSAGE_FIELD, msg) .add("fieldErrors", Json.createObjectBuilder(fieldErrors).build()) .build() ) @@ -1103,7 +1094,7 @@ protected Response conflict( String msg ) { } protected Response authenticatedUserRequired() { - return error(Status.UNAUTHORIZED, RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED); + return error(Status.UNAUTHORIZED, ApiConstants.RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED); } protected Response permissionError( PermissionException pe ) { @@ -1128,9 +1119,9 @@ protected Response unauthorized( String message ) { protected static Response error( Status sts, String msg ) { return Response.status(sts) - .entity( NullSafeJsonBuilder.jsonObjectBuilder() - .add("status", ApiConstants.STATUS_ERROR) - .add( "message", msg ).build() + .entity(NullSafeJsonBuilder.jsonObjectBuilder() + .add(ApiConstants.STATUS_FIELD, ApiConstants.STATUS_ERROR) + .add(ApiConstants.MESSAGE_FIELD, msg ).build() ).type(MediaType.APPLICATION_JSON_TYPE).build(); } } diff --git a/src/main/java/edu/harvard/iq/dataverse/api/ApiConstants.java b/src/main/java/edu/harvard/iq/dataverse/api/ApiConstants.java index 15114085c21..3d99d294729 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/ApiConstants.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/ApiConstants.java @@ -7,13 +7,24 @@ private ApiConstants() { } // Statuses + public static final String STATUS_FIELD = "status"; public static final String STATUS_OK = "OK"; public static final String STATUS_ERROR = "ERROR"; + public static final String STATUS_WF_IN_PROGRESS = "WORKFLOW_IN_PROGRESS"; + + public static final String DATA_FIELD = "data"; + public static final String TOTAL_COUNT_FIELD = "totalCount"; + public static final String MESSAGE_FIELD = "message"; // Authentication public static final String CONTAINER_REQUEST_CONTEXT_USER = "user"; + public static final String ALIAS_KEY=":alias"; + public static final String DATAVERSE_KEY_HEADER_NAME = "X-Dataverse-key"; + public static final String DATAVERSE_WORKFLOW_INVOCATION_HEADER_NAME = "X-Dataverse-invocationID"; + public static final String RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED = "Only authenticated users can perform the requested operation"; // Dataset + public static final String PERSISTENT_ID_KEY=":persistentId"; public static final String DS_VERSION_LATEST = ":latest"; public static final String DS_VERSION_DRAFT = ":draft"; public static final String DS_VERSION_LATEST_PUBLISHED = ":latest-published"; diff --git a/src/main/java/edu/harvard/iq/dataverse/api/BatchImport.java b/src/main/java/edu/harvard/iq/dataverse/api/BatchImport.java index a2d06bff93e..7bdeceba13b 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/BatchImport.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/BatchImport.java @@ -89,7 +89,7 @@ public Response postImport(@Context ContainerRequestContext crc, String body, @Q JsonObjectBuilder status = importService.doImport(dataverseRequest, owner, body, filename, ImportType.NEW, cleanupLog); return this.ok(status); } catch (ImportException | IOException e) { - return this.error(Response.Status.BAD_REQUEST, e.getMessage()); + return error(Response.Status.BAD_REQUEST, e.getMessage()); } } @@ -134,7 +134,7 @@ private Response startBatchJob(User user, String fileDir, String parentIdtf, Str batchService.processFilePath(fileDir, parentIdtf, dataverseRequest, owner, importType, createDV); } catch (ImportException e) { - return this.error(Response.Status.BAD_REQUEST, "Import Exception, " + e.getMessage()); + return error(Response.Status.BAD_REQUEST, "Import Exception, " + e.getMessage()); } return this.accepted(); } diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/DataverseRequest.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/DataverseRequest.java index 4d3ec2842a1..5094e84cb75 100644 --- a/src/main/java/edu/harvard/iq/dataverse/engine/command/DataverseRequest.java +++ b/src/main/java/edu/harvard/iq/dataverse/engine/command/DataverseRequest.java @@ -1,6 +1,6 @@ package edu.harvard.iq.dataverse.engine.command; -import edu.harvard.iq.dataverse.api.AbstractApiBean; +import edu.harvard.iq.dataverse.api.ApiConstants; import edu.harvard.iq.dataverse.authorization.groups.impl.ipaddress.ip.IpAddress; import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; import edu.harvard.iq.dataverse.authorization.users.User; @@ -139,7 +139,7 @@ public DataverseRequest(User aUser, HttpServletRequest aHttpServletRequest) { } } - String headerParamWFKey = aHttpServletRequest.getHeader(AbstractApiBean.DATAVERSE_WORKFLOW_INVOCATION_HEADER_NAME); + String headerParamWFKey = aHttpServletRequest.getHeader(ApiConstants.DATAVERSE_WORKFLOW_INVOCATION_HEADER_NAME); String queryParamWFKey = aHttpServletRequest.getParameter("invocationId"); invocationId = headerParamWFKey!=null ? headerParamWFKey : queryParamWFKey; diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java b/src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java index 2e86fae610e..1f3eb12fe74 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/FeatureFlags.java @@ -249,9 +249,23 @@ public enum FeatureFlags { * @since Dataverse 6.9 */ ONLY_UPDATE_DATACITE_WHEN_NEEDED("only-update-datacite-when-needed"), - + + /** + * Historically, success messages have returned success messages as {data:{message:...}}. + * Error messages have been return as either {message:...} or {message:{message:...}}. + * While the alignment of error messages is opt-out (see {@link JvmSettings#LEGACY_API_RESPONSE_MESSAGE_STYLE}, + * changing the response format of ~230 success responses may cause a lot of friction. + * This feature flag makes sure any early adopters can change to the unified response layout, + * and it will graduate to the new default later. + * + * @apiNote Raise flag by setting "dataverse.feature.unify-api-response-message-style" + * @since Dataverse 6.10 + */ + UNIFY_API_RESPONSE_MESSAGE_STYLE("unify-api-response-message-style") + ; + final String flag; final boolean defaultStatus; @@ -285,5 +299,16 @@ public enum FeatureFlags { public boolean enabled() { return JvmSettings.FEATURE_FLAG.lookupOptional(Boolean.class, flag).orElse(defaultStatus); } + + /** + * Returns the scoped configuration key for this feature flag. + * The key is constructed by inserting the flag name into the {@link JvmSettings#FEATURE_FLAG} pattern, + * resulting in a string of the form "dataverse.feature.{flag}". + * + * @return the scoped configuration key as a String + */ + public String getScopedKey() { + return JvmSettings.FEATURE_FLAG.insert(flag); + } } diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java index 05390ba8a8c..e1456df3856 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java @@ -109,6 +109,10 @@ public enum JvmSettings { // Avoids adding flag entries twice. FEATURE_FLAG(SCOPE_FLAGS), + // LEGACY BEHAVIOUR SETTINGS + SCOPE_LEGACY(PREFIX, "legacy"), + LEGACY_API_RESPONSE_MESSAGE_STYLE(SCOPE_LEGACY, "api-response-message-style"), + // METADATA SETTINGS SCOPE_METADATA(PREFIX, "metadata"), MDB_SYSTEM_METADATA_KEYS(SCOPE_METADATA, "block-system-metadata-keys"), diff --git a/src/test/java/edu/harvard/iq/dataverse/api/AbstractApiBeanTest.java b/src/test/java/edu/harvard/iq/dataverse/api/AbstractApiBeanTest.java index c67dfeeadfa..86616946076 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/AbstractApiBeanTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/AbstractApiBeanTest.java @@ -5,6 +5,10 @@ import java.util.HashMap; import java.util.Map; import java.util.logging.Logger; + +import edu.harvard.iq.dataverse.settings.FeatureFlags; +import edu.harvard.iq.dataverse.util.testing.FeatureFlag; +import edu.harvard.iq.dataverse.util.testing.LocalFeatureFlags; import jakarta.json.Json; import jakarta.json.JsonObject; import jakarta.json.JsonReader; @@ -18,19 +22,20 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -public class AbstractApiBeanTest { +@LocalFeatureFlags +class AbstractApiBeanTest { private static final Logger logger = Logger.getLogger(AbstractApiBeanTest.class.getCanonicalName()); AbstractApiBeanImpl sut; @BeforeEach - public void before() { + void before() { sut = new AbstractApiBeanImpl(); } @Test - public void testParseBooleanOrDie_ok() throws Exception { + void testParseBooleanOrDie_ok() throws Exception { assertTrue(sut.parseBooleanOrDie("1")); assertTrue(sut.parseBooleanOrDie("yes")); assertTrue(sut.parseBooleanOrDie("true")); @@ -50,7 +55,7 @@ void testFailIfNull_ok() { } @Test - public void testMessagesNoJsonObject() { + void testMessagesNoJsonObject() { String message = "myMessage"; Response response = sut.ok(message); JsonReader jsonReader = Json.createReader(new StringReader((String) response.getEntity().toString())); @@ -65,6 +70,21 @@ public void testMessagesNoJsonObject() { logger.info(sw.toString()); assertEquals(message, jsonObject.getJsonObject("data").getString("message")); } + + @Test + @FeatureFlag(flag = FeatureFlags.UNIFY_API_RESPONSE_MESSAGE_STYLE) + void testUnifiedMessageStyle() { + // given + String message = "myMessage"; + + // when + Response response = sut.ok(message); + + // then + JsonReader jsonReader = Json.createReader(new StringReader(response.getEntity().toString())); + JsonObject jsonObject = jsonReader.readObject(); + assertEquals(message, jsonObject.getString(ApiConstants.MESSAGE_FIELD)); + } /** * dummy implementation diff --git a/src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java b/src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java index 457bb405795..66de98306b8 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/AdminIT.java @@ -116,7 +116,7 @@ void testSettingsRoundTrip() { .assertThat() .statusCode(OK.getStatusCode()) .body("status", equalTo("OK")) - .body("message.message", containsString("successfully updated")); + .body("message", containsString("successfully updated")); // Step 5: Verify the harmless setting is gone (restored to original state) Response verifyRestoredResponse = UtilIT.getSetting(harmlessSetting); diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java index b7cbb37480c..9f88d9311d7 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DatasetsIT.java @@ -2599,7 +2599,7 @@ public void testCreateDatasetWithDcmDependency() { getRsyncScriptPermErrorGuest.then().assertThat() .statusCode(UNAUTHORIZED.getStatusCode()) .contentType(ContentType.JSON) - .body("message", equalTo(AbstractApiBean.RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED)); + .body("message", equalTo(ApiConstants.RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED)); Response createNoPermsUser = UtilIT.createRandomUser(); String noPermsUsername = UtilIT.getUsernameFromResponse(createNoPermsUser); diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java index dc658c7134b..a3181fda3d9 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java @@ -1120,7 +1120,7 @@ public void testAttributesApi() { Response changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "name", newCollectionName, apiToken); changeAttributeResp.then().assertThat() .statusCode(OK.getStatusCode()) - .body("message.message", equalTo("Update successful")); + .body("message", equalTo("Update successful")); // Change the description of the collection: @@ -1128,7 +1128,7 @@ public void testAttributesApi() { changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "description", newDescription, apiToken); changeAttributeResp.then().assertThat() .statusCode(OK.getStatusCode()) - .body("message.message", equalTo("Update successful")); + .body("message", equalTo("Update successful")); // Change the affiliation of the collection: @@ -1136,7 +1136,7 @@ public void testAttributesApi() { changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "affiliation", newAffiliation, apiToken); changeAttributeResp.then().assertThat() .statusCode(OK.getStatusCode()) - .body("message.message", equalTo("Update successful")); + .body("message", equalTo("Update successful")); // Cannot update filePIDsEnabled from a regular user: @@ -1149,7 +1149,7 @@ public void testAttributesApi() { changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "alias", newCollectionAlias, apiToken); changeAttributeResp.then().assertThat() .statusCode(OK.getStatusCode()) - .body("message.message", equalTo("Update successful")); + .body("message", equalTo("Update successful")); // Check on the collection, under the new alias: diff --git a/src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java b/src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java index ca9e19e1bbf..168b74ae7e2 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java @@ -163,7 +163,7 @@ public void testSearchPermisions() { Response dataverse47behaviorOfTokensBeingRequired = UtilIT.search("id:dataset_" + datasetId, nullToken); dataverse47behaviorOfTokensBeingRequired.prettyPrint(); dataverse47behaviorOfTokensBeingRequired.then().assertThat() - .body("message", CoreMatchers.equalTo(AbstractApiBean.RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED)) + .body("message", CoreMatchers.equalTo(ApiConstants.RESPONSE_MESSAGE_AUTHENTICATED_USER_REQUIRED)) .statusCode(UNAUTHORIZED.getStatusCode()); Response reEnableTokenlessSearch = UtilIT.deleteSetting(SettingsServiceBean.Key.SearchApiRequiresToken); diff --git a/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlag.java b/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlag.java new file mode 100644 index 00000000000..5c4f95a1082 --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlag.java @@ -0,0 +1,62 @@ +package edu.harvard.iq.dataverse.util.testing; + +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.parallel.ResourceAccessMode; +import org.junit.jupiter.api.parallel.ResourceLock; +import org.junit.jupiter.api.parallel.Resources; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Repeatable; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + + +/** + * {@code @FeatureFlag} is a JUnit Jupiter extension to set the value of a + * feature flag (internally a system property) for a test execution. + * + *

The name and state of the feature flag to be set must be specified via + * {@link #flag()} and {@link #value()}. After the annotated method has been + * executed, the initial default value is restored.

+ * + *

{@code @FeatureFlag} can be used on the method and on the class level. + * It is repeatable and inherited from higher-level containers. If a class is + * annotated, the configured flag will be set before every test inside that + * class. Any method level configurations will override the class level + * configurations.

+ * + * Parallel execution of tests using this extension is prohibited by using + * resource locking provided by JUnit5 - system properties are a global state, + * these tests NEED to be done serial. + */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ ElementType.METHOD, ElementType.TYPE }) +@Inherited +@Repeatable(FeatureFlag.FeatureFlags.class) +@ExtendWith(FeatureFlagExtension.class) +@ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ_WRITE) +public @interface FeatureFlag { + + /** + * The name of the feature flag to be set. + */ + edu.harvard.iq.dataverse.settings.FeatureFlags flag(); + + /** + * The state of the flag to be set. + */ + boolean value() default true; + + /** + * Containing annotation of repeatable {@code @FeatureFlag}. + */ + @Retention(RetentionPolicy.RUNTIME) + @Target({ ElementType.METHOD, ElementType.TYPE }) + @Inherited + @ExtendWith(FeatureFlagExtension.class) + @interface FeatureFlags { + FeatureFlag[] value(); + } +} diff --git a/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagBroker.java b/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagBroker.java new file mode 100644 index 00000000000..87b00117853 --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagBroker.java @@ -0,0 +1,42 @@ +package edu.harvard.iq.dataverse.util.testing; + +import edu.harvard.iq.dataverse.settings.FeatureFlags; + +import java.io.IOException; + +/** + * Provide an interface to access and manipulate {@link edu.harvard.iq.dataverse.settings.FeatureFlags} + * at some place (local, remote, different ways to access, etc.). + * Part of the {@link FeatureFlagExtension} extension to allow JUnit5 tests to manipulate these + * settings, enabling to test different code paths and so on. + * @implNote Keep in mind to use methods that do not require restarts or similar to set or delete a setting. + * This must be changeable on the fly, otherwise it will be useless for testing. + * Yes, non-hot-reloadable settings may be a problem. The code should be refactored in these cases. + */ +public interface FeatureFlagBroker { + + /** + * Receive the status of a {@link edu.harvard.iq.dataverse.settings.FeatureFlags}. + * @param flag The feature flag to receive + * @return The status of the flag (false = disabled, true = enabled, null = not set) + * @throws IOException When communication goes sideways. + */ + Boolean get(FeatureFlags flag) throws IOException; + + /** + * Set the state of a {@link edu.harvard.iq.dataverse.settings.FeatureFlags}. + * @param flag The feature flag to set + * @param value The flag state we want to have it set to. + * @throws IOException When communication goes sideways. + */ + void set(FeatureFlags flag, boolean value) throws IOException; + + /** + * Remove the state of a {@link edu.harvard.iq.dataverse.settings.FeatureFlags}. + * For some tests, one might want to clear a certain setting again and potentially have it set back afterward. + * @param flag The feature flag to delete. + * @throws IOException When communication goes sideways. + */ + void delete(FeatureFlags flag) throws IOException; + +} \ No newline at end of file diff --git a/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagExtension.java b/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagExtension.java new file mode 100644 index 00000000000..da06c6c0a3d --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/util/testing/FeatureFlagExtension.java @@ -0,0 +1,97 @@ +package edu.harvard.iq.dataverse.util.testing; + +import org.junit.jupiter.api.extension.AfterAllCallback; +import org.junit.jupiter.api.extension.AfterTestExecutionCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.BeforeTestExecutionCallback; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.platform.commons.support.AnnotationSupport; + +import java.util.List; + +public class FeatureFlagExtension implements BeforeTestExecutionCallback, AfterTestExecutionCallback, BeforeAllCallback, AfterAllCallback { + + @Override + public void beforeAll(ExtensionContext extensionContext) throws Exception { + List flags = AnnotationSupport.findRepeatableAnnotations(extensionContext.getTestClass(), FeatureFlag.class); + ExtensionContext.Store store = extensionContext.getStore( + ExtensionContext.Namespace.create(getClass(), extensionContext.getRequiredTestClass())); + + setFlag(extensionContext.getRequiredTestClass(), flags, getBroker(extensionContext), store); + } + + @Override + public void afterAll(ExtensionContext extensionContext) throws Exception { + List flags = AnnotationSupport.findRepeatableAnnotations(extensionContext.getTestClass(), FeatureFlag.class); + ExtensionContext.Store store = extensionContext.getStore( + ExtensionContext.Namespace.create(getClass(), extensionContext.getRequiredTestClass())); + + resetFlag(flags, getBroker(extensionContext), store); + } + + @Override + public void beforeTestExecution(ExtensionContext extensionContext) throws Exception { + List flags = AnnotationSupport.findRepeatableAnnotations(extensionContext.getTestMethod(), FeatureFlag.class); + ExtensionContext.Store store = extensionContext.getStore( + ExtensionContext.Namespace.create( + getClass(), + extensionContext.getRequiredTestClass(), + extensionContext.getRequiredTestMethod() + )); + + setFlag(extensionContext.getRequiredTestClass(), flags, getBroker(extensionContext), store); + } + + @Override + public void afterTestExecution(ExtensionContext extensionContext) throws Exception { + List flags = AnnotationSupport.findRepeatableAnnotations(extensionContext.getTestMethod(), FeatureFlag.class); + ExtensionContext.Store store = extensionContext.getStore( + ExtensionContext.Namespace.create( + getClass(), + extensionContext.getRequiredTestClass(), + extensionContext.getRequiredTestMethod() + )); + + resetFlag(flags, getBroker(extensionContext), store); + } + + private void setFlag(Class testClass, List flags, FeatureFlagBroker broker, ExtensionContext.Store store) throws Exception { + for (FeatureFlag flag : flags) { + // get the current state + Boolean oldState = broker.get(flag.flag()); + + // if present - store in context to restore later + if (oldState != null) { + store.put(flag, oldState); + } + + broker.set(flag.flag(), flag.value()); + } + } + + private void resetFlag(List flags, FeatureFlagBroker broker, ExtensionContext.Store store) throws Exception { + for (FeatureFlag flag : flags) { + // get a stored setting from context + Boolean oldState = store.remove(flag, Boolean.class); + + // if present before, restore + if (oldState != null) { + broker.set(flag.flag(), oldState); + // if NOT present before, delete + } else { + broker.delete(flag.flag()); + } + } + } + + private FeatureFlagBroker getBroker(ExtensionContext extensionContext) throws Exception { + // Is this test class using local system properties, then get a broker for these + if (AnnotationSupport.isAnnotated(extensionContext.getTestClass(), LocalFeatureFlags.class)) { + return LocalFeatureFlags.localBroker; + // NOTE: this might be extended later with other annotations to support other means of handling the settings + } else { + throw new IllegalStateException("You must provide the @LocalFeatureFlags annotation to the test class"); + } + } + +} diff --git a/src/test/java/edu/harvard/iq/dataverse/util/testing/LocalFeatureFlags.java b/src/test/java/edu/harvard/iq/dataverse/util/testing/LocalFeatureFlags.java new file mode 100644 index 00000000000..b9717a4816a --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/util/testing/LocalFeatureFlags.java @@ -0,0 +1,46 @@ +package edu.harvard.iq.dataverse.util.testing; + +import edu.harvard.iq.dataverse.settings.FeatureFlags; +import org.junit.jupiter.api.extension.ExtendWith; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * This annotation expresses that a test class wants to manipulate local + * settings (because the tests run within the same JVM as the code itself). + * This is mostly true for unit tests. + */ +@Retention(RetentionPolicy.RUNTIME) +@Target({ ElementType.TYPE }) +@ExtendWith(FeatureFlagExtension.class) +@Inherited +public @interface LocalFeatureFlags { + + FeatureFlagBroker localBroker = new FeatureFlagBroker() { + @Override + public Boolean get(FeatureFlags flag) { + // In case the setting wasn't set by us, return null. + if (System.getProperty(flag.getScopedKey()) == null) { + return null; + } + + // Otherwise: lookup via MPCONFIG as we need to take the default state into account, which is not exposed. + return flag.enabled(); + } + + @Override + public void set(FeatureFlags flag, boolean value) { + System.setProperty(flag.getScopedKey(), String.valueOf(value)); + } + + @Override + public void delete(FeatureFlags flag) { + System.clearProperty(flag.getScopedKey()); + } + }; + +} \ No newline at end of file