Ticket #9084 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

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:

  1. Remove create() from CatalogFactory as it currently creates only one catalog.
  2. 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.
  3. 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:

  1. CatalogManager will hold a list of active catalogs and their sessionId in a member variable (m_activeCatalogs).
  2. Create Two methods (getActiveCatalogs() and getActiveCatalog(sessionId) in CatalogManger. One to obtain all active catalogs, and the other to obtain a specific catalog.
  3. A create method to add the catalog to the compositeCatalog and returns that catalog.
  4. 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

UpdatedICat.jpg (65.9 KB) - added by Jay Rainey 7 years ago.
An updated class diagram containing CatalogSession and updated CatalogManager

Change History

comment:1 Changed 7 years ago by Jay Rainey

  • Description modified (diff)

comment:2 Changed 7 years ago by Jay Rainey

  • Description modified (diff)

comment:3 Changed 7 years ago by Nick Draper

  • Status changed from new to assigned

comment:4 Changed 7 years ago by Jay Rainey

  • Blocked By 9069 added; 9062 removed

comment:5 Changed 7 years ago by Jay Rainey

  • Blocked By 9026 added; 9069 removed

comment:6 Changed 7 years ago by Jay Rainey

  • Status changed from assigned to inprogress
Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:7 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:8 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:9 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:10 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:11 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:12 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:13 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:14 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:15 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:16 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:17 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:18 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:19 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:20 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:21 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:22 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:23 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:24 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:25 Changed 7 years ago by Jay Rainey

Last edited 7 years ago by Jay Rainey (previous) (diff)

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

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

  • Blocking 9112 added

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

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:54 Changed 7 years ago by Jay Rainey

Minor type consistency improvement. Refs #9084.

Changeset: 8de645fb134645e4d62da9cc23b31431c39ae311

Last edited 7 years ago by Jay Rainey (previous) (diff)

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:

  1. Verify the algorithms used in the interfaces function correctly by using the related (e.g. Login, Search, Publish & Logout)
  2. 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.

Last edited 7 years ago by Jay Rainey (previous) (diff)

comment:56 Changed 7 years ago by Jay Rainey

  • Blocking 9089 added

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

Note: See TracTickets for help on using tickets.