Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add getZipDownloadLimit and getMaxEmbargoDurationInMonths API info endpoints #9881

Merged
merged 11 commits into from
Sep 13, 2023
Merged

Add getZipDownloadLimit and getMaxEmbargoDurationInMonths API info endpoints #9881

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2ec5d36
Added: getZipDownloadLimit and getEmbargoEnabled API info endpoints
GPortas Sep 6, 2023
7dd6ca8
Added: docs for new info API endpoints
GPortas Sep 6, 2023
e0bac9c
Fixed: missing guides reference in config.rst
GPortas Sep 6, 2023
75b6832
stub out page on API design, esp paths #9880
pdurbin Sep 7, 2023
9726451
Changed: :MaxEmbargoDurationInMonths setting directly exposed via API…
GPortas Sep 11, 2023
d3d305c
Changed: updated release notes
GPortas Sep 11, 2023
f754b71
Changed: private Info.java method renamed
GPortas Sep 11, 2023
7929c4f
remove embargo example, no longer used in #9881
pdurbin Sep 11, 2023
6e08bad
typo #9880
pdurbin Sep 11, 2023
703a2a0
Merge pull request #9890 from IQSS/9880-api-design-docs
GPortas Sep 11, 2023
5ce8c90
Merge branch 'develop' of github.com:IQSS/dataverse into 9880-info-zi…
GPortas Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/release-notes/9880-info-api-zip-limit-embargo.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Implemented the following new endpoints:

- getZipDownloadLimit (/api/info/zipDownloadLimit): Get the configured zip file download limit. The response contains the long value of the limit in bytes.

- getEmbargoEnabled (/api/info/embargoEnabled): Know if the Dataverse instance has been configured to allow embargoes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something feels off to me...

We should probably make an active decision about two things:

  • Should we continue putting "settings" in the path?
  • Should we match the name of the setting including the colon?

Another way to ask this is which of the following we want.

/api/info/zipDownloadLimit
/api/info/settings/zipDownloadLimit
/api/info/settings/:ZipDownloadLimit

For reference here is a sorted list of "info" endpoints as of this PR:

/api/info/apiTermsOfUse
/api/info/embargoEnabled
/api/info/server
/api/info/settings/:DatasetPublishPopupCustomText
/api/info/settings/incompleteMetadataViaApi
/api/info/version
/api/info/zipDownloadLimit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems appropriate to me to add the setting subpath, and even include the colon if it is more understandable for the setting name, as long as the endpoint only reads the setting value, with no other operations around it.

For example, the /admin/settings/{name} endpoint returns the plain value of the setting if it exists, otherwise it returns a not found error.

On the other hand, the info/zipDownloadLimitendpoint does more than read the setting, since if the associated setting is not found it assigns a default value. So I find appropriate to exclude the "settings" endpoint subpath/naming here.

Same for the info/embargoEnabled endpoint, that after reading the :MaxEmbargoDurationInMonths setting, based on its possible values, checks whether the embargo functionality is enabled or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GPortas I wrote some docs to merge into this PR once we are happy with them:

37 changes: 37 additions & 0 deletions doc/sphinx-guides/source/api/native-api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3330,6 +3330,43 @@ The fully expanded example above (without environment variables) looks like this

curl "https://demo.dataverse.org/api/info/settings/incompleteMetadataViaApi"

Get Zip File Download Limit
~~~~~~~~~~~~~~~~~~~~~~~~~~~

Get the configured zip file download limit. The response contains the long value of the limit in bytes.

This limit comes from the database setting :ref:`:ZipDownloadLimit` if set, or the default value if the database setting is not set, which is 104857600 (100MB).

.. code-block:: bash

export SERVER_URL=https://demo.dataverse.org

curl "$SERVER_URL/api/info/zipDownloadLimit"

The fully expanded example above (without environment variables) looks like this:

.. code-block:: bash

curl "https://demo.dataverse.org/api/info/zipDownloadLimit"

Show Support Of The Embargo Feature
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Know if the Dataverse instance has been configured to allow embargoes.

The endpoint checks whether the database setting :ref:`:MaxEmbargoDurationInMonths`, which enables the embargo feature, has a value that enables the feature or not.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, will the SPA (or some other system) need to know the value of :MaxEmbargoDurationInMonths? If so, should we return it now, as part of the endpoint we're adding? Or would we add a second endpoint to get the number of months?

Copy link
Contributor Author

@GPortas GPortas Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not entirely sure, but I don't think so, considering that the embargo duration is something that is managed internally by the setting, and that for now, I've only seen that the UI needs to know if the feature is enable (To show / not show related UI) or if a file is embargoed, but not the configured embargo duration.

In any case, if necessary in the future, we can extend the endpoint to include the setting value or create a separate endpoint. For now I have preferred to keep it simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the SPA will need to perform some validation when we get to the EDIT side of the dataset page. I can imagine an error like this:

