Fixed ServletException #4548 #4691
Conversation
| } | ||
| outputStream.write(METADATA_END_ELEMENT.getBytes()); | ||
| } else { | ||
| outputStream.write(customMetadataExtensionRef(this.dataset.getGlobalId()).getBytes()); | ||
| } | ||
| outputStream.flush(); | ||
|
|
There was a problem hiding this comment.
@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:
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:
There was a problem hiding this comment.
Hi @pdurbin and @landreev, I think that I can response to your three arrows:
- Line 73: In OAI protocol, when a record (dataset) has been retired (deaccessioned) should be shown as deleted without metadata but with header information (you can see first record in https://edatos.consorciomadrono.es/oai?verb=ListRecords&resumptionToken=MToyMDB8MjpvcGVuYWlyZV9kYXRhfDM6fDQ6fDU6b2FpX2RhdGFjaXRl ) . An exception at this point would hide the header information.
- Line 101: I think that an else is not needed because header information is written out in line 76.
- Line 113: Here is written out the "status=deleted" attribute needed for the OAI protocol
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist
Update by @pdurbin 2018-05-18
connects to #4548
I'm adding the "connects to" line above to associate the pull request with the issue. For more on this, please see http://guides.dataverse.org/en/4.8.6/developers/version-control.html#make-a-pull-request