Skip to content
This repository has been archived by the owner on Nov 14, 2022. It is now read-only.

Simplify ICAT Usage #1495

Open
Pasarus opened this issue Aug 18, 2022 · 0 comments
Open

Simplify ICAT Usage #1495

Pasarus opened this issue Aug 18, 2022 · 0 comments

Comments

@Pasarus
Copy link
Member

Pasarus commented Aug 18, 2022

Is this necessary? should we not move this complication upstream to the ICAT developers?

Issue raised by: [developer]

What?
There is no consistent pattern when using ICAT which has lead to duplication and complexity

Where?
The 3 big classes are ICATCache, ICATCommunication and ICATClient
From a basic assessment these 3 can be summarised as follows:

ICATCache

  • Is an inline cache to ICAT
  • Uses an ICATCommunication instance to contact ICAT (Also has a method to get an ICAT Session)

ICATCommunication

  • Has an instance of the external python-icat client
  • Has different methods for different queries (hardcoded strings in methods)
  • Has strange _add_list_to_set() that returns a list (Didn't look much into this but I am GUESSING that this to stop duplicates being added to result lists)

ICATClient

  • wraps python-icat external library
  • query execution
  • session / login / logout functionality

We appear to only use a small subset of the functionality provided by python-icat so wrapping it with a facade makes sense, and the ICATClient does an OK job at this. However I believe that a better API could be created, that will allow us to abstract and encapsulate a lot of the details of the ICATClient.

One idea I had was something like this (very rough idea):
image

Where the ICATClient is responsible for logging in and executing queries etc. The factory returns queries either as the raw strings or some new value object. And the ICATService can be called without knowledge of either the ICATClient or factory

Pros:

  • Single interface for using ICAT
  • Queries in a single location
  • Hides details from consumers e.g. hiding concepts such as icat session from the webapp etc.
  • Simplified testing and removal of similar/duplicate tests
  • Much easier to change how we access ICAT in the future, e.g. if we were to swap to using the future datagateway REST API in the future
  • Remove ICATCache and use a django cache see Investigate Django caching #713

Cons:

  • Could be tricky to implement efficient way of creating icat sessions across multiple queries
  • A large change to make all at once
  • If in the future different parts of this repo were separated into different repos. e.g. webapp being in its own repo. Thought would need to be made into where to keep this and import it across repos.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant