10286 add owner info #10322

sekmiller · 2024-02-14T19:29:08Z

What this PR does / why we need it: Adds info about the owners of an object for use by the SPA to display breadcrumbs

Which issue(s) this PR closes:

Closes #10286 Extend the API to include owner information for dv objects

Special notes for your reviewer:
I added a boolean of "includeOwners" to the json printers of the various objects. Per a suggestion from Guillermo I didn't include the object itself in the owners array.

Suggestions on how to test this: add a query param of "returnOwners=true" to any of the get object apis. You should see an array of each owning object up to the root dv collection.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?:

Additional documentation:

coveralls · 2024-02-14T19:37:17Z

coverage: 20.27% (-0.005%) from 20.275%
when pulling 9790d23 on 10286-add-owner-info
into de45c13 on develop.

src/main/java/edu/harvard/iq/dataverse/api/Datasets.java

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

MellyGray · 2024-02-19T15:59:12Z

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

+                }
+                if (dvo.isInstanceofDataverse()){
+                    Dataverse in = (Dataverse) dvo;
+                    ownerObject.add("identifier", in.getAlias());


It would be beneficial to include the version as well to the ownerObject, because for a file, its owner might be the dataset doi:10.5072/FK2/WTBMGC with version=2.0, but its owner is not the same dataset at an earlier version such as 1.0, because the file was added in a later version. I hope I'm making myself clear

I added the logic for including a version number, but since we're omitting the base object from the isPartOf it can only apply to Files, however there's no endpoint for getting a file by its version like there is for dataset - the current result is that if I use the getFileData (@path("{id}")) it gets me the latest available version

@MellyGray @GPortas I think I need #10280 to be completed before I can finish this the way it needs to be finished.

Thanks @sekmiller , I was testing the last changes. identifier includes both the identifier and the version, this can complicate parsing and utilizing these distinct pieces of information separately. I suggest separating the version information from the DOI identifier

{ "type": DvObjectType.DATASET, "identifier": "doi:10.5072/FK2/HEGZLV", "version": "DRAFT", // New dedicated property for version information "displayName": "First Dataset", "isPartOf": { "type": DvObjectType.DATAVERSE, "identifier": "root", "displayName": "Root" } }

Sure. I can do that. The reason I had put them together is that the url for the dataset page currently looks like this: http://localhost:8080/dataset.xhtml?persistentId=doi:10.5072/FK2/UE0DKV&version=2.0

Got it, @sekmiller! I see where you're coming from, but we're aiming to follow an API-first approach to decouple the API implementation from the UI use cases, ensuring our API remains flexible for various applications. We don't know who is going to consume this endpoint, so we should keep these two pieces of information separate to allow them to be used independently. Thanks for implementing the suggestion

GPortas · 2024-02-20T12:11:05Z

src/main/java/edu/harvard/iq/dataverse/api/Datasets.java

        return response( req -> {
            final Dataset retrieved = execCommand(new GetDatasetCommand(req, findDatasetOrDie(id)));
            final DatasetVersion latest = execCommand(new GetLatestAccessibleDatasetVersionCommand(req, retrieved));
-            final JsonObjectBuilder jsonbuilder = json(retrieved);
+            Boolean includeOwners = returnOwners == null ? false : returnOwners;


You can directly use the primitive boolean type: @QueryParam("returnOwners") boolean returnOwners

And thus avoid the null check. If the query param is not set in the URL, false is the default value.

Same for other endpoints in this PR.

GPortas · 2024-02-20T12:14:37Z

src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java

@@ -610,10 +610,12 @@ private Dataset parseDataset(String datasetJson) throws WrappedResponse {
    @GET
    @AuthRequired
    @Path("{identifier}")
-    public Response viewDataverse(@Context ContainerRequestContext crc, @PathParam("identifier") String idtf) {
+    public Response viewDataverse(@Context ContainerRequestContext crc, @PathParam("identifier") String idtf, @QueryParam("returnOwners") Boolean returnOwners) {


To be consistent with method naming in other places, why not rename the endpoint method from "viewDataverse" to "getDataverse"?

GPortas · 2024-02-20T12:17:59Z

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

+                if (dvo.isInstanceofDataset()){
+                    ownerObject.add("type", "DATASET");
+                }
+                 if (dvo.isInstanceofDataFile()){


There is an indentation issue in this line

GPortas · 2024-02-20T12:24:41Z

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

+
+        for (DvObject dvo : ownerList){
+                JsonObjectBuilder ownerObject = jsonObjectBuilder();
+                if (dvo.isInstanceofDataverse()){


I would extract the type check to a separate method to improve readability, and try to use a switch statement instead of these ifs. As far as I know a dvObject can't be of more than one type, and by having these ifs without else ifs it seems that a dvObject can have multiple types which is confusing.

I simplified it here because since we're ignoring the object itself we can only have a Dataverse or a Dataset

into 10286-add-owner-info

GPortas

Thanks for applying the requested changes.

Although overall it looks good, I have left a couple of comments.

I have tested the following endpoints and they work as expected:

Files
curl "http://localhost:8080/api/v1/files/5?returnOwners=true"

The payload includes the isPartOf object, which looks good:

"isPartOf":{
  "type":"DATASET",
  "persistentIdentifier":"doi:10.5072/FK2/JSKMZS",
  "identifier":2,
  "version":"1.0",
  "displayName":"test",
  "isPartOf":{
     "type":"DATAVERSE",
     "identifier":"root",
     "displayName":"Root"
  }
}

Dataset version
curl "http://localhost:8080/api/v1/datasets/2/versions/:latest?includeFiles=false&returnOwners=true"

The isPartOf object looks good:

"isPartOf": {
      "type": "DATAVERSE",
      "identifier": "root",
      "displayName": "Root"
},

Collections

First, I requested the root collection (curl "http://localhost:8080/api/v1/dataverses/root?returnOwners=true").

No owners retrieved, which makes sense.

Then, I created a sub collection root called "child", and a sub collection in "child" called "child1", and called the endpoint for "child1". (curl "http://localhost:8080/api/v1/dataverses/child1?returnOwners=true")

The isPartOf looks good:

"isPartOf": {
      "type": "DATAVERSE",
      "identifier": "child",
      "displayName": "child",
      "isPartOf": {
        "type": "DATAVERSE",
        "identifier": "root",
        "displayName": "Root"
      }
    },

GPortas · 2024-02-27T11:46:59Z

src/main/java/edu/harvard/iq/dataverse/api/Datasets.java

@@ -421,6 +421,7 @@ public Response getVersion(@Context ContainerRequestContext crc,
                               @PathParam("versionId") String versionId,
                               @QueryParam("excludeFiles") Boolean excludeFiles,
                               @QueryParam("includeDeaccessioned") boolean includeDeaccessioned,
+                               @QueryParam("returnOwners") boolean includeOwners,


I would rename the parameter includeOwners to returnOwners to match the query param, as in other endpoints.

GPortas · 2024-02-27T11:47:14Z

src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java

@@ -610,10 +610,12 @@ private Dataset parseDataset(String datasetJson) throws WrappedResponse {
    @GET
    @AuthRequired
    @Path("{identifier}")
-    public Response viewDataverse(@Context ContainerRequestContext crc, @PathParam("identifier") String idtf) {
+    public Response getDataverse(@Context ContainerRequestContext crc, @PathParam("identifier") String idtf, @QueryParam("returnOwners") Boolean returnOwners) {
+        Boolean includeOwners = returnOwners == null ? false : returnOwners;


You can avoid this using boolean primitive, as I mentioned in the first review.

GPortas · 2024-02-27T11:50:01Z

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

+    public static JsonObjectBuilder json(DatasetVersion dsv, List<String> anonymizedFieldTypeNamesList, boolean includeFiles, boolean includeOwners) {
+    /*    return json(dsv, null, includeFiles, null);
+    }
+    public static JsonObjectBuilder json(DatasetVersion dsv, List<String> anonymizedFieldTypeNamesList, boolean includeFiles, Long numberOfFiles) {*/


Can we remove this commented code?

GPortas · 2024-02-27T11:51:56Z

src/test/java/edu/harvard/iq/dataverse/api/FilesIT.java

+                .body("data.dataFile.filesize", equalTo(8361))
+                .statusCode(OK.getStatusCode());
+
+        getFileDataResponse.then().assertThat().body("data.dataFile.isPartOf.identifier", equalTo(datasetId));


Can we add an assertion to check the persistentIdentifier?

missed this one, sorry.

OK. it's in now...

GPortas · 2024-02-27T13:04:20Z

doc/sphinx-guides/source/api/native-api.rst

+.. code-block:: bash
+
+  curl -H "X-Dataverse-key:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" "https://demo.dataverse.org/api/files/:persistentId/versions/:draft?persistentId=doi:10.5072/FK2/J8SJZB&returnOwners=true"
+


Updated docs for getPrivateUrlDatasetVersion are missing, if I am not wrong.

This reverts commit a34d1ec.

github-actions · 2024-02-27T17:03:30Z

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:10286-add-owner-info

ghcr.io/gdcc/configbaker:10286-add-owner-info

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.

GPortas

LGTM. Thank you for applying the changes @sekmiller.

sekmiller added 10 commits February 8, 2024 09:06

#10286 add breadcrumbs to dataset api

8abeaf0

Merge branch 'develop' into 10286-add-owner-info

9fdebb3

#10286 move owner type to beginning

572b9cb

#10286 update pathparam name/terms

889e942

#10286 add owner array to file api

6e51bbc

#10286 add owner array to view dv

43f61e6

Merge branch 'develop' into 10286-add-owner-info

43f8706

#10286 add test for get ds api

1898c14

#10286 add integration tests

a828fd1

Merge branch 'develop' into 10286-add-owner-info

9fd7c48

This comment has been minimized.

Sign in to view

#10286 delete test data

923f02e

This comment has been minimized.

Sign in to view

Merge branch 'develop' into 10286-add-owner-info

5ba6a1a

This comment has been minimized.

Sign in to view

Merge branch 'develop' into 10286-add-owner-info

d3482af

This comment has been minimized.

Sign in to view

GPortas mentioned this pull request Feb 15, 2024

Integrate breadcrumbs with new owner information in js-dataverse IQSS/dataverse-frontend#311

Closed

GPortas assigned GPortas and MellyGray Feb 19, 2024

MellyGray reviewed Feb 19, 2024

View reviewed changes

MellyGray removed their assignment Feb 20, 2024

GPortas reviewed Feb 20, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

sekmiller added 4 commits February 26, 2024 15:27

#10286 add release note

14a817c

Update native-api.rst

a34d1ec

Merge branch '10286-add-owner-info' of https://github.com/IQSS/dataverse

b17d3d9

into 10286-add-owner-info

#10286 remove debug code

3751638

sekmiller assigned GPortas and unassigned sekmiller Feb 26, 2024

This comment has been minimized.

Sign in to view

GPortas requested changes Feb 27, 2024

View reviewed changes

GPortas assigned sekmiller and unassigned GPortas Feb 27, 2024

sekmiller added 2 commits February 27, 2024 09:22

#10286 code cleanup

34c0f67

Revert "Update native-api.rst"

ae132a3

This reverts commit a34d1ec.

This comment has been minimized.

Sign in to view

#10286 one more test

da77732

This comment has been minimized.

Sign in to view

Update native-api.rst

9790d23

sekmiller assigned GPortas and unassigned sekmiller Feb 27, 2024

GPortas approved these changes Feb 27, 2024

View reviewed changes

GPortas removed their assignment Feb 27, 2024

jp-tosca self-assigned this Feb 28, 2024

jp-tosca merged commit dcd7c22 into develop Feb 28, 2024
12 checks passed

pdurbin added this to the 6.2 milestone Feb 29, 2024

cmbz mentioned this pull request Mar 1, 2024

Epic: GREI - Dataverse Rearchitecture IQSS/dataverse-pm#123

Open

landreev mentioned this pull request Mar 12, 2024

Fixing the null-unsafe bug in JsonPrinter #10365 #10366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

10286 add owner info #10322

10286 add owner info #10322

sekmiller commented Feb 14, 2024

coveralls commented Feb 14, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MellyGray Feb 19, 2024 •

edited

Loading

sekmiller Feb 21, 2024

sekmiller Feb 22, 2024

MellyGray Feb 22, 2024

sekmiller Feb 22, 2024

MellyGray Feb 23, 2024

GPortas Feb 20, 2024 •

edited

Loading

GPortas Feb 20, 2024

GPortas Feb 20, 2024

GPortas Feb 20, 2024

sekmiller Feb 21, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GPortas left a comment •

edited

Loading

GPortas Feb 27, 2024

GPortas Feb 27, 2024

GPortas Feb 27, 2024

GPortas Feb 27, 2024

sekmiller Feb 27, 2024

sekmiller Feb 27, 2024

GPortas Feb 27, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 27, 2024

GPortas left a comment

		.. code-block:: bash

		curl -H "X-Dataverse-key:xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" "https://demo.dataverse.org/api/files/:persistentId/versions/:draft?persistentId=doi:10.5072/FK2/J8SJZB&returnOwners=true"

10286 add owner info #10322

10286 add owner info #10322

Conversation

sekmiller commented Feb 14, 2024

coveralls commented Feb 14, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MellyGray Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GPortas Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GPortas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 27, 2024

GPortas left a comment

Choose a reason for hiding this comment

coveralls commented Feb 14, 2024 •

edited

Loading

MellyGray Feb 19, 2024 •

edited

Loading

GPortas Feb 20, 2024 •

edited

Loading

GPortas left a comment •

edited

Loading