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.

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

Issue-9435 #9436

Merged
merged 3 commits into from May 3, 2024
Merged

Issue-9435 #9436

merged 3 commits into from May 3, 2024

Conversation

Xiqinger
Copy link
Contributor

References

Description

  • add path check before mkdirs.
  • add collection empty check before call 'iterator().next()'
  • move 'outputStream.close()' from callee method 'disseminate' to caller, avoid write after close in for loop.

Instructions for Reviewers

  • check path before mkdirs

    • Move the path validation statement from line 1968 to before line 1960.
  • Add empty check before call iterator().next()
    Before call "Item item = bundles.iterator().next().getItems().iterator().next();",

    • check "bundles" is not empty.
    • Then, check "bundles.iterator().next().getItems()" is not empty.
  • avoid outputStream write after close
    Because "generateBodyMail" has a for loop, it calls "out.close()" in callee "disseminate", then may call "out.write()".

    • remove "out.close()" in callee method "disseminate".
    • add "out.close()" in caller method "makeMdSec" and "generateBodyMail".

@tdonohue tdonohue added bug 1 APPROVAL pull request only requires a single approval to merge. port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels Mar 26, 2024
@tdonohue
Copy link
Member

tdonohue commented May 2, 2024

For some reason the automated GitHub checks have failed to run on this PR. I'm closing it temporarily & reopening to see if that will trigger the checks to run.

@tdonohue tdonohue closed this May 2, 2024
@tdonohue tdonohue reopened this May 2, 2024
@tdonohue tdonohue self-requested a review May 2, 2024 20:07
@tdonohue tdonohue added this to the 8.0 milestone May 2, 2024
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @Xiqinger ! My apologies for taking so long in reviewing this PR. Today I finally had time to do a detailed review of the suggested changes. I agree that these are all improvements to the current code. I also ran a few basic tests of the modified code and found no changes in behavior.

@tdonohue tdonohue merged commit 5b8f491 into DSpace:main May 3, 2024
23 checks passed
@dspace-bot
Copy link

Successfully created backport PR for dspace-7_x:

@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge. bug
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Report some code that may cause security issues.
3 participants