refactor: Apply PR #576 review comments and improve architecture

Changes based on @neSpecc review:

1. TypeDefs improvements:
   - Use DateTime! instead of String! for date parameters in Project.chartData

2. Method renaming for clarity:
   - eventsFactory: getChartData → getProjectChartData
   - eventsFactory: getChartDataFromMongo → getEventDailyChart
   - Improved parameter order (main identifier first)

3. Code quality:
   - Remove duplication in try-catch blocks
   - Extract Redis key composition to utility functions

4. Architecture improvements:
   - Create ChartDataService to separate business logic from Redis client
   - Simplify RedisHelper to only low-level Redis operations
   - Add tsRange() method for TS.RANGE commands
   - Move chart data logic from redisHelper to dedicated service

5. New utilities:
   - src/utils/redisKeys.ts with composeTimeSeriesKey(), composeEventTimeSeriesKey()
   - Type-safe key composition with required parameters

Benefits:
- Clear separation of concerns (Data Access vs Business Logic vs Service)
- RedisHelper stays focused and doesn't grow
- Easier to test and maintain
- Better type safety and documentation

Author: pavelzotikov

src/models/eventsFactory.js

@@ -8,6 +8,7 @@ const mongo = require('../mongo');
 const Event = require('../models/event');
 const { ObjectID } = require('mongodb');
 import RedisHelper from '../redisHelper';
+import ChartDataService from '../services/chartDataService';
 const { composeEventPayloadByRepetition } = require('../utils/merge');
 
 const MAX_DB_READ_BATCH_SIZE = Number(process.env.MAX_DB_READ_BATCH_SIZE);
