METRON-2172 Solr Updates Not Tested in Integration Test

[nickwallen]

The SolrUpdateIntegrationTest is not testing that the SolrDao can update and retrieve values.

• This PR builds on METRON-2168 Elasticsearch Updates Not Tested in Integration Test #1451 . The diffs here will include the changes in METRON-2168 Elasticsearch Updates Not Tested in Integration Test #1451 until that PR is merged.

What?

This gap in the integration test is hiding a few existing bugs in the SolrUpdateIntegrationTest. This fix is similar to what was done for Elasticsearch in #1451.

Why?

These problems arise because of the way the test is setup. Instead of directly testing a SolrDao this test runs against a MultiIndexDao initialized with both a SolrDao and an HBaseDao. On retrievals the MultIndexDao will return the document from whichever index responds first.

With the current test setup, the underlying SolrDao will never retrieve the document that the test case is expecting. In all cases where the test passes, the document is actually being returned from the HBaseDao which is actually just interacting with a mock backend. The test needs to actually test that we can update and retrieve documents from Elasticsearch.

Bugs?

After correcting the test setup in SolrUpdateIntegrationTest the following bugs were found.

1. A timestamp is not being populated in the documents returned from Solr.
2. The comment field is not removed from the document when the last comment is removed.
3. A NullPointerException occurs if a document does not contain a sensor type.

Changes

1. The SolrUpdateIntegrationTest was changed so that it specifically tests a SolrDao.

2. The logic to serialize and deserialize a document to/from Solr was moved into a new abstraction called a DocumentBuilder. The Solr DAOs now all use a SolrDocumentBuilder to do this. Most of the existing logic from SolrUtilities was moved to SolrDocumentBuilder, except for portions that needed corrected.

   This allowed me to add unit tests for this logic in SolrDocumentBuilderTests. Specifically, I was able to add a test to ensure that the document's timestamp is being set correctly and that a missing sensor type does not cause a NullPointerException.

3. Duplicate logic to add, remove, and (de)serialize comments existed in UpdateIntegrationTest, SolrUpdateDao, SolrUtilities, HBaseUpdateDao, and ElasticsearchUpdateDao. This logic was also not covered by unit tests.

   The comment logic was moved to Document so that it could be reused. This made it so that no additional "reformatting" logic was needed for the comments. This also made it easy to add unit tests to test this functionality in DocumentTest.

4. After the previous change the logic for adding and removing comments is exactly the same for Solr, HBase, and Elasticsearch. So there is no need for custom addCommentToAlert and removeCommentFromAlert in HBaseUpdateDao, SolrUpdateDao, and ElasticsearchUpdateDao. The shared implementation for these was moved up to the UpdateDao interface as default methods.

Acceptance Testing

• Validate against Elasticsearch.

  1. Spin-up Full Dev with Elasticsearch running.
  2. Ensure alerts are visible in the Alerts UI.
  3. Add a comment to an alert.

• Validate against Solr.

  1. Spin-up Full Dev with Solr running.
  2. Ensure alerts are visible in the Alerts UI.
  3. Add a comment to an alert.

Pull Request Checklist

• Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
• Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
• Has your PR been rebased against the latest commit within the target branch (typically master)?
• Have you included steps to reproduce the behavior or problem that is being changed or addressed?
• Have you included steps or a guide to how the change may be verified and tested manually?
• Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
• Have you written or updated unit tests and or integration tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

[merrimanr]

These changes look good to me. Thank you for fixing this. +1 by inspection

[nickwallen]

I found a problem with this change. The Alerts UI does not properly display comments when indexing into Elasticsearch. See screenshot.

The problem is that we serialize comments into Elasticsearch differently than we do Solr. For Elasticsearch we write the comments as a list of maps. For Solr, we write a list of JSON strings. I do not understand why we serialize these differently, but I definitely don't want to change that behavior in this PR.

With this change, it wrote the comments as a list of JSON strings for both Solr and Elasticsearch. When running against Elasticsearch, the Alerts UI is then passed a raw JSON string that it does not know how to handle; hence the"Invalid Date" error in the Alerts UI.

To really fix this, I would need to refactor deeper into the Elasticsearch side of the DAOs and have them also use the DocumentBuilder abstraction. I would also need to add a test to catch this issue.

As mentioned in the PR description we (de)serialize comments in many different places and this should ultimately be cleaned up and more thoroughly tested. I am going to shelve this refactor for later. Until then, the minimal necessary fixes to test current behavior is in #1456 . Its less than ideal, but it gets the job done.