Ticket #9084 (closed: fixed)
Create a catalog manager
Reported by: | Jay Rainey | Owned by: | Jay Rainey |
---|---|---|---|
Priority: | major | Milestone: | Release 3.2 |
Component: | Framework | Keywords: | ICAT |
Cc: | martyn.gigg@… | Blocked By: | #9026 |
Blocking: | #7640, #9089, #9112 | Tester: | Peter Parker |
Description (last modified by Jay Rainey) (diff)
Currently, there is no way to obtain a specific catalog if many existed. Functionality needs to be implemented to address this, and can be achieved by:
- Remove create() from CatalogFactory as it currently creates only one catalog.
- Remove Session.h and add related member variables to each catalog (m_sessionId and m_facility). They should be set in the login method of each catalog. This will require added an additional parameter to ICatalog` for the facility name.
- Create a new singleton class (CatalogManager) that can managers active catalogs.
CatalogManager is the core change in this ticket, and will be addressed as follows:
- CatalogManager will hold a list of active catalogs and their sessionId in a member variable (m_activeCatalogs).
- Create Two methods (getActiveCatalogs() and getActiveCatalog(sessionId) in CatalogManger. One to obtain all active catalogs, and the other to obtain a specific catalog.
- A create method to add the catalog to the compositeCatalog and returns that catalog.
- A destroy method to remove the catalog from m_activeCatalogs in CatalogManager, and also from the compositeCatalog.
Here is a the class diagram to make this a bit clearer.
Attachments
Change History
comment:10 Changed 7 years ago by Jay Rainey
comment:11 Changed 7 years ago by Jay Rainey
comment:12 Changed 7 years ago by Jay Rainey
comment:13 Changed 7 years ago by Jay Rainey
comment:14 Changed 7 years ago by Jay Rainey
comment:15 Changed 7 years ago by Jay Rainey
comment:16 Changed 7 years ago by Jay Rainey
comment:17 Changed 7 years ago by Jay Rainey
comment:18 Changed 7 years ago by Jay Rainey
comment:19 Changed 7 years ago by Jay Rainey
comment:20 Changed 7 years ago by Jay Rainey
comment:21 Changed 7 years ago by Jay Rainey
comment:22 Changed 7 years ago by Jay Rainey
comment:23 Changed 7 years ago by Jay Rainey
comment:24 Changed 7 years ago by Jay Rainey
comment:25 Changed 7 years ago by Jay Rainey
comment:26 Changed 7 years ago by Jay Rainey
Added new catalog session class to API. Refs #9084.
Changeset: b4b6313304c3c03fd8566cfd7534b3f25ead7463
comment:27 Changed 7 years ago by Jay Rainey
Add setSession to CatalogSession. Refs #9084.
Changeset: 9c33488935ca4d87909422b3027c6d7b379cc978
comment:28 Changed 7 years ago by Jay Rainey
Add facility parameter to catalog login. Refs #9084.
Changeset: bdfe1d5c16945a9bf0bec58b4ded2da2b1ce7868
comment:29 Changed 7 years ago by Jay Rainey
Pass parameters by reference in CatalogManager. Refs #9084.
- I have to create and pass in an empty string to getCatalog (instead of "") otherwise linker errors occur.
Changeset: 2c3772edd220e899772654e8822de5b1f3adceb6
comment:30 Changed 7 years ago by Jay Rainey
Update login method to return a session. Refs #9084.
- Throw exception in the compositeCatalog for the login method as it no longer performs the correct composite functionality. (It could not return all the sessions for each catalog the user logs in to, just the last.)
Changeset: 3a2216adbb9f3a3eacf96d3a4db1c411e3f47583
comment:31 Changed 7 years ago by Jay Rainey
Use CatalogSession in ICat4Catalog. Refs #9084.
Changeset: cb60dc4718c70b655793f029f67986f4384742bc
comment:32 Changed 7 years ago by Jay Rainey
Removed unnecessary sessionID in catalog algorithms. Refs #9084.
Changeset: 830277c12afcecd268e31862f90fd49139285c1a
comment:33 Changed 7 years ago by Jay Rainey
Updated ICat3Catalog to use new session class. Refs #9084.
- Made login method return the new session.
- Removed destructor from ICat3Helper.
- Removed unnecessary method setReqParam from ICat3Helper.
- Removed invalid isvailidSession (it never makes a reques to verify the session)
- Updated listInstruments and `listInvestigionTypes to append to the list rather than reassigning it entirely.
Changeset: cf3396bea3779b3cb033723de52852b69e41c186
comment:34 Changed 7 years ago by Jay Rainey
Removed old singleton session. Refs #9084.
- Removed references to session from unit-tests. Th
- Updated CompositeCatalogTest to throw an runtime on login method.
Changeset: aa1545f41d87e77de69f181bf62de49c76013abe
comment:35 Changed 7 years ago by Jay Rainey
Merge branch 'feature/9084_create_catalog_manager' into develop. Refs #9084.
Conflicts:
- Code/Mantid/Framework/API/inc/MantidAPI/CatalogSession.h
- Code/Mantid/Framework/API/src/CatalogSession.cpp
Changeset: 741b28ef131bf1bf6fcc4d1c6ef5511490f7f6dd
comment:36 Changed 7 years ago by Jay Rainey
Fix build issue on OSX. Refs #9084.
Changeset: b9763b2e42a676b9632151a41e9f2fe4bd1d9d49
comment:37 Changed 7 years ago by Jay Rainey
Attempt to fix OSX build. Refs #9084.
Changeset: 5e23bb4255eaf87bbdb827633710dbc92e8e6002
comment:38 Changed 7 years ago by Jay Rainey
Attempt to fix bad merge. Refs #9084.
Changeset: 72941341a845a66b8fcbba674a9b1729781072cf
Changed 7 years ago by Jay Rainey
- Attachment UpdatedICat.jpg added
An updated class diagram containing CatalogSession and updated CatalogManager
comment:39 Changed 7 years ago by Jay Rainey
Prevent column with name exists error. Refs #9084.
Changeset: dfd34a49c3b33e36b9f729aecb13e056698f4b87
comment:40 Changed 7 years ago by Jay Rainey
Removed unused methods from ICat3Helper. Refs #9084.
Changeset: 4b3d3456920b42803817b188657c697d3ce2b53f
comment:41 Changed 7 years ago by Jay Rainey
Update catalogManger to use catalogSession. Refs #9084.
Changeset: 0d537f9e02315cfbd2c11bd0c56f49d1d8191dc3
comment:42 Changed 7 years ago by Jay Rainey
Replace create method with login. Refs #9084.
- This allows us to obtain and store the session once the user logs into a catalog.
Changeset: 52e4089820dec94cc3ce124b440d9553a2d36cd6
comment:43 Changed 7 years ago by Jay Rainey
Add session property to catalog algorithms. Refs #9084.
Changeset: 8476aca0d7695a71e6a874702a9fa544d130a848
comment:44 Changed 7 years ago by Jay Rainey
Destroy all catalogs if empty sessionID. Refs #9084.
- Updated destroyCatalog to reflect similar changes made to getCatalog. That is, it will now destroy all catalogs if no sessionID is provided when called.
Changeset: 2747172dbd8bd5bdbc77952624dc4c1a25009599
comment:45 Changed 7 years ago by Jay Rainey
Added getActiveSessions method. Refs #9084.
Changeset: 958100fa9bb9c4077e9c2c6da2903d381977e9db
comment:46 Changed 7 years ago by Jay Rainey
Removed unused createCatalog method. Refs #9084.
Changeset: 21a90b49ddcd4f48385da42a35e4ab52f23d6f38
comment:47 Changed 7 years ago by Jay Rainey
Ensure catalogdowndatafiles works. Refs #9084.
Changeset: 450540b06b1f8fbe3b23b91327200778ccfd3dcd
comment:48 Changed 7 years ago by Jay Rainey
Prevent multiple facilities from existing. Refs #9084.
Changeset: 49ac4a2225e499ce7e4074f7b0d242b30b712ab9
comment:49 Changed 7 years ago by Jay Rainey
Ensure CatalogPublish works with a catalog. Refs #9084.
Changeset: 073a26b840193fa2a71a5363b3dd12020d0e512b
comment:50 Changed 7 years ago by Jay Rainey
Remove unnecessary destructor. Refs #9084.
Changeset: 32697789a54e996e8692af65b3140376bd7fb972
comment:51 Changed 7 years ago by Jay Rainey
Set session property in publish algorithm. Refs #9084.
Changeset: 56bc69f293e35cf62c2f06938e396006f8ae42e4
comment:52 Changed 7 years ago by Jay Rainey
- Status changed from inprogress to verify
- Resolution set to fixed
Fix coverity issue. Refs #9084.
Changeset: 3749fe5c4da95416372a69716961067e1b2213f8
comment:54 Changed 7 years ago by Jay Rainey
Minor type consistency improvement. Refs #9084.
Changeset: 8de645fb134645e4d62da9cc23b31431c39ae311
comment:55 Changed 7 years ago by Jay Rainey
Overview
The objective of this ticket was to create a new class that would manage active catalogs and their related session. This is visualised in this class diagram.
Within this ticket, I have created CatalogManager, CatalogSession and removed create from CatalogFactory, which enables multiple catalogs to co-exist. I have opted to disable this functionality as I am unable to get the current code into a testing state without several large changes, which would make this ticket extremely difficult to test. I will instead address this in #9112.
Testing
You need to verify that CatalogManager and CatalogSession are used correctly, and that all catalog functionality works as it did prior to the ticket. To do this you need to:
- Verify the algorithms used in the interfaces function correctly by using the related (e.g. Login, Search, Publish & Logout)
- Manually run each algorithm from the Algorithms menu. (You can leave session empty as it will use the current (and only) session available).
ICat3
As we support ICat3 I made several changes to ICat3Catalog and ICatHelper to ensure it will work with multiple catalogs as expected. To access ICat3 change the following in Facilities.xml:
<catalog name="ICat3Catalog"> <soapendpoint url="https://facilities01.esc.rl.ac.uk:443/ICATService/ICAT"></soapendpoint>
Code review
As I have created several new classes, which are crucial to the functionality of the catalog algorithms you must verify that the changes made address the issue described above.
comment:57 Changed 7 years ago by Peter Parker
- Status changed from verify to verifying
- Tester set to Peter Parker
comment:58 follow-up: ↓ 65 Changed 7 years ago by Peter Parker
- Status changed from verifying to reopened
- Resolution fixed deleted
Commit History Summary
There are some "dangling" (unreachable) commits that have been published as a result of this ticket:
I've discussed these with Owen and it was decided that they should be relatively harmless, though it is unclear whether or not they will ever be garbage collected up on GitHub.
A bigger problem is c9d8866, which is a commit that now only exists on develop. Your merge at 741b28e is probably something that you will end up doing again and again, whenever you make changes to CatalogSession.h or CatalogSession.cpp.
The problem will go away at the next release when develop is regenerated from master, so we'll leave it for now. If things get too bad in the meantime you might want to consider asking one of the senior developers to do it before then.
For future reference, a simple way of moving "private" code between two repos that you own is to use git diff to generate a patch file, and then apply the patch with git apply. Custom editing of published branch history should be avoided if possible.
Code Issues
Code review has highlighted a couple of things we should change.
Returning by Const Reference
I wouldn't say that returning by const reference is bad per se, but it does come with enough enough of a downside to make passing by value what we should be doing by default. The two main issues are 1) the lifetime of the thing being referenced, and 2) unexpected changes that may happen.
An example to run in coliru:
#include <string> #include <iostream> #include <vector> class Object { public: Object(const std::string & content) : m_content(content) {} const std::string & getContentByConstRef() const { return m_content; } void setContent(const std::string & content) { m_content = content; } private: std::string m_content; }; void possiblyUnexpectedBehaviour() { Object test("Some string."); const std::string & message = test.getContentByConstRef(); std::cout << message << std::endl; // The message variable is changing under our feet. In real code, this may // be obfuscated and not very easy to debug. test.setContent("Some other string you may not be expecting."); std::cout << message << std::endl; } const std::string & getString() { Object test("This is a string."); // Uh-oh, we're passing a reference to something that is about to disappear! return test.getContentByConstRef(); } void undefinedBehaviour() { std::cout << "Bad ===> " << getString() << " <=== Bad" << std::endl; } int main() { possiblyUnexpectedBehaviour(); undefinedBehaviour(); }
This produces the following output:
Some string. Some other string you may not be expecting. bash: line 7: 17885 Segmentation fault (core dumped) ./a.out
As a general rule, it is probably best to reserve returning const references to object internals for performance-critical code, or where the thing being returned is really large. Even in those cases, class users then have to be very wary about using the method.
For everything else, returning by value is usually fine, since the compiler optimises things anyway.
Algorithm Input / Output Parameter Order
For CatalogListInstruments, CatalogMyDataSearch, CatalogSearch, CatalogListInvestigationTypes and possibly others, we have introduced some new output parameters. Can we change the order in which they are declared so that input parameters appear before the output ones?
Crashes
- Logging in and then clicking Publish causes a crash.
- Searching for some data and then clicking "Load" for one of the files causes a crash. "Download to..." crashes as well.
There may be more but that's enough to go on for now.
comment:59 Changed 7 years ago by Jay Rainey
- Status changed from reopened to inprogress
Move input property above output. Refs #9084.
Changeset: ae8afaafebe80e93ba09a365e2d644308c813ed1
comment:60 Changed 7 years ago by Jay Rainey
Return by value instead of const reference. Refs #9084.
Changeset: 8198a56f38a5e19a40874a0461c616ec56c35464
comment:61 Changed 7 years ago by Jay Rainey
Prevent crash on manual algorithm use. Refs #9084.
Changeset: 70c5d14c329e4bbb4231814cd2c4ffb3d2f48b9d
comment:62 Changed 7 years ago by Jay Rainey
Attempt to fix cpp error. Refs #9084.
Changeset: daa4a28a65ca6cc30d16cc37fb34ac35d584ed26
comment:63 Changed 7 years ago by Jay Rainey
Revert to const reference. Refs #9084.
- Passing by value does not work on windows when setting the endpoint inside setICATProxySettings in ICat4Catalog.
Changeset: 8e9a609da93e8d544abfea71292149fd754ce464
comment:64 Changed 7 years ago by Jay Rainey
Fix download and upload on windows. Refs #9084.
Changeset: d5ef042b43de8cb37b22fc3c79a7acca6552af83
comment:65 in reply to: ↑ 58 Changed 7 years ago by Jay Rainey
- Status changed from inprogress to verify
- Resolution set to fixed
Replying to Peter Parker:
Returning by Const Reference
As discussed with Peter, I had to revert to using const reference in order for the catalog functionality to work as the icat.soap_endpoint would not accept a string value.
Having debugged this issue with Peter, it appears to be an internal issue with ICatPortBindingProxy, which is automatically generated and out of our hands.
Algorithm Input / Output Parameter Order
I have re-arranged the order to input is first in this commit.
Crashes when downloading/uploading
I have added a temporary measures to ensure the download & upload functionality works with a single catalog.
Interestingly, I was still unable to download or upload after making these changes (no session would be picked up when I was logged in). This was due to DLL issues with the CatalogManager.
comment:66 Changed 7 years ago by Peter Parker
- Status changed from verify to closed
Merge remote-tracking branch 'origin/feature/9084_create_catalog_manager'
Full changeset: 132dda5be15165b6778c045f5be3c1360adaabe0
comment:67 Changed 7 years ago by Peter Parker
Looks good, notwithstanding those pesky const references.
Tested via the algorithms as well as the menu, for both ICat 3 and 4.
comment:68 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 9927