Composite catalog implementation

[Jay Rainey]

Implementation

There needs to exist support for multiple catalogs within Mantid, which can be achieved via a composite pattern.

1. A new class (CompositeCatalog) needs to be created.
2. This class will hold a list of ICatalog instances (shared pointers) named m_catalogs.
3. Catalogs can be added to m_catalogs using an add() method.
4. The class will inherit from ICatalog and override all the parents methods.
5. Each method will iterate over m_catalogs and call the exact method from the catalog.

Unit test

A test class needs to be created for this composite class to verify the functionality works as expected.

1. Create a test class (CompositeCatalogTest).
2. Create a DummyCatalog class within the test class that extends ICatalog and then overrides all methods (to verify that each work).
3. Have a static counter in each method, in order to verify when they are called.
4. Create a test method for each ICatalog method.
5. Create several (3-5) DummyCatalogs in each method, and call the related ICatalog method, verifying that the counter increased as expected.

[Jay Rainey]

Add header for CompositeCatalog. Refs #9026.

Changeset: f375e3b15bf305701d07619684646e0cb5b49d3d

[Jay Rainey]

Added initial empty compositeCatalog implementation. Refs #9026.

Changeset: 5da146b297300d9ea15a8086e0335822aaa864b0

[Jay Rainey]

Added files to makelist. Refs #9026.

• Updated include to use ICatalog instead of algorithm.

Changeset: 57c8205dcc729ed52dde900d755822a30467f8a4

[Jay Rainey]

Added implementation for each method. Refs #9026.

Changeset: e06c13d0a6fc7b9e984d6951b539d85f604f71c0

[Jay Rainey]

Updated documentation. Refs #9026.

Changeset: a24c1fd76fb85af13157e0394cefc320c1775d27

[Jay Rainey]

Initial compositeCatalog test. Refs #9026.

Changeset: 1108ea09cf95f05833ec7c6691ed33073b54f313

[Jay Rainey]

Added unit tests for compositeCatalog. Refs #9026.

• Still need to write a test for search and getNumResults, but having issues with CatalogSearchParam.

Changeset: e6f0f77442b2e696cbdc77f8f593dcb28b4df36b

[Jay Rainey]

Remove invalid test method. Refs #9026.

Changeset: 21d48bf84b448183901c814887eff76a307cdc08

[Jay Rainey]

Pass parameters by reference. Refs #9026.

Changeset: e6af91624cd63ddc66e163d5427478d84dfd564d

[Jay Rainey]

Added additional tests. Refs #9026.

• Removed forward declaration from catalog interface to allow creation and assignment of CatalogSearchParam.
• Had to add DLLExport to CatalogSearchParam to prevent linker issues.
• Removed CatalogSearchParam include from catalogs as it is now in the inherited interface.

Changeset: 0a6469dea8bc44340da5cd01620ffb4481c589a7

[Jay Rainey]

Fix unix issues. Refs #9026.

• Added missing DLL include.
• Revert to forward declaration of CatalogSearchParam.

Changeset: d5b8673e5639b4c76e1ec3dd32b357de1f7d8851

[Jay Rainey]

Refactor compositeCatalogTest. Refs #9026.

Changeset: 8982762efba956d586aee833ad3a4de681cd9a41

[Jay Rainey]

The changes in this ticket allow for multiple catalogs to be stored, and their methods to be called in succession. That is, when the compositeCatalog performs a search, it will invoke a search in each catalog that the composite knows of.

To test

1. Perform a code review to verify the ​changes address the problem in the ​description.
2. Verify the unit test introduced is sensible and passes on the build servers.

[Peter Parker]

We discussed some issues with the current implementation:

1. The current implementation of some methods (for example getNumberOfSearchResults, or the ones that return strings) don't make sense in the context of the Composite pattern.

2. The unit tests ensure that the member functions of the "child leaves" in the Composite pattern are called, but not a lot else. The functionality of 1., once corrected, needs to be covered as well.

[Jay Rainey]

Increase the number of search results. Refs #9026.

Changeset: ec1042203be028cfb055724435750effa88abf43

[Jay Rainey]

Remove implementation from methods. Refs #9026.

• Update add to be const correct.

Changeset: b6a48e144079853b59c9127767ba234fd10a3df1

[Jay Rainey]

Disable related tests. Refs #9026.

Changeset: 5e098042fac6c09c09ca30970e6151a13460554a

[Jay Rainey]

Fix compiler warnings. Refs #9026.

Changeset: 8d21b9266e1df13577f4c603ee25ce7b35c24cbd

