-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add support for deposit to the DSpace REST API #135
base: main
Are you sure you want to change the base?
Conversation
fec504f
to
d4a366b
Compare
6ce178a
to
d750107
Compare
5bf461e
to
c84a802
Compare
…guration properties.
94a9f14
to
d34fdf5
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.
Nice job, Mark! Just a few comments, but in general it looks good. I still need to test and review the other PRs, but this is a first pass at the review for this main PR.
throw new RuntimeException("SourceMD with id '" + id + "' not found."); | ||
} | ||
|
||
private SourceMD createSourceMd() throws METSException { |
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.
Do the method deletions in this class affect the sword deposit for dspace? I know we are going to deprecate the sword deposit eventually, but should these changes be done at that time?
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.
These were unused methods. It wasn't really meaningful to delete them. But I did it during another cleanup.
...services/deposit-core/src/main/java/org/eclipse/pass/deposit/provider/util/CitationUtil.java
Show resolved
Hide resolved
...services/deposit-core/src/main/java/org/eclipse/pass/deposit/provider/util/CitationUtil.java
Show resolved
Hide resolved
deposit.setDepositStatusRef(packageStream.metadata().packageDepositStatusRef()); | ||
|
||
// Only set deposit status ref if not already set during transport | ||
if (deposit.getDepositStatusRef() == null) { |
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.
What about the retry case? Is it possible deposit.getDepositStatusRef()
has a value, then after retry it needs to be updated?
|
||
public boolean verifyConnectivity() { | ||
URI uri = URI.create(dspaceApiUrl); | ||
return repositoryConnectivityService.verifyConnect(uri.getHost(), uri.getPort()); |
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.
Probably want to use repositoryConnectivityService .verifyConnectByURL(dspaceApiUrl )
.
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'm not sure if that url would work. I think it would give a 404.
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 sort of see this as making sure the host is running and can be connected to on a port. The problem is that dspaceApiUrl is not a valid service call. It's the base for other service calls. This is what I was writing and then I did some testing. It turns out that, completely undocumented, that it is in fact a service endpoint and returns links to the other service endpoints. There is actually an health endpoint that could be used, but just the api service endpoint seems fine.
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.
the api service endpoint seems fine
-> yes, better to use this to cover cases where response code is > 500, like 502 Bad Gateway
when the server is unreachable but there is a proxy in front of it.
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.
Done.
.header(AUTHORIZATION, authContext.authToken()) | ||
.header(X_XSRF_TOKEN, authContext.xsrfToken()) | ||
.header(COOKIE, DSPACE_XSRF_COOKIE + authContext.xsrfToken()) | ||
.body(body) |
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.
Do you know what the spring RestClient does with a post body like this, will it stream the contents of the Resources to the server? I ask for concern of memory for large files that risk of OOM exceptions?
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 believe it will stream, but I will poke around.
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.
The body is composed of resources and the default is to stream, not buffer. I tested with some larger files and I think it is ok.
...ices/deposit-core/src/main/java/org/eclipse/pass/deposit/transport/dspace/DSpaceSession.java
Outdated
Show resolved
Hide resolved
...deposit-core/src/main/java/org/eclipse/pass/deposit/support/dspace/DSpaceDepositService.java
Outdated
Show resolved
Hide resolved
...deposit-core/src/main/java/org/eclipse/pass/deposit/support/dspace/DSpaceDepositService.java
Show resolved
Hide resolved
|
||
AuthContext authContext = dspaceDepositService.authenticate(); | ||
|
||
// Create WorkspaceItem if it does not already exist |
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.
Style comment: this method a bit long. The logic in this method could be split up into maybe three methods like createWorkspace
, updateMetadata
, and createWorkflow
. These methods can then be called from this one.
try { | ||
DepositSubmission depositSubmission = packageStream.getDepositSubmission(); | ||
|
||
LOG.info("Processing Dspace Deposit for Submission: {}", depositSubmission.getId()); |
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 change to LOG.warn
so it shows up in default deployed logging level.
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.
This seems a bit odd to be at level warn instead of info. As in this isn't a warning. This is normal operation.
String itemUuid = workspaceItemContext.read("$._embedded.workspaceitems[0]._embedded.item.uuid"); | ||
String accessUrl = dspaceDepositService.createAccessUrlFromItemUuid(itemUuid); | ||
|
||
LOG.info("Completed DSpace Deposit for Submission: {}, accessUrl: {}", |
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 change to LOG.warn
so it shows up in default deployed logging level.
…ake the connection verification for DSpace more robust.
|
A new deposit option is added which uses the DSpace REST API.
Also:
DSpace deposit takes three calls and tries to handle resumption after an error in the last two calls.
Operational changes:
Required prs:
Related prs:
Test by building the pass-core, pass-ui, and pass-support prs. Then using the pass-docker pr, start up PASS with dspace support. Finally attempt to do some deposits and verify that they work. You should notice that publication date is now required.
Notes on interacting directly with the DSpace REST API:
Generate a CSRF token. Note that this endpoint returns 404.
Login and get the auth token
Create a workspace item
See workspace items
Set required metadata
Create a workflow item for workspace item
Example of patch