@@ -71,11 +72,6 @@ const MAX_DB_READ_BATCH_SIZE = Number(process.env.MAX_DB_READ_BATCH_SIZE);
  * Factory Class for Event's Model
  */
 class EventsFactory extends Factory {
-  /**
-   * Redis helper instance for modifying data through redis (singleton)
-   */
-  redis = RedisHelper.getInstance();
-
   /**
    * Event types with collections where they stored
    * @return {{EVENTS: string, DAILY_EVENTS: string, REPETITIONS: string, RELEASES: string}}
@@ -97,6 +93,16 @@ class EventsFactory extends Factory {
   constructor(projectId) {
     super();
 
+    /**
+     * Redis helper instance (singleton)
+     */
+    this.redis = RedisHelper.getInstance();
+
+    /**
+     * Chart data service for fetching data from Redis TimeSeries
+     */
+    this.chartDataService = new ChartDataService(this.redis);
+
     if (!projectId) {
       throw new Error('Can not construct Event model, because projectId is not provided');
     }
@@ -424,54 +430,52 @@ class EventsFactory extends Factory {
   }
 
   /**
-   * Get chart data for projects (uses Redis with fallback to MongoDB)
+   * Get project chart data from Redis or fallback to MongoDB
    *
-   * @param {string} startDate - start date (ISO string or Unix timestamp)
-   * @param {string} endDate - end date (ISO string or Unix timestamp)
-   * @param {number} groupBy - grouping interval in minutes
-   * @param {number} timezoneOffset - user's local timezone offset in minutes
    * @param {string} projectId - project ID
-   * @param {string} groupHash - event's group hash (empty for project-level data)
+   * @param {string} startDate - start date (ISO string)
+   * @param {string} endDate - end date (ISO string)
+   * @param {number} groupBy - grouping interval in minutes (1=minute, 60=hour, 1440=day)
+   * @param {number} timezoneOffset - user's local timezone offset in minutes
    * @returns {Promise<Array>}
    */
-  async getChartData(startDate, endDate, groupBy = 60, timezoneOffset = 0, projectId = '', groupHash = '') {
+  async getProjectChartData(projectId, startDate, endDate, groupBy = 60, timezoneOffset = 0) {
+    // Calculate days for MongoDB fallback
+    const start = new Date(startDate).getTime();
+    const end = new Date(endDate).getTime();
+    const days = Math.ceil((end - start) / (24 * 60 * 60 * 1000));
+
     try {
-      const redisData = await this.redis.getChartDataFromRedis(
+      const redisData = await this.chartDataService.getProjectChartData(
+        projectId,
         startDate,
         endDate,
         groupBy,
-        timezoneOffset,
-        projectId,
-        groupHash
+        timezoneOffset
       );
 
       if (redisData && redisData.length > 0) {
         return redisData;
       }
 
-      // Fallback to Mongo
-      const start = new Date(startDate).getTime();
-      const end = new Date(endDate).getTime();
-      const days = Math.ceil((end - start) / (24 * 60 * 60 * 1000));
-      return this.findChartData(days, timezoneOffset, groupHash);
+      // Fallback to Mongo (empty groupHash for project-level data)
+      return this.findChartData(days, timezoneOffset, '');
     } catch (err) {
-      console.error('[EventsFactory] getChartData error:', err);
-      const start = new Date(startDate).getTime();
-      const end = new Date(endDate).getTime();
-      const days = Math.ceil((end - start) / (24 * 60 * 60 * 1000));
-      return this.findChartData(days, timezoneOffset, groupHash);
+      console.error('[EventsFactory] getProjectChartData error:', err);
+      // Fallback to Mongo on error (empty groupHash for project-level data)
+      return this.findChartData(days, timezoneOffset, '');
     }
   }
 
   /**
-   * Get chart data from MongoDB only (for events)
+   * Get event daily chart data from MongoDB only
    *
+   * @param {string} groupHash - event's group hash
    * @param {number} days - how many days to fetch
    * @param {number} timezoneOffset - user's local timezone offset in minutes
-   * @param {string} groupHash - event's group hash
    * @returns {Promise<Array>}
    */
-  async getChartDataFromMongo(days, timezoneOffset = 0, groupHash = '') {
+  async getEventDailyChart(groupHash, days, timezoneOffset = 0) {
     return this.findChartData(days, timezoneOffset, groupHash);
   }
 

src/redisHelper.ts

@@ -116,95 +116,32 @@ export default class RedisHelper {
   public isConnected(): boolean {
     return this.redisClient.isOpen;
   }
-  
-  public async getChartDataFromRedis(
-    startDate: string,
-    endDate: string,
-    groupBy: number, // minutes: 1=minute, 60=hour, 1440=day
-    timezoneOffset = 0,
-    projectId = '',
-    groupHash = ''
-  ): Promise<{ timestamp: number; count: number }[]> {
-    // If Redis is not connected, throw error to fallback to MongoDB
-    if (!this.redisClient.isOpen) {
-      console.warn('[Redis] Client not connected, will fallback to MongoDB');
-      throw new Error('Redis client not connected');
-    }
-
-    // Determine suffix based on groupBy
-    let suffix: string;
-    if (groupBy === 1) {
-      suffix = 'minutely';
-    } else if (groupBy === 60) {
-      suffix = 'hourly';
-    } else if (groupBy === 1440) {
-      suffix = 'daily';
-    } else {
-      // For custom intervals, fallback to minutely with aggregation
-      suffix = 'minutely';
-    }
-
-    const key = groupHash
-      ? `ts:events:${groupHash}:${suffix}`
-      : projectId
-      ? `ts:events:${projectId}:${suffix}`
-      : `ts:events:${suffix}`;
-
-    // Parse dates (support ISO string or Unix timestamp in seconds)
-    const start = typeof startDate === 'string' && startDate.includes('-')
-      ? new Date(startDate).getTime()
-      : Number(startDate) * 1000;
-    const end = typeof endDate === 'string' && endDate.includes('-')
-      ? new Date(endDate).getTime()
-      : Number(endDate) * 1000;
-
-    const bucketMs = groupBy * 60 * 1000;
-
-    let result: [string, string][] = [];
-    try {
-      // Use aggregation to sum values within each bucket
-      // TS.INCRBY creates one point per time period with accumulated count
-      result = (await this.redisClient.sendCommand([
-        'TS.RANGE',
-        key,
-        start.toString(),
-        end.toString(),
-        'AGGREGATION',
-        'sum',
-        bucketMs.toString(),
-      ])) as [string, string][] | [];
-    } catch (err: any) {
-      if (err.message.includes('TSDB: the key does not exist')) {
-        console.warn(`[Redis] Key ${key} does not exist, returning zeroed data`);
-        result = [];
-      } else {
-        throw err;
-      }
-    }
 
-    // Transform data from Redis
-    const dataPoints: { [ts: number]: number } = {};
-    for (const [tsStr, valStr] of result) {
-      const tsMs = Number(tsStr);
-      dataPoints[tsMs] = Number(valStr) || 0;
-    }
-
-    // Fill missing intervals with zeros
-    const filled: { timestamp: number; count: number }[] = [];
-    let current = start;
-
-    // Round current to the nearest bucket boundary
-    current = Math.floor(current / bucketMs) * bucketMs;
-
-    while (current <= end) {
-      const count = dataPoints[current] || 0;
-      filled.push({
-        timestamp: Math.floor((current + timezoneOffset * 60 * 1000) / 1000),
-        count,
-      });
-      current += bucketMs;
-    }
-
-    return filled.sort((a, b) => a.timestamp - b.timestamp);
+  /**
+   * Execute TS.RANGE command with aggregation
+   *
+   * @param key - Redis TimeSeries key
+   * @param start - start timestamp in milliseconds
+   * @param end - end timestamp in milliseconds
+   * @param aggregationType - aggregation type (sum, avg, min, max, etc.)
+   * @param bucketMs - bucket size in milliseconds
+   * @returns Array of [timestamp, value] tuples
+   */
+  public async tsRange(
+    key: string,
+    start: string,
+    end: string,
+    aggregationType: string,
+    bucketMs: string
+  ): Promise<[string, string][]> {
+    return (await this.redisClient.sendCommand([
+      'TS.RANGE',
+      key,
+      start,
+      end,
+      'AGGREGATION',
+      aggregationType,
+      bucketMs,
+    ])) as [string, string][];
   }
 }

src/resolvers/event.js

@@ -86,7 +86,7 @@ module.exports = {
     async chartData({ projectId, groupHash }, { days, timezoneOffset }, context) {
       const factory = getEventsFactory(context, projectId);
 
-      return factory.getChartDataFromMongo(days, timezoneOffset, groupHash);
+      return factory.getEventDailyChart(groupHash, days, timezoneOffset);
     },
 
     /**

src/resolvers/project.js

@@ -486,7 +486,7 @@ module.exports = {
     async chartData(project, { startDate, endDate, groupBy, timezoneOffset }, context) {
       const factory = getEventsFactory(context, project._id);
 
-      return factory.getChartData(startDate, endDate, groupBy, timezoneOffset, project._id);
+      return factory.getProjectChartData(project._id, startDate, endDate, groupBy, timezoneOffset);
     },
 
     /**

src/services/chartDataService.ts

@@ -0,0 +1,92 @@
+import RedisHelper from '../redisHelper';
+import { composeTimeSeriesKey, getTimeSeriesSuffix } from '../utils/redisKeys';
+
+/**
+ * Service for fetching chart data from Redis TimeSeries
+ */
+export default class ChartDataService {
+  private redisHelper: RedisHelper;
+
+  constructor(redisHelper: RedisHelper) {
+    this.redisHelper = redisHelper;
+  }
+
+  /**
+   * Get project chart data from Redis TimeSeries
+   *
+   * @param projectId - project ID
+   * @param startDate - start date as ISO string (e.g., '2025-01-01T00:00:00Z')
+   * @param endDate - end date as ISO string (e.g., '2025-01-31T23:59:59Z')
+   * @param groupBy - grouping interval in minutes (1=minute, 60=hour, 1440=day)
+   * @param timezoneOffset - user's local timezone offset in minutes (default: 0)
+   * @returns Array of data points with timestamp and count
+   * @throws Error if Redis is not connected (caller should fallback to MongoDB)
+   */
+  public async getProjectChartData(
+    projectId: string,
+    startDate: string,
+    endDate: string,
+    groupBy: number,
+    timezoneOffset = 0
+  ): Promise<{ timestamp: number; count: number }[]> {
+    // Check if Redis is connected
+    if (!this.redisHelper.isConnected()) {
+      console.warn('[ChartDataService] Redis not connected, will fallback to MongoDB');
+      throw new Error('Redis client not connected');
+    }
+
+    // Determine suffix and compose key
+    const suffix = getTimeSeriesSuffix(groupBy);
+    const key = composeTimeSeriesKey(suffix, projectId);
+
+    // Parse ISO date strings to milliseconds
+    const start = new Date(startDate).getTime();
+    const end = new Date(endDate).getTime();
+    const bucketMs = groupBy * 60 * 1000;
+
+    // Fetch data from Redis
+    let result: [string, string][] = [];
+    try {
+      result = await this.redisHelper.tsRange(
+        key,
+        start.toString(),
+        end.toString(),
+        'sum',
+        bucketMs.toString()
+      );
+    } catch (err: any) {
+      if (err.message.includes('TSDB: the key does not exist')) {
+        console.warn(`[ChartDataService] Key ${key} does not exist, returning zeroed data`);
+        result = [];
+      } else {
+        throw err;
+      }
+    }
+
+    // Transform data from Redis
+    const dataPoints: { [ts: number]: number } = {};
+    for (const [tsStr, valStr] of result) {
+      const tsMs = Number(tsStr);
+      dataPoints[tsMs] = Number(valStr) || 0;
+    }
+
+    // Fill missing intervals with zeros
+    const filled: { timestamp: number; count: number }[] = [];
+    let current = start;
+
+    // Round current to the nearest bucket boundary
+    current = Math.floor(current / bucketMs) * bucketMs;
+
+    while (current <= end) {
+      const count = dataPoints[current] || 0;
+      filled.push({
+        timestamp: Math.floor((current + timezoneOffset * 60 * 1000) / 1000),
+        count,
+      });
+      current += bucketMs;
+    }
+
+    return filled.sort((a, b) => a.timestamp - b.timestamp);
+  }
+}
+

src/typeDefs/project.ts

@@ -354,14 +354,14 @@ type Project {
   """
   chartData(
     """
-    Start date (ISO string or Unix timestamp in seconds)
+    Start date (ISO 8601 DateTime string)
     """
-    startDate: String!
+    startDate: DateTime!
 
     """
-    End date (ISO string or Unix timestamp in seconds)
+    End date (ISO 8601 DateTime string)
     """
-    endDate: String!
+    endDate: DateTime!
 
     """
     Grouping interval in minutes (1=minute, 60=hour, 1440=day)