[Jay Rainey]

Merge remote-tracking branch 'origin/master' into feature/9026_composite_catalog. Refs #9026.

Conflicts:

• Code/Mantid/Framework/API/inc/MantidAPI/ICatalog.h
• Code/Mantid/Framework/ICat/inc/MantidICat/ICat3/ICat3Catalog.h

Changeset: 916ae6433ed2282a1a3356e817f785cde7a5ec1b

[Jay Rainey]

Remove unused IDS methods. Refs #9026.

Changeset: 94cca81caa80d1493e68bef3a0de3f8aeaa796fb

[Jay Rainey]

Accidently removed wrong method. Refs #9026.

Changeset: 5452d4295b5f98c4a090a533a906216603d545fc

[Jay Rainey]

Remove keepAliveInMinutes from ICatalog. Refs #9026.

• This method is invalid in the compositeCatalog. An alternative keepAlive method exists, which should be used instead.

Changeset: 59e0177faf71e2db2b05c43c294aa77da5161105

[Jay Rainey]

Improve compsiteCatalog tests. Refs #9026.

• I now check the functionality of each test in relation to how they are implemented in the catalog. That is, each method should append data to a workspace or vector.

Changeset: b3270cdcc0b4f98bbe379b993055b04b532a592c

[Jay Rainey]

Move getFileLocation to the IDS Interface. Refs #9026.

Changeset: 9d2b1d1e2a560d7912257d9f0b9e451a024ad3f1

[Jay Rainey]

Update methods to return strings. Refs #9026.

Changeset: f74cf6f8055fa9641e33054de4943a864bb549b0

[Jay Rainey]

Fix doxygen warnings. Refs #9026.

Changeset: 119503d31744e5c39bb1dc52f15576bda55c47b7

[Jay Rainey]

The changes in this ticket allow for multiple catalogs to be stored, and their methods to be called in succession. That is, when the compositeCatalog performs a search, it will invoke a search in each catalog that the composite knows of.

Having taking Peter's feedback in comment:19 into account I have:

1. Improved compositeCatalogTest to also test that the functionality of each ICatalog interface works, e.g. it appends data rather than creating new.
2. I removed keepAliveInMinutes from ICatalog as it's not used, and not valid for a compositeCatalog.
3. I moved getFileLocation to the IDS interface.

To test

1. Perform a code review to verify the ​changes address the problem in the ​description.
2. Verify the unit test introduced is sensible and passes on the build servers.
3. Verify the changes I made above are sensible and address the issues noted by Peter in comment:19

[Peter Parker]

Tests pass and the points were addressed, but I've seen a couple more minor things:

1. We don't need to provide a destructor in CompositeCatalog (either in the .h or the .cpp) since the default compiler-generated destructor does everything we need in this case. To see what I mean, paste the following into ​Coliru and run it:

   #include <iostream>
   #include <vector>
   #include <memory>
   
   class Object
   {
   public:
       ~Object(){ std::cout << "Object destructor called." << std::endl; }
   };
   
   class Interface
   {
       virtual ~Interface() {}
       
       virtual void functionality() = 0;
   };
   
   class Implementation
   {
   public:
       Implementation() : m_objects()
       {
           auto object = std::make_shared<Object>();
           m_objects.push_back(object);
       }
       
       virtual void functionality() { /* ... */ }
   
   private:
       std::vector<std::shared_ptr<Object>> m_objects;
   };
    
   int main() 
   {
       Implementation test;
   }
   

You should get an output of "Object destructor called.", even though the instance of Object is wrapped in a shared pointer and a vector, and we don't define a destructor for the Implementation class.

2. Do we have plans to provide any implementations of keepAlive()? If not, we should remove it from ICatalog.

[Jay Rainey]

Remove composite destructor. Refs #9026.

Changeset: 8c84499f14ebea179c797da20b4e18c5cc0d5734

[Jay Rainey]

Replying to Peter Parker:

We don't need to provide a destructor in CompositeCatalog.

That's true, thanks! I have removed the destructor in the above ​commit.

Do we have plans to provide any implementations of keepAlive()? If not, we should remove it from ICatalog.

Yes, we plan to implement keepAlive() in a separate ticket - #9089.

[Peter Parker]

Merge remote-tracking branch 'origin/feature/9026_composite_catalog'

Full changeset: ​a3b15492885f38d32ad666ee507d956b0e16b14d

[Peter Parker]

Sorry for holding this up. Looks good now.

[Stuart Campbell]

This ticket has been transferred to github issue ​9869