[OGUI-1895] Filter out any empty hosts if returned so by Apricot

Control/lib/api.js

@@ -249,7 +249,14 @@ module.exports.setup = (http, ws) => {
       res.status(503).send({message: error.message});
     }
   });
-  http.get('/core/hostsByDetectors', (req, res) => apricotService.getHostsByDetectorList(req, res));
+  http.get('/core/hostsByDetectors', async (_, res) => {
+    try {
+      const hostsByDetector = await detectorService.getHostsByDetector();
+      res.status(200).json({hosts: Object.fromEntries(hostsByDetector)});
+    } catch (error) {
+      res.status(503).send({message: error.message});
+    }
+  });
 
   http.post('/execute/o2-roc-config', coreMiddleware, (req, res) => ctrlService.createAutoEnvironment(req, res));
 
@@ -378,8 +385,7 @@ async function initializeData(detectorService, apricotService, lockService, cons
 
   const detectors = await detectorService.getDetectorList();
   lockService.setLockStatesForDetectors(detectors);
-
-  await apricotService.init();
+  await detectorService.getHostsByDetector();
 }
 
 /**

Control/lib/control-core/ApricotService.js

@@ -34,35 +34,6 @@ class ApricotService {
     assert(grpcClient, 'Missing GrpcServiceClient dependency for Apricot');
     this._grpcClient = grpcClient;
     this._logger = LogManager.getLogger(`${process.env.npm_config_log_label ?? 'cog'}/apricotservice`);
-
-    this.detectors = [];
-    this.hostsByDetector = new Map();
-  }
-
-  /**
-   * Initialize service with static data from AliECS
-   */
-  async init() {
-    try {
-      this.detectors = await this._loadDetectors();
-      this._logger.infoMessage(`Initial data retrieved from AliECS/Apricot: ${this.detectors} detectors`, {
-        level: 99,
-        system: 'GUI',
-        facility: 'cog/api'
-      });
-      await Promise.allSettled(
-        this.detectors.map(async (detector) => {
-          try {
-            const {hosts} = await this._grpcClient['GetHostInventory']({detector});
-            this.hostsByDetector.set(detector, hosts);
-          } catch (error) {
-            this._logger.error(`Unable to retrieve list of hosts for detector: ${detector}`);
-          }
-        })
-      );
-    } catch (error) {
-      this._logger.error('Unable to list detectors');
-    }
   }
 
   /**
@@ -98,36 +69,6 @@ class ApricotService {
     }
   }
 
-  /**
-   * Return an in-memory map of hosts grouped by their detector
-   * If map is empty, make a request to Apricot
-   * @param {Request} req
-   * @param {Response} res
-   */
-  async getHostsByDetectorList(_, res) {
-    if (this.hostsByDetector.size === 0) {
-      try {
-        this.detectors = await this._loadDetectors();
-
-        await Promise.allSettled(
-          this.detectors.map(async (detector) => {
-            try {
-              const {hosts} = await this._grpcClient['GetHostInventory']({detector});
-              this.hostsByDetector.set(detector, hosts);
-            } catch (error) {
-              this._logger.error(`Unable to retrieve list of hosts for detector: ${detector}`);
-              this._logger.error(error);
-            }
-          })
-        );
-      } catch (error) {
-        errorHandler(error, res, 503, 'apricotservice');
-        return;
-      }
-    }
-    res.status(200).json({hosts: Object.fromEntries(this.hostsByDetector)});
-  }
-
   /**
    * Request a list of detectors from Apricot to confirm
    * connection and O2Apricot are up

Control/lib/services/Detector.service.js

@@ -12,7 +12,7 @@
  * or submit itself to any jurisdiction.
 */
 
-const { grpcErrorToNativeError } = require('@aliceo2/web-ui');
+const { grpcErrorToNativeError, LogManager } = require('@aliceo2/web-ui');
 
 /**
  * @class
@@ -39,6 +39,13 @@ class DetectorService {
      * @type {Array<string>}
      */
     this._detectors = [];
