-
Notifications
You must be signed in to change notification settings - Fork 492
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
improve the linked dataverse listing API #9665
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)")?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdurbin done
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). |
I will do it. |
This is a nice, small API improvement that I think is a good candidate for 6.4. |
3972a4d
to
494c31b
Compare
There was a problem hiding this 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): |
There was a problem hiding this comment.
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 ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks great.
There was a problem hiding this 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.
196ac01
to
54fad55
Compare
changes made in #9665 belong in API changelog for 6.5, not 6.4
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:
curl -s -H "X-Dataverse-key:$API_TOKEN" $SERVER_URL/api/datasets/$DATASET_ID/links
it will returns something like this:
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.