-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
DS-4257: DSpace 7 backend as one webapp (RESTv7, SWORD, SWORDv2, OAI, RDF) #2265
Conversation
98b4547
to
f97b0a5
Compare
Hi Tim, This is a great idea and I took a quick look at what is already there, see my comments below: I noticed that you deleted the "sword" folder from the "modules" directory. This "maven module" was used as for creating a webapp that could be deployed, so I understand why it was removed. But I think we might want to think about altering the purpose of it instead of straight out removing it entirely. I noticed that the dspace-sword pom.xml still contains the "maven-war-plugin", this can prob be left out (nitpicking now) |
Hi @KevinVdV : thanks for the review. A note on the If we were to create a "sword-additions" folder, this would similarly end up packaged as a separate JAR (from both sword.jar and additions.jar), which doesn't seem like it serves any real purpose. Why would we need multiple types of "additions" folders? If we need to customize SWORD behavior, why not just use the single "additions" folder, or use the Maven Overlay in the "spring-rest" WAR? I feel that, with this one webapp approach, we logically will be forced into only supporting UPDATE: As a sidenote, conceptually, I agree that overlaying SWORD in something called "spring-rest" sounds odd. But, keep in mind that, as this one webpap project moves forward, I feel we will need to rename spring-rest. It no longer will be only a REST webapp, it'll be the entire DSpace backend. |
If there would be a need to make a customization of e.g. a SWORD class which is not implemented as a service, this is supported in the sword module in e.g. DSpace 6. Any changes in this module would end up in the Using a single webapp approach, this would only fit in a module which is dependent on the standard DSpace SWORD webapp. So the additions module would not be a good fit. Such customizations would also not need to end up in I think your sidenote is very relevant here. Would you both agree this solves the issues in a nice and clean way? |
@benbosman and @KevinVdV : Agreed, during this whole process I've always felt this PR would require us to rename the "spring-rest" webapp. I've updated the description of this PR to make that clear (by adding a checkbox at the end that we'll need to rename it). If I'm understanding @benbosman's comments correctly, it sounds like you are saying I need not change anything in the current strategy for how I'm merging these webapps in this PR? For what it's worth, I think there are three options that will still be available (and developers can choose whichever one they feel best aligns with their needs). For the descriptions below, I'm calling the "one-webapp" the "Backend Webapp":
So, I think we'll still have plenty of options for developers to apply custom code. The only one that we lose is the ability for per webapp overlays -- as we no longer have multiple webapps, we only have one. |
@tdonohue that sounds good, and |
6971655
to
748b14c
Compare
I've removed the "work-in-progress" label from this PR, as all (non-deprecated) webapps are now running in Spring Boot (with ITs to prove they work). This includes REST API v7, OAI-PMH, SWORDv1, SWORDv2 and RDF. Reviewers are welcome. However, before merging, I think we should consider renaming the |
[tangent] Regarding local overrides: we are going to have to re-think the Additions mechanism anyway. It is dependent on Tomcat behavior which was never part of the Servlet specification, and which was removed during Tomcat 8 development. Tomcat 8+ will search the JARs in lib/ in whatever order readdir() returns them, which depends on the filesystem implementation and the history of mutations of the directory. So our current method might or might not work on Day 1 in any particular instance, and might stop (or resume) working at any time. We need to find some way either to order the classpath ourselves, or to make actual replacements in dspace-*.jar from the custom classes at assembly time. There is probably a better place to discuss this. |
@mwoodiupui : Point taken. I suspect longer term questions about Overlays/Additions should likely move to the JIRA ticket on Tomcat 8+ support: https://jira.duraspace.org/browse/DS-3092 As a more general note, I'd rather not muddy up this PR discussion any further regarding Overlays or the "Additions" module. I only wanted to note that this PR (obviously) must remove the Maven WAR Overlay concept from any merged webapps...as Overlays are only applicable to WARs (and all merged webapps, like OAI, SWORDv1/v2, RDF, become JAR modules now). Longer term discussions of Overlays/Additions should move elsewhere, as this PR is not attempting to solve that problem. |
In order to simplify the review process of this PR, I've decided to move the module renaming (and corresponding configuration changes) to a separate, follow-up PR. As I began that process, I realized it involves touching a large number of files -- and I don't want to make this PR impossible to review. So, this PR should be considered complete and ready to review. The final result of this PR is that it just ensures all old Webapps (except dspace-rest and dspace-solr) run from a single (Spring Boot) webapp in the existing I will be submitting a separate, follow-up PR to rename the |
I am testing this change in Docker. The spring-rest service came up nicely. Angular was able to connect. Here are some preliminary observations. I will post more as I continue my testing and review. I am deploying this app to /spring-rest. I initially presumed that setting oai.path or rdf.path would make those services available at /oai or /rdf. Looking at the code, I see that these paths will still fall below the one webapp. That might be useful to clarify in the documentation. I set I am attempting to look at config properties. I am seeing logging library error when I attempt to run the CLI command. The command does succeed.
I started up the RDF service. I am able to run rdfizer, but I am unable to access the rdf web service at /spring-rest/rdf. PerformanceWith the one webapp, I saw the initialization time drop by 30% - 50%! One Webapp
Without One Webapp
Error PageA user-friendly error page needs to be displayed when a URL is invalid. Here is the text that currently displays.
|
@terrywbrady : Thanks for the quick test/review. A few responses to your comments: RE: deployment question. you are correct, all the RE: OAI configuration issues The OAI-PMH links are all driven from the existing The configuration issues are definitely a pain at this point. But, I will clean them all up in the follow-up PR. I'm worried about lumping them into this PR, as it will require touching a large number of files, as I'll be removing the existing RE: Performance Glad to hear the performance is better!! RE: Error Page This is a general issue for the |
@terrywbrady : On second thought, I think I might do some basic "sane" updates to existing configurations, to ensure testing this PR is a bit easier. So, while the notes on in the PR description will provide info on what to add to your |
Based on @terrywbrady's feedback/fixes, I've just fixed a minor bug in the OAI-PMH module. It was not correctly determining which OAI context you were using (which broke some links in the browsable UI). I've also defaulted
|
ca23373
to
6da8225
Compare
This PR was just rebased on the latest master after the Solr upgrade was merged (#2058). So, testing this PR will now require installing an external Solr (like latest |
6da8225
to
0b89a35
Compare
I created a docker image to facilitate testing of this PR.
Note that we do not yet have a mechanism to auto-build images for a specific PR, so this image would need to be rebuilt manually if the image changes. @tdonohue , I confirmed that the OAI issues I had reported are resolved. |
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.
@tdonohue, The Dockerfile creates symlinks for the DSpace webapps. The list should be pruned.
Options
- Only provide /spring-rest
- Provide both /spring-rest and /rest
The override for the solr service is no longer needed. Depending on the answer above, the override for /rest may not be needed. Based on that decision, we may be able to consolidate to only one Dockerfile.
Additional Notes. I have attempted to test services.
While I can access the OAI service, the links within that service are not valid. I am unclear if the code is wrong or if I need to pass a different variable.- I can rdfize the content, but I cannot access the rdf service.
- I can deposit with sword v2
but notand v1
@terrywbrady : I think I'm going to need help in correcting any Docker related issues here. I definitely want to ensure this is working properly with Docker. But, as you saw, I made no Docker related changes in this PR as I'm not familiar enough with where the changes should be made. It also sounds like some of the problems you noted might be related to configuration in the Docker setup. (e.g. SWORDv1 does a lot of URL validation and actively blocks deposits if they are POSTed to a URL that doesn't exactly match the known SWORD URL . So, it's possible that SWORDv1 is blocking your requests if the URL configurations are not as expected.) Is there a Docker setup you are using to perform these tests? Do you have a Docker setup working on |
…atic files are loaded properly
…t. Disable existing ITs (will be removed)
… to Spring Boot in previous commit.
…dd IT to prove it works
…aseUrl to single webapp path.
…cessary dependencies
…F. Add note about upgrading Jena for future.
03e5c65
to
4d35e0f
Compare
@tdonohue , the latest changes resolved the issue that I was seeing with the RDF service. Thanks for resolving the issue! |
As noted in last week's DSpace 7 Meeting, this can now be merged. It's at +2, and we wanted to merge this immediately after Preview Release (which came out yesterday). |
https://jira.duraspace.org/browse/DS-4257
This is a replacement PR for #2231.
The goal of this PR is to revisit ways to simplify the DSpace 7 install/deployment process by turning the backend into a single webapp. See also early notes at: https://wiki.duraspace.org/display/DSPACE/DSpace+Backend+as+One+Webapp
What this PR currently achieves (will keep this list up-to-date)
application.properties
. Fixed in commit 939d4b8 which refactors Application startup to load our DSpace Configs early in Spring Boot startup processsword-server.enabled
configurationsword-server.path
configuration)sword-server.cfg
. This URL configuration issue has been noted as a TODO below.sword-server.servicedocument.url = ${dspace.restUrl}/${sword-server.path}/servicedocument
in yourlocal.cfg
swordv2-server.enabled
configurationswordv2-server.path
configuration)swordv2-server.servicedocument.url = ${dspace.restUrl}/${swordv2-server.path}/servicedocument
in yourlocal.cfg
oai.enabled
configurationoai.path
configuration)oai.url = ${dspace.restUrl}/${oai.path}
in yourlocal.cfg
rdf.enabled
configurationrdf.path
configuration)rdf.contextPath = ${dspace.restUrl}/${rdf.path}
in yourlocal.cfg
FINAL RESULT: The final result of this PR is that it just ensures all old Webapps (except dspace-rest and dspace-solr) run from a single (Spring Boot) webapp in the existing
dspace-spring-rest
module. While this module is not the most appropriately named (yet), merging this PR as-is should not have major conflicts with other development efforts within the currentdspace-spring-rest
module.A FOLLOW-UP PR (coming soon) will be submitted that includes the following. This work will be submitted as a follow-up PR since it has a higher likelihood of causing conflicts in ongoing development efforts:
dspace.cfg
will be standardized across all modules. Primarily this will involve creating a newdspace.server.url
configuration to replacedspace.url
,dspace.restUrl
anddspace.baseUrl