"Please an embargo date before September 8, 2025" (for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. When creating an embargo, there is a date picker that I was overlooking where dates within the range determined by the :MaxEmbargoDurationInMonths setting are displayed. So the SPA has to know the value of the setting beforehand.

I think then it is better to directly expose the setting. Something like: /api/info/settings/:MaxEmbargoDurationInMonths

Then the SPA/consumer will know if the feature is enabled (by having a value set different than 0) and the embargo duration in months.

What do you think? @pdurbin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, directly exposing the setting sounds good.


.. code-block:: bash

export SERVER_URL=https://demo.dataverse.org

curl "$SERVER_URL/api/info/embargoEnabled"

The fully expanded example above (without environment variables) looks like this:

.. code-block:: bash

curl "https://demo.dataverse.org/api/info/embargoEnabled"

.. _metadata-blocks-api:

Expand Down
2 changes: 2 additions & 0 deletions doc/sphinx-guides/source/installation/config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2980,6 +2980,8 @@ This setting controls the number of files that can be uploaded through the UI at

``curl -X PUT -d 500 http://localhost:8080/api/admin/settings/:MultipleUploadFilesLimit``

.. _:ZipDownloadLimit:

:ZipDownloadLimit
+++++++++++++++++

Expand Down
6 changes: 6 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/api/AbstractApiBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,12 @@ protected Response ok( boolean value ) {
.add("data", value).build() ).build();
}

protected Response ok(long value) {
return Response.ok().entity(Json.createObjectBuilder()
.add("status", ApiConstants.STATUS_OK)
.add("data", value).build()).build();
}

/**
* @param data Payload to return.
* @param mediaType Non-JSON media type.
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/api/Info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,18 @@ public Response getTermsOfUse(@Context ContainerRequestContext crc) {
public Response getAllowsIncompleteMetadata() {
return ok(JvmSettings.API_ALLOW_INCOMPLETE_METADATA.lookupOptional(Boolean.class).orElse(false));
}

@GET
@Path("zipDownloadLimit")
public Response getZipDownloadLimit() {
long zipDownloadLimit = SystemConfig.getLongLimitFromStringOrDefault(settingsSvc.getValueForKey(SettingsServiceBean.Key.ZipDownloadLimit), SystemConfig.defaultZipDownloadLimit);
return ok(zipDownloadLimit);
}

@GET
@Path("embargoEnabled")
public Response getEmbargoEnabled() {
String setting = settingsSvc.getValueForKey(SettingsServiceBean.Key.MaxEmbargoDurationInMonths);
return ok(setting != null && !setting.equals("0"));
}
}
58 changes: 53 additions & 5 deletions src/test/java/edu/harvard/iq/dataverse/api/InfoIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,27 @@
import static io.restassured.RestAssured.given;
import io.restassured.response.Response;
import edu.harvard.iq.dataverse.settings.SettingsServiceBean;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

import static jakarta.ws.rs.core.Response.Status.NOT_FOUND;
import static jakarta.ws.rs.core.Response.Status.OK;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.notNullValue;

public class InfoIT {

@BeforeAll
public static void setUpClass() {
UtilIT.deleteSetting(SettingsServiceBean.Key.MaxEmbargoDurationInMonths);
}

@AfterAll
public static void afterClass() {
UtilIT.deleteSetting(SettingsServiceBean.Key.MaxEmbargoDurationInMonths);
}

@Test
public void testGetDatasetPublishPopupCustomText() {

Expand All @@ -21,7 +35,7 @@ public void testGetDatasetPublishPopupCustomText() {
Response response = given().urlEncodingEnabled(false)
.get("/api/info/settings/" + SettingsServiceBean.Key.DatasetPublishPopupCustomText);
response.prettyPrint();
response.then().assertThat().statusCode(200)
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data.message", equalTo("Hello world!"));

given().urlEncodingEnabled(false)
Expand All @@ -31,7 +45,7 @@ public void testGetDatasetPublishPopupCustomText() {
response = given().urlEncodingEnabled(false)
.get("/api/info/settings/" + SettingsServiceBean.Key.DatasetPublishPopupCustomText);
response.prettyPrint();
response.then().assertThat().statusCode(404)
response.then().assertThat().statusCode(NOT_FOUND.getStatusCode())
.body("message", equalTo("Setting "
+ SettingsServiceBean.Key.DatasetPublishPopupCustomText
+ " not found"));
Expand All @@ -42,7 +56,7 @@ public void testGetVersion() {
Response response = given().urlEncodingEnabled(false)
.get("/api/info/version");
response.prettyPrint();
response.then().assertThat().statusCode(200)
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data.version", notNullValue());
}

Expand All @@ -51,7 +65,7 @@ public void testGetServer() {
Response response = given().urlEncodingEnabled(false)
.get("/api/info/server");
response.prettyPrint();
response.then().assertThat().statusCode(200)
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data.message", notNullValue());
}

Expand All @@ -60,7 +74,41 @@ public void getTermsOfUse() {
Response response = given().urlEncodingEnabled(false)
.get("/api/info/apiTermsOfUse");
response.prettyPrint();
response.then().assertThat().statusCode(200)
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data.message", notNullValue());
}

@Test
public void getAllowsIncompleteMetadata() {
Response response = given().urlEncodingEnabled(false)
.get("/api/info/settings/incompleteMetadataViaApi");
response.prettyPrint();
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data", notNullValue());
}

@Test
public void getZipDownloadLimit() {
Response response = given().urlEncodingEnabled(false)
.get("/api/info/zipDownloadLimit");
response.prettyPrint();
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data", notNullValue());
}

@Test
public void getEmbargoEnabled() {
String testEndpoint = "/api/info/embargoEnabled";
// Embargo disabled
Response response = given().urlEncodingEnabled(false).get(testEndpoint);
response.prettyPrint();
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data", equalTo(false));
// Embargo enabled
UtilIT.setSetting(SettingsServiceBean.Key.MaxEmbargoDurationInMonths, "12");
response = given().urlEncodingEnabled(false).get(testEndpoint);
response.prettyPrint();
response.then().assertThat().statusCode(OK.getStatusCode())
.body("data", equalTo(true));
}
}
Loading