+
+    /**
+     * @type {Map<string, Array<string>>}
+     */
+    this._hostsByDetector = new Map();
+
+    this._logger = LogManager.getLogger(`${process.env.npm_config_log_label ?? 'cog'}/detectorservice`);
   }
 
   /**
@@ -78,6 +85,33 @@ class DetectorService {
     }
   }
 
+  /**
+   * Method to retrieve a map of hosts grouped by their detector via Apricot gRPC service.
+   * If the in-memory cache is non-empty, it is returned directly without querying Apricot.
+   * Individual detector failures are non-fatal; the partial result is cached and returned.
+   * @returns {Promise<Map<string, Array<string>>>} - map of detector name to list of hosts
+   * @throws {Error} - throws JS native error converted from gRPC error if detector list fetch fails
+   */
+  async getHostsByDetector() {
+    if (this._hostsByDetector.size > 0) {
+      return this._hostsByDetector;
+    }
+
+    const detectors = await this.getDetectorList();
+    await Promise.allSettled(
+      detectors.map(async (detector) => {
+        try {
+          const { hosts } = await this._apricotGrpcClient.GetHostInventory({ detector });
+          const filteredHosts = hosts.filter((host) => typeof host === 'string' && host.trim().length > 0);
+          this._hostsByDetector.set(detector, filteredHosts);
+        } catch (error) {
+          this._logger.errorMessage(`Failed to retrieve hosts for detector ${detector}: ${error.message}`);
+        }
+      })
+    );
+    return this._hostsByDetector;
+  }
+
   /**
    * Getter for the list of detectors cached in memory
    * @returns {Array<string>}
@@ -93,6 +127,14 @@ class DetectorService {
   set detectors(detectors) {
     this._detectors = detectors;
   }
+
+  /**
+   * Getter for the hosts-by-detector map cached in memory
+   * @returns {Map<string, Array<string>>}
+   */
+  get hostsByDetector() {
+    return this._hostsByDetector;
+  }
 }
 
 module.exports = {DetectorService};

Control/test/api/detectors/api-get-hosts-by-detector.test.js

@@ -0,0 +1,60 @@
+/**
+ * @license
+ * Copyright 2019-2020 CERN and copyright holders of ALICE O2.
+ * See http://alice-o2.web.cern.ch/copyright for details of the copyright holders.
+ * All rights not expressly granted are reserved.
+ *
+ * This software is distributed under the terms of the GNU General Public
+ * License v3 (GPL Version 3), copied verbatim in the file "COPYING".
+ *
+ * In applying this license CERN does not waive the privileges and immunities
+ * granted to it by virtue of its status as an Intergovernmental Organization
+ * or submit itself to any jurisdiction.
+ */
+
+const assert = require('assert');
+const request = require('supertest');
+const test = require('../../mocha-index');
+const {ADMIN_TEST_TOKEN, TEST_URL} = require('../generateToken.js');
+
+describe("'API - GET - /core/hostsByDetectors' test suite", () => {
+  let apricotCalls;
+
+  // @link{test/config/apricot-grpc.js} — getHostInventory returns ['ali-flp-22', 'ali-flp-23'] for every detector
+  const EXPECTED_HOSTS = {
+    hosts: {
+      MID: ['ali-flp-22', 'ali-flp-23'],
+      DCS: ['ali-flp-22', 'ali-flp-23'],
+      ODC: ['ali-flp-22', 'ali-flp-23'],
+    },
+  };
+
+  before(() => {
+    apricotCalls = test.helpers.apricotCalls;
+  });
+
+  beforeEach(() => {
+    apricotCalls['getHostInventory'] = undefined;
+  });
+
+  it('should successfully retrieve hosts grouped by detector', async () => {
+    await request(`${TEST_URL}/api`)
+      .get(`/core/hostsByDetectors?token=${ADMIN_TEST_TOKEN}`)
+      .expect(200, EXPECTED_HOSTS);
+  });
+
+  it('should serve hosts from in-memory cache without calling apricot again', async () => {
+    // First request can either warm the cache or use an already warm cache.
+    await request(`${TEST_URL}/api`)
+      .get(`/core/hostsByDetectors?token=${ADMIN_TEST_TOKEN}`)
+      .expect(200, EXPECTED_HOSTS);
+
+    apricotCalls['getHostInventory'] = undefined;
+
+    await request(`${TEST_URL}/api`)
+      .get(`/core/hostsByDetectors?token=${ADMIN_TEST_TOKEN}`)
+      .expect(200, EXPECTED_HOSTS);
+
+    assert.strictEqual(apricotCalls['getHostInventory'], undefined);
+  });
+});

Control/test/lib/control-core/mocha-apricot.service.test.js

@@ -58,57 +58,7 @@ describe('ApricotService test suite', () => {
       assert.rejects(() => apricotService.getStatus(), new Error('Apricot is not working'));
     });
   });
