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.

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

improve the linked dataverse listing API #9665

Merged

Conversation

pkiraly
Copy link
Member

@pkiraly pkiraly commented Jun 20, 2023

What this PR does / why we need it:

This PR makes the response of the linked dataverse listing API more structured. Instead of returning a formatted string, it returns a structured JSON, where the individual data pieces are separated.

Which issue(s) this PR closes:

Closes #9650

Special notes for your reviewer:

Right now the API can be executed only by administrator. I think it might be changed so that the owner of the dataset is also able to execute it.

Suggestions on how to test this:

  1. build and deploy the war file
  2. create a link in a dataset
  3. use API to retrieve the list of linked dataverses via curl -s -H "X-Dataverse-key:$API_TOKEN" $SERVER_URL/api/datasets/$DATASET_ID/links

it will returns something like this:

{
  "status": "OK",
  "data": {
    "id": 5,
    "identifier": "FK2/OTCWMM",
    "linked-dataverses": [
      {
        "id": 2,
        "alias": "dataverse1",
        "displayName": "Lab experiments 2023 June"
      }
    ]
  }
}

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?:

It might be worth to mention about this API, which previously was not documented.

Additional documentation:

There were a previous PR about the documentation of the API.

@coveralls
Copy link

coveralls commented Jun 20, 2023

Coverage Status

coverage: 20.869% (-0.001%) from 20.87%
when pulling 18db193 on pkiraly:9650-5-improve-list-linked-dataverses-API
into 08249f5 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I like the structure! However, what about backward compatibility? 🤔

}
JsonObjectBuilder response = Json.createObjectBuilder();
response.add("dataverses that link to dataset id " + datasetId, dataversesThatLinkToThisDatasetIdBuilder);
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep this line in for backward compatibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll put it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pdurbin Sorry, do you mean the key of the object ("dataverses that link to dataset id X") or the value (e.g. "crc990 (id 18802)")?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I think you need both for backward compatibility. We'd want dataversesThatLinkToThisDatasetIdBuilder to have whatever value it has always had.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was decided today that there is no need for backward compatibility since the api was never documented

Copy link
Member

Choose a reason for hiding this comment

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

@pkiraly can you please add a release note snippet?

You can put it here:

/doc/release-notes/9650-5-improve-list-linked-dataverses-API.md

Copy link
Member Author

Choose a reason for hiding this comment

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

@pdurbin done

@pdurbin pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Feb 28, 2024
@scolapasta
Copy link
Contributor

If you are still interested in this PR, can you please merge and resolve any merge conflicts with the latest from develop? If so, we can prioritize reviewing and QAing the changes. If we don’t hear from you by May 22, 2024, we’ll go ahead and close this PR (it can always be reopened after that date, if there is still interest).

@pkiraly
Copy link
Member Author

pkiraly commented Apr 25, 2024

I will do it.

@pdurbin pdurbin added the Champion: pdurbin Championed by @pdurbin for inclusion in the next release label Jul 19, 2024
@pdurbin
Copy link
Member

pdurbin commented Jul 19, 2024

This is a nice, small API improvement that I think is a good candidate for 6.4.

@cmbz cmbz added the FY25 Sprint 6 FY25 Sprint 6 label Sep 11, 2024
@stevenwinship stevenwinship self-assigned this Sep 12, 2024
@stevenwinship stevenwinship force-pushed the 9650-5-improve-list-linked-dataverses-API branch from 3972a4d to 494c31b Compare September 12, 2024 16:09
@stevenwinship stevenwinship removed their assignment Sep 12, 2024
@pdurbin pdurbin changed the title #9650 improve the linked dataverse listing API improve the linked dataverse listing API Sep 12, 2024
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Please update the API changelog.

@@ -129,15 +129,21 @@ Lists the link(s) created between a dataset and a Dataverse collection (see the

curl -H "X-Dataverse-key: $API_TOKEN" http://$SERVER/api/datasets/$linked-dataset-id/links

It returns a list in the following format:
It returns a list in the following format (new format as of v6.4):
Copy link
Member

Choose a reason for hiding this comment

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

"It was decided today that there is no need for backward compatibility since the api was never documented"

Yes, we rarely document the output JSON, but clients can still rely on it.

Since the change is backward-incompatible, please update the API changelog ( https://guides.dataverse.org/en/latest/api/changelog.html ).

Copy link
Contributor

Choose a reason for hiding this comment

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

added

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Looks great.

@stevenwinship stevenwinship removed their assignment Sep 12, 2024
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I didn't run the code but tests are passing and it looks good to me. Approved.

@pdurbin pdurbin removed their assignment Sep 12, 2024
@pdurbin pdurbin removed the Champion: pdurbin Championed by @pdurbin for inclusion in the next release label Sep 16, 2024
@stevenwinship stevenwinship force-pushed the 9650-5-improve-list-linked-dataverses-API branch from 196ac01 to 54fad55 Compare September 25, 2024 14:24
@cmbz cmbz added the FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) label Sep 25, 2024
@ofahimIQSS ofahimIQSS self-assigned this Oct 2, 2024
@ofahimIQSS
Copy link
Contributor

ofahimIQSS commented Oct 2, 2024

Testing complete in internal.
Made sure non super users can't use this API (test passed).
Tested to ensure that when there is no linked data set, api call does not display data under linked database section.
Test where I linked the dataset to 3 collections:
image

image

@ofahimIQSS ofahimIQSS merged commit a0cb73d into IQSS:develop Oct 2, 2024
12 checks passed
@pdurbin pdurbin added this to the 6.5 milestone Oct 2, 2024
@ofahimIQSS ofahimIQSS removed their assignment Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 6 FY25 Sprint 6 FY25 Sprint 7 FY25 Sprint 7 (2024-09-25 - 2024-10-09) Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: Done 🧹
Development

Successfully merging this pull request may close these issues.

List of linked dataverses in UI and API
7 participants