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
Refactor/remove custom MultipartFileSender to use Spring Boot Range requests #3009
Refactor/remove custom MultipartFileSender to use Spring Boot Range requests #3009
Conversation
4f5040f
to
867aec4
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.
@jonas-atmire : I gave this a code review & some basic testing today. First off, the code looks good.. there's just some missing JavaDocs & a few inline questions below that require cleanup.
I also tested this from commandline (using the same commands that I reported previously didn't work in this comment)... and I've found this seems to work great! I'm now seeing the headers that are automatically added by Spring Security like Access-Control-Allow-Origin
and Access-Control-Expose-Headers
. Previously, those were missing.
I've not yet had a chance to test it with the corresponding Angular UI PR (DSpace/dspace-angular#827) simply because that PR is outdated & needs a rebase to make it easier to test.
Overall this looks/works great. So, I'm only requesting JavaDocs additions / minor cleanup here. Once the Angular PR is ready again, I'll do a test of both together & then I expect to approve both. Thanks!
dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/BitstreamResource.java
Show resolved
Hide resolved
dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java
Outdated
Show resolved
Hide resolved
dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java
Show resolved
Hide resolved
dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java
Outdated
Show resolved
Hide resolved
dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/HttpHeadersInitializer.java
Show resolved
Hide resolved
@jonas-atmire Thank you for your hard work on this PR and the changes that were made to this PR. It looks good to me as long as @tdonohue requested changes have been made. |
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.
👍 @jonas-atmire and @Raf-atmire : Code looks great now. I also tested this today with the frontend PR (DSpace/dspace-angular#827) and it all works perfectly. Thanks again!
References
Description
The custom MultipartFileSender was completely bypassing Spring Security & Spring automated tools.
This PR makes sure that we lean more towards the 'default' usages of the previously mentioned Spring Boot etc.
Instructions for Reviewers
Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.
List of changes in this PR:
Testing steps using REST
Testing steps using Angular
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
pom.xml
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.