nickwallen opened a new pull request #1454: METRON-2172 Solr Updates Not Tested in Integration Test URL: https://github.com/apache/metron/pull/1454 The `SolrUpdateIntegrationTest` is not testing that the `SolrDao` can update and retrieve values. * This PR builds on #1451 . The diffs here will include the changes in #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. 1. The comment field is not removed from the document when the last comment is removed. 1. 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`. 1. 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. 1. 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`. 1. 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. ## Pull Request Checklist - [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel). - [x] 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. - [x] Has your PR been rebased against the latest commit within the target branch (typically master)? - [x] Have you included steps to reproduce the behavior or problem that is being changed or addressed? - [x] Have you included steps or a guide to how the change may be verified and tested manually? - [x] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via: - [x] Have you written or updated unit tests and or integration tests to verify your changes? - [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? - [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
