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.

Sign up for GitHub

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

Jump to bottom

Fixed ServletException #4548 #4691

Merged
merged 2 commits into from
May 18, 2018
Merged

Fixed ServletException #4548 #4691

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51611d2
Fixed ServletException #4548
May 18, 2018
36837f6
add javadoc to clarify why the fix works #4548
pdurbin May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 21 additions & 19 deletions src/main/java/edu/harvard/iq/dataverse/harvest/server/xoai/Xrecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,27 +75,29 @@ public void writeToStream(OutputStream outputStream) throws IOException {

outputStream.write(headerString.getBytes());

if (!isExtendedDataverseMetadataMode(formatName)) {
outputStream.write(METADATA_START_ELEMENT.getBytes());

outputStream.flush();

if (dataset != null && formatName != null) {
InputStream inputStream = null;
try {
inputStream = ExportService.getInstance().getExport(dataset, formatName);
} catch (ExportException ex) {
inputStream = null;
}

if (inputStream == null) {
throw new IOException("Xrecord: failed to open metadata stream.");
if (header.getStatus() == null) { // Deleted records should not show metadata
if (!isExtendedDataverseMetadataMode(formatName)) {
outputStream.write(METADATA_START_ELEMENT.getBytes());

outputStream.flush();

if (dataset != null && formatName != null) {
InputStream inputStream = null;
try {
inputStream = ExportService.getInstance().getExport(dataset, formatName);
} catch (ExportException ex) {
inputStream = null;
}

if (inputStream == null) {
throw new IOException("Xrecord: failed to open metadata stream.");
}
writeMetadataStream(inputStream, outputStream);
}
writeMetadataStream(inputStream, outputStream);
outputStream.write(METADATA_END_ELEMENT.getBytes());
} else {
outputStream.write(customMetadataExtensionRef(this.dataset.getGlobalId()).getBytes());
}
outputStream.write(METADATA_END_ELEMENT.getBytes());
} else {
outputStream.write(customMetadataExtensionRef(this.dataset.getGlobalId()).getBytes());
}
outputStream.flush();

Copy link
Member

Choose a reason for hiding this comment

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

@juancorr thanks again for the pull request.

At standup this morning I offered to take a look but I just requested a review from @landreev because this is all code he wrote. I loaded up the code in Netbeans and here's a screenshot with some arrows pointing to specific lines that I'm wondering about:

screen shot 2018-05-18 at 12 23 02 pm

At line 73 I'm wondering if we should put the newly added header.getStatus() check in this part of the code since it's where we're throwing an IOException already. Maybe all the error checking should go here, I mean.

At line 101 I'm wondering if we should add an else for the new header.getStatus() check. What do we want to happen? Throw an IOException?

At line 113 I noticed that we're already checking header.getStatus() (the itemHeaderToString method gets called at line 70). It seems like there's potential to only have one check.

Oh, I didn't add an arrow for this but in line 87 I noticed that we're using a deprecated getInstance method. I left a comment about this at #4683 (comment) because it feels a little more on topic there but while we're in this part of the code I'm wondering if we should fix #4683. Here's the screenshot I added there:

screen shot 2018-05-18 at 12 15 46 pm

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pdurbin and @landreev, I think that I can response to your three arrows:

Copy link
Member

Choose a reason for hiding this comment

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

@juancorr thanks! @landreev explained some of this to me on Friday and the pull request was already merged then but it's always good to share knowledge about how all this works!

Expand Down