FCREPO-3753

[Geoffsc]

This PR resolves Jira ticket FCREPO-3753: adds a generic HTTP message service to the Camel Toolbox

JIRA Ticket: https://fedora-repository.atlassian.net/browse/FCREPO-3753

[demiankatz]

Note that this is very much a minimum viable product -- it simply forwards the JMS messages to HTTP as headers. We couldn't figure out how to reformat them into a message body and ultimately discovered that they work just fine as headers.

I have just completed a test integration with our application using the code in this branch, and it appears to be fully functional.

Note that there are several details discussed on the August 26 meeting which are NOT currently implemented here:

• Support for basic authentication
• Support for addition of arbitrary headers
• Support for routing to more than one HTTP endpoint
• Support for message filtering

The question is whether we can live without some or all of these; I would think that at very least, basic authentication and/or arbitrary headers would be important to support security needs.

Happy to discuss/expand as needed, but for now we'd like a review of what we've done so far before we invest more time into it, just to be sure we're not too far off base.

[demiankatz]

Another important detail is reindexing support: this has NOT been tested yet, and I suspect it probably won't work as expected. I'm not sure of the best way to test, but happy to do so when time permits.

[demiankatz]

Just a few comments that might be helpful...

[demiankatz]

I suspect that some of these dependencies aren't actually needed here; we based this pom.xml on the Solr component, and there's probably some junk that can be trimmed out.

[demiankatz]

What's the convention for @author tags? Is this okay, or should there be a separate line for each author?

[dbernstein]

Separate line I believe is preferable.

[demiankatz]

Thanks -- fixed!

[demiankatz]

Not sure what this is for; is it used?

[dbernstein]

This is for ignoring messages from resources that start with the specified paths.

[demiankatz]

@dbernstein, I believe this is now fully functional -- seems to be working as expected for both normal Fedora updates and the reindex case. Can you please give it another review and let us know if you see any rough edges that need polishing? We'll see about adding some test coverage tomorrow.

[dbernstein]

Looking good. As you mentioned, an integration test would be good. Another useful thing would be to update the ./README.md with the properties and add a blurb about what the HTTP service does.

[dbernstein]

Re filtering, you can just crib the code from the other services that support it.
Re basic auth, you can see an example in this PR: #167 namely something like this:

https://github.com/fcrepo-exts/fcrepo-camel-toolbox/pull/167/files#diff-d439075a8cdfdc12f02d6282463e2ec51fcc1a4a59ee3ebea207ec06be45fa62R130

[demiankatz]

@dbernstein, I believe that most of your review comments have now been addressed. Properties have been renamed, basic authentication support has been added, and filtering has been implemented (though I'm not sure how to test this feature). I have also started a test class, though coverage is far from complete at the moment. @Geoffsc is still working on README updates, but I expect those should be pushed up some time tomorrow.

[demiankatz]

@dbernstein, I'm about out of time, so let me explain where I've left off: I copied the integration tests from the triplestore component and attempted to replace Fuseki references with MockServer. I was hoping to get the point where I could at least see some failing assertions, but I think I'm still missing a few details to get all the parts to play together nicely.

Specific issues:

1.) Right now, the tests are using server.verify() to make assertions about calls to the MockServer HTTP endpoint. This does not appear to be working right. I'm not sure if I'm supposed to do this before or after the calls have occurred, or if I'm using entirely the wrong strategy. All I know is that if I comment out the server.verify() calls, the tests actually seem to get further and do more... I can actually see calls being made to the MockServer in the logs when I comment out the verify call, but when I include it, things fail earlier. I just left it in the commit for now to show my work so far.

2.) I don't think the test setup is creating events correctly (or else the code isn't handling the full range of possibilities, or some of the configuration is wrong). When messages get sent to the MockServer, they always seem to have a blank id, and they end up defaulting as Update events even when they're supposed to be Deletes. This suggests to me that for some reason or other, the expected headers are not in the expected places, but I'm not familiar with enough of the details to have a strong feeling of where to go first to fix it.

3.) I removed some await calls and other details that seemed to be TripleStore-specific... but maybe we need something similar/equivalent to make the timing work right. Again, I have a vague idea of how this works but don't understand enough details to grasp the whole thing just yet.

I hope I've at least gotten the ball rolling, but some assistance would be greatly appreciated. Happy to devote some time to this on Monday if there is time during/after our sprint meeting, if that's the best path forward.

[demiankatz]

I've improved the existing unit tests so that they cover more of the Camel routing (commit a800275) and have also added validation of the http.baseUrl property as discussed on today's call, with a corresponding test (c5b4d04). I wasn't able to figure out how to get the "missing baseUrl" tests to coexist with the "defined baseUrl" tests, so I ended up creating two separate (but very redundant) test classes. If there's a way to factor these together, I'd welcome input. All of this work is independent of the integration test work, which I'm hoping that @mikejritter can help with.

[demiankatz]

Also still to do: update the basic authentication work to reflect forthcoming shared logic.

[demiankatz]

Thanks to @mikejritter, I've made significant progress here, and the build should now be passing. However, the tests are not yet correct and need a bit more work; I'm hoping perhaps @dbernstein can help.

The issue is that the wrong event URI is getting passed to the HTTP listener, which I'm almost certain is happening because the test environment is not setting the expected org.fcrepo.jms.eventtype header, causing the Camel route to always default to "Update." Right now, both of the tests are expecting Update URIs (which is why they are passing), but this is incorrect -- they should be expecting Create and Delete respectively. My problem is that I don't entirely understand the tests, so I'm not sure how to fix this.

I'm also a bit unclear on why we are creating objects in a real Fedora instance but then manually triggering the Camel route; does that really serve any purpose? I think perhaps that was needed for the Triplestore test that I used as a model since the Triplestore route directly interacts with Fedora... but in these instances, it's probably sufficient to send a fake URI, since testing the HTTP service doesn't really require an active Fedora instance, unless we actually want to test the JMS portion of the integration, which does not appear to be happening here.

Hopefully we can discuss all this later today. Once the tests are sorted out, the only outstanding work that I am aware of is the basic auth refactoring.

[demiankatz]

@mikejritter and @dbernstein: I just made a major breakthrough; I figured out how to send the expected headers in the integration tests, so 373ea19 makes the tests actually test things realistically! As I suspected, the use of a real live Fedora is not necessary for these tests, so 9584d77 removes the Fedora interactions, since they have no bearing on what is being tested. I wonder if we can also completely remove Jetty from the test configuration... I don't have time to test it right now, but we can discuss when we talk later.

[demiankatz]

I've also made some more attempts at unifying RouteTest.java and RouteValidationTest.java into a single class, but I can't seem to figure out how to make it work. Perhaps there is a simple solution that I'm not finding.

[demiankatz]

More updates: based on discussion on the call, it sounds like two unit test classes is acceptable (and due to test framework limitations and timing, there may not be a better way to do it). I've also pushed up further simplification to the integration test environment to eliminate the unnecessary Jetty in pom.xml, so I think tests are fully done if @dbernstein sees no further fault with them.

[demiankatz]

@dbernstein, I believe this is now complete -- I saw that you merged #173, so I merged main into this branch and simplified the basic auth support. I also noticed that the checkstyle plugin was disabled here for some reason (probably copied and pasted from the Solr indexer, which also seems to have had it disabled at some point), so I enabled it and fixed all the issues.