-
-  describe('Check detectors caching', () => {
-    let req, res;
-
-    beforeEach(() => {
-      req = {};
-      res = {
-        status: sinon.stub().returnsThis(),
-        json: sinon.spy(),
-        send: sinon.spy(),
-      };
-    });
-
-    it('should successfully request hosts for each detector from AliECS core if none are present', async () => {
-      const apricotProxy = {
-        isConnectionReady: true,
-        ListDetectors: sinon.stub().resolves({detectors: ['TST']}),
-        GetHostInventory: sinon.stub().resolves({hosts: ['flp001']}),
-      };
-      const apricotService = new ApricotService(apricotProxy);
-      await apricotService.getHostsByDetectorList(req, res);
-      assert.ok(res.status.calledOnce);
-      assert.ok(res.status.calledWith(200));
-      assert.ok(res.json.calledOnce);
-      assert.ok(res.json.calledWith({hosts: {TST: ['flp001']}}));
-    });
-
-    it('should successfully return a map of hosts grouped by detectors if already present', async () => {
-      const apricotService = new ApricotService({});
-      apricotService.hostsByDetector = new Map([['TST', ['flp001']]]);
-      await apricotService.getHostsByDetectorList(req, res);
-      assert.ok(res.status.calledOnce);
-      assert.ok(res.status.calledWith(200));
-      assert.ok(res.json.calledOnce);
-      assert.ok(res.json.calledWith({hosts: {TST: ['flp001']}}));
-    });
-
-    it('should return error response if hostsByDetectors are not present and AliECS replies with error for initial detector request', async () => {
-      const apricotProxy = {
-        isConnectionReady: true,
-        ListDetectors: sinon.stub().rejects(new Error('unable to load detector list')),
-      };
-      const apricotService = new ApricotService(apricotProxy);
-      await apricotService.getHostsByDetectorList(req, res);
-      assert.ok(res.status.calledOnce);
-      assert.ok(res.status.calledWith(503));
-      assert.ok(res.send.calledOnce);
-      assert.ok(res.send.calledWith({message: 'unable to load detector list'}));
-    });
-  });
-
+
   describe('Check executing commands through `GrpcServiceClient`', () => {
     let apricotService;
     let req, res;

Control/test/lib/services/mocha-detector.service.test.js

@@ -107,4 +107,66 @@ describe(`'DetectorService' test suite`, () => {
       );
     });
   });
+
+  describe(`'getHostsByDetector' test suite`, async () => {
+    it('should initialize with empty in-memory hosts map', () => {
+      const detectorService = new DetectorService({}, {});
+      assert.deepStrictEqual(detectorService.hostsByDetector.size, 0);
+    });
+
+    it('should successfully retrieve a map of hosts grouped by detector from apricot', async () => {
+      const detectorService = new DetectorService({}, {
+        ListDetectors: sinon.stub().resolves({detectors: ['TPC', 'TRD']}),
+        GetHostInventory: sinon.stub().resolves({hosts: ['flp001', 'flp002']})
+      });
+
+      const hostsByDetector = await detectorService.getHostsByDetector();
+      assert.deepStrictEqual(hostsByDetector, new Map([
+        ['TPC', ['flp001', 'flp002']],
+        ['TRD', ['flp001', 'flp002']],
+      ]));
+    });
+
+    it('should remove empty and whitespace-only hosts from each detector entry', async () => {
+      const detectorService = new DetectorService({}, {
+        ListDetectors: sinon.stub().resolves({detectors: ['TPC']}),
+        GetHostInventory: sinon.stub().resolves({hosts: ['flp001', '', '   ', '\t', 'flp002']})
+      });
+
+      const hostsByDetector = await detectorService.getHostsByDetector();
+      assert.deepStrictEqual(hostsByDetector, new Map([['TPC', ['flp001', 'flp002']]]));
+    });
+
+    it('should cache result and return it without calling apricot again', async () => {
+      const listDetectorsStub = sinon.stub().resolves({detectors: ['SHOULD_NOT_BE_USED']});
+      const detectorService = new DetectorService({}, {ListDetectors: listDetectorsStub});
+      detectorService._hostsByDetector = new Map([['TPC', ['flp001']]]);
+
+      const hostsByDetector = await detectorService.getHostsByDetector();
+      assert.deepStrictEqual(hostsByDetector, new Map([['TPC', ['flp001']]]));
+      assert.ok(listDetectorsStub.notCalled);
+    });
+
+    it('should return partial result when individual GetHostInventory calls fail', async () => {
+      const detectorService = new DetectorService({}, {
+        ListDetectors: sinon.stub().resolves({detectors: ['TPC', 'TRD']}),
+        GetHostInventory: sinon.stub()
+          .onFirstCall().resolves({hosts: ['flp001']})
+          .onSecondCall().rejects(new Error('host fetch failed'))
+      });
+
+      const hostsByDetector = await detectorService.getHostsByDetector();
+      assert.deepStrictEqual(hostsByDetector, new Map([['TPC', ['flp001']]]));
+    });
+
+    it('should reject with JS native error if ListDetectors fails', async () => {
+      const detectorService = new DetectorService({}, {
+        ListDetectors: sinon.stub().rejects({code: 4, details: 'Timeout'})
+      });
+      await assert.rejects(
+        () => detectorService.getHostsByDetector(),
+        (error) => error instanceof TimeoutError && error.message === 'Timeout'
+      );
+    });
+  });
 });

Control/test/mocha-index.js

@@ -183,6 +183,7 @@ describe('Control', function() {
   require('./api/configuration/api-get-configuration-restrictions.test');
   require('./api/configuration/api-put-configuration.test');
   require('./api/detectors/api-get-detectors.test');
+  require('./api/detectors/api-get-hosts-by-detector.test');
 
   beforeEach(() => this.ok = true);