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

Reworking fcrepo indexer to be file system aware #51

Merged
merged 5 commits into from
Aug 15, 2018

Conversation

dannylamb
Copy link
Contributor

GitHub Issue: Part of Islandora/documentation#769

What does this Pull Request do?

Updates islandora-indexing-fcrepo to handle the new message format, and deposits derivatives in the filesystem selected through the Drupal UI. Don't mind all the POJO boilerplate.

How should this be tested?

Test as part of Islandora/documentation#92

Additional Notes:

Needs tests, just putting this out there before wrapping it all up.

Interested parties

@Islandora-CLAW/committers

@codecov
Copy link

codecov bot commented Aug 2, 2018

Codecov Report

Merging #51 into master will increase coverage by 0.88%.
The diff coverage is 85.49%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #51      +/-   ##
============================================
+ Coverage     79.47%   80.36%   +0.88%     
- Complexity       56       93      +37     
============================================
  Files            11       17       +6     
  Lines           229      331     +102     
  Branches          1        1              
============================================
+ Hits            182      266      +84     
- Misses           46       64      +18     
  Partials          1        1
Impacted Files Coverage Δ Complexity Δ
...ca/indexing/fcrepo/event/AS2AttachmentContent.java 100% <100%> (ø) 5 <5> (?)
...lpaca/indexing/triplestore/TriplestoreIndexer.java 100% <100%> (ø) 6 <0> (ø) ⬇️
.../connector/houdini/event/AS2AttachmentContent.java 100% <100%> (ø) 11 <2> (ø) ⬇️
...ora/alpaca/connector/houdini/HoudiniConnector.java 100% <100%> (ø) 3 <0> (ø) ⬇️
...landora/alpaca/indexing/fcrepo/event/AS2Actor.java 70% <70%> (ø) 4 <4> (?)
...andora/alpaca/indexing/fcrepo/event/AS2Object.java 70% <70%> (ø) 5 <5> (?)
...islandora/alpaca/indexing/fcrepo/event/AS2Url.java 75% <75%> (ø) 7 <7> (?)
...landora/alpaca/indexing/fcrepo/event/AS2Event.java 78.94% <78.94%> (ø) 9 <9> (?)
...ra/alpaca/indexing/fcrepo/event/AS2Attachment.java 80% <80%> (ø) 5 <5> (?)
...slandora/alpaca/indexing/fcrepo/FcrepoIndexer.java 90.24% <94.44%> (+2%) 6 <3> (+2) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8daf703...9a5d206. Read the comment docs.

Copy link

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Tiny typo: there are some "name":"Canoncial", around in test json AS2 files that need to be "Canonical" I marked some of them but probably easier to fix via grep and replace

@@ -14,26 +14,28 @@
]
},
"object":{
"id":"urn:uuid:3d3550ff-1d4c-4484-a406-8f97c1673e5c",
"id":"urn:uuid:72358916-51e9-4712-b756-4b0404c91b1d",
"url":[
{
"name":"Canoncial",

Choose a reason for hiding this comment

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

This should be "Canonical" instead of Canocial

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @DiegoPino. Thanks man. I'll update that shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

cool!

"id":"urn:uuid:4f528a92-4be2-4a9d-85e6-c5f4d36b1bce",
"url":[
{
"name":"Canoncial",

Choose a reason for hiding this comment

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

and again, should be "Canonical",

"id":"urn:uuid:148dfe8f-9711-4263-97e7-3ef3fb15864f",
"url":[
{
"name":"Canoncial",

Choose a reason for hiding this comment

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

Same as before, "Canonical", probably replicated from copy/paste

.removeHeaders("*", "Authorization")
.setHeader(Exchange.HTTP_METHOD, constant("POST"))
.setHeader("Content-Location", simple("${exchangeProperty.jsonUrl}"))
.transform(simple("${null}"))
Copy link
Member

Choose a reason for hiding this comment

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

Could this just be .setBody(simple("${null}")) instead of a transform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure what the difference is, but "setBody" seems clearer to me. I'll have to try it out to know for sure.

Copy link
Member

Choose a reason for hiding this comment

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

I think transform() is part of the message translator EIP, so if you are just wanting to make the body of the message empty I think .setBody() would be more efficient.

But I don't know for sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whelp, I did it and tested it and nothing seems to have changed, so I'm pushing it up. It just reads better IMO.

@whikloj
Copy link
Member

whikloj commented Aug 15, 2018

@DiegoPino can I merge this, I think your requests have been addressed.

@whikloj whikloj merged commit fd421da into Islandora:master Aug 15, 2018
@DiegoPino
Copy link

@whikloj i did not get to test all the code, but my concerns are addressed, feel free to merge, you know the internals way better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants