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

expose links to all export formats via Signposting #11045

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

expose links to all export formats via Signposting #11045

wants to merge 7 commits into from

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Nov 22, 2024 •
edited
Loading

What this PR does / why we need it:

Especially for Croissant but really all export formats, we're interested in exposing the URLs for each format via Signposting so that crawlers (and API users) can efficiently get just the HEAD of a page (or linkset API) to get the URLs.

If Google and others adopt Signposting, it will mean they can do a HEAD, get the Croissant URL (for example), and download the Croissant file, which has the potential to be large. See also discussion at mlcommons/croissant#530 (comment) and especially this URL:

Which issue(s) this PR closes:

Special notes for your reviewer:

Please see the comment about how I changed the mimetype for our schema.org format.

I also did a fair amount of doc improvement. Feedback welcome. Heads up to @julian-schneider that I tweaked your docs in #10739 (just merged).

Suggestions on how to test this:

Try HEAD and GET on a published dataset, looking for the "Link header:

Try the linkset API

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

Yes, included.

Additional documentation:

A good entry point for doc changes: https://dataverse-guide--11045.org.readthedocs.build/en/11045/api/native-api.html#retrieve-signposting-information

@pdurbin pdurbin self-assigned this Nov 22, 2024
@coveralls
Copy link

coveralls commented Nov 22, 2024 •
edited
Loading

Coverage Status

coverage: 22.442% (-0.005%) from 22.447%
when pulling f704651 on 10542-signposting
into f95c1a0 on develop.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pdurbin pdurbin marked this pull request as ready for review November 22, 2024 21:16
@pdurbin pdurbin removed their assignment Nov 22, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
qqmyers
qqmyers requested changes Nov 22, 2024
View reviewed changes
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

Looks good. Nice to have all our formats in Signposting - we should make sure Herbert vdS knows. I suggested on change to avoid problems with permalinks.

src/main/java/edu/harvard/iq/dataverse/util/SignpostingResources.java Outdated
try {
exporter = ExportService.getInstance().getExporter(formatName);
describedby += ",<" + systemConfig.getDataverseSiteUrl() + "/api/datasets/export?exporter=" + formatName + "&persistentId="
+ ds.getProtocol() + ":" + ds.getAuthority() + "/" + ds.getIdentifier() + ">;rel=\"describedby\"" + ";type=\"" + exporter.getMediaType() + "\"";
Copy link
Member

@qqmyers qqmyers Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These (this and line 137) won't work for all permalinks since they don't necessarily have / as a separator. I think you can just ds.getGlobalId().asString() instead. For a real dataset, I don't think you can ever have a null GlobalId so not sure you even need to check for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@qqmyers thanks, in ca93d60 I corrected the two you found plus two more.

@cmbz cmbz added the FY25 Sprint 11 FY25 Sprint 11 (2024-11-20 - 2024-12-04) label Nov 23, 2024
@pdurbin pdurbin self-assigned this Nov 24, 2024
@pdurbin pdurbin assigned qqmyers and unassigned pdurbin Nov 25, 2024
qqmyers
qqmyers approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

Looks good.

@qqmyers qqmyers removed their assignment Nov 25, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pdurbin
Copy link
Member Author

pdurbin commented Nov 25, 2024

Whoops, tests are failing. https://jenkins.dataverse.org/blue/organizations/jenkins/IQSS-Dataverse-Develop-PR/detail/PR-11045/3/tests

I'll take a look.

@pdurbin pdurbin self-assigned this Nov 25, 2024
@pdurbin
fix test and update changelog about schema.org content type change #1…
a92366e 
…0542

The test file is used in InfoIT#testGetExportFormats
@pdurbin
fix broken header #10542
8a0102b
@pdurbin
remove baseurl to get test to pass on Jenkins #10542
f704651 
Before this PR...

In development:

Expected: is "http://localhost:8080/dataset.xhtml?persistentId=doi:10.5072/FK2/6A3292"
  Actual: is "http://localhost:8080/dataset.xhtml?persistentId=doi:10.5072/FK2/6A3292"

On Jenkins

Expected: is "http://localhost:8080/dataset.xhtml?persistentId=doi:10.5072/FK2/6A3292"
  Actual: http://ec2-3-225-221-142.compute-1.amazonaws.com/dataset.xhtml?persistentId=doi:10.5072/FK2/6A3292

So we'll change to just "endsWith" since we aren't actually testing the baseurl,
just the datasetPid which we fixed up in ca93d60.
@pdurbin pdurbin assigned qqmyers and unassigned pdurbin Nov 25, 2024
@pdurbin
Copy link
Member Author

pdurbin commented Nov 25, 2024

I pushed some commits to fix the broken tests, update the API changelog and release note, and fix a broken header in the docs.

This test was failing in Jenkins: mvn test -Dtest=FilesIT#testDeleteFile. I ran it locally and it seems fine. I think it's unrelated to this PR. Here's the failure...

JSON path data.files[0].dataFile.filename doesn't match.
Expected: cc0.png
  Actual: orcid_16x16.png

... on this line:

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

Line 2278 in f95c1a0

.body("data.files[0].dataFile.filename", equalTo("cc0.png"))

Again, I don't think it has anything to do with changes in the PR but I'm just mentioning it in case we start seeing it elsewhere.

@qqmyers if you would take another look I'd appreciate it!

@pdurbin
Copy link
Member Author

pdurbin commented Nov 25, 2024

By the way, @4tikhonov pointed out to me that Dataverse shows up in this report of who has implemented Signposting: https://s11.no/2024/signposting-report/

@github-actions GitHub Actions
Copy link

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:10542-signposting

ghcr.io/gdcc/configbaker:10542-signposting

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

qqmyers
qqmyers approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

Updates look fine. I agree the one test failure is probably unrelated to the PR - possibly a timing issue again.

@qqmyers qqmyers removed their assignment Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qqmyers qqmyers qqmyers approved these changes

Assignees
No one assigned
Labels
FY25 Sprint 11 FY25 Sprint 11 (2024-11-20 - 2024-12-04)
Projects
IQSS Dataverse Project
Status: Ready for QA ⏩
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Croissant to Signposting "describedby" output
4 participants
@pdurbin @coveralls @qqmyers @cmbz