-
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
Versions API improvements (9763) #9883
Conversation
… the (default) output of the api. (#9763)
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.
Just some quick comments.
return embargoCitationDate; | ||
} | ||
} | ||
// @todo: remove this commented-out code once/if the PR passes review - L.A. |
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.
Yes, good to remove commented out code.
src/main/java/edu/harvard/iq/dataverse/search/SearchIncludeFragment.java
Outdated
Show resolved
Hide resolved
Newest, | ||
Oldest, | ||
Size, | ||
Type |
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 recognize this from #9693. Does it matter which PR is merged first?
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 would prefer #9693 to be merged first, and then resolve the conflict in this branch (I added a database lookup optimization on top of the code introduced in that PR).
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
…ults to "true". (#9763)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…lier PR, as part of a quick fix for #9803
This comment has been minimized.
This comment has been minimized.
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.
As discussed at standup, @landreev sounds happy to add some tests so I'm returning this to him. Otherwise, I'd say it's ready for QA.
This comment has been minimized.
This comment has been minimized.
avoid conflict with V6.0.0.1__9599-guestbook-at-request.sql
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 it looks good. Conflicts resolved. Tests added. Docs look good.
I did notice a sql script naming conflict so I bumped the version. And I merged the latest from develop.
I'm only holding off on moving this to QA because Jenkins tests are failing (the "fire off installer" step). I started a thread in Slack about it: https://iqss.slack.com/archives/C010LA04BCG/p1697206082190619
This comment has been minimized.
This comment has been minimized.
I just kicked this off to get a second opinion from our container-based API test runner: https://github.com/gdcc/api-test-runner/actions/runs/6509472756 |
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.
Jenkins is still failing but GitHub Actions passed: https://github.com/gdcc/api-test-runner/actions/runs/6509472756
Approved.
I ran Jenkins again. It's now saying @landreev should we be worried about this? From https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9883/17/consoleFull |
This comment has been minimized.
This comment has been minimized.
1 similar comment
📦 Pushed preview images as
🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name. |
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.
We're having some testing challenges within Jenkins at the moment...
- https://iqss.slack.com/archives/C010LA04BCG/p1697466092510429
- https://iqss.slack.com/archives/C010LA04BCG/p1697466281385699
... but I'm pretty sure they are unrelated to this PR. (Tests passed in GitHub Actions.) Moving to ready for QA.
What this PR does / why we need it:
(copy-and-pasting from the comments in the linked issue):
I'm addressing the performance issues in the versions api via a combination of the following approaches:
/api/datasets/{id}/versions
includeFiles
is added to both/api/datasets/{id}/versions
and/api/datasets/{id}/versions/{vid}
(true by default), providing an option to drop the file information from the outputThe following real life datasets from IQSS prod. service are used in the sample tests below:
/api/datasets/{id}/versions
:/api/datasets/{id}/versions/{vid}
:Note:
/api/datasets/4554342/versions
and/api/datasets/4554342/versions/draft
produce the same amount of output because the former api was called without authentication, so only one, published version was included.Note: tests above were run on the dedicated IQSS test system (not in prod., which is a beefier and faster system).
Which issue(s) this PR closes:
Closes #9763
Special notes for your reviewer:
Please take an extra look at the framework for caching the adjusted "embargo publication date" that I added, replacing the real time lookup that used to be in
Dataset.getCitationDatet()
, and especially the flyway script added in the PR.Suggestions on how to test this:
The description above outlines the way I tested the branch during dev.
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: