#3249 Refactor DAO layer for modular architecture by Patrykb0802 · Pull Request #3252 · SCADA-LTS/Scada-LTS

Patrykb0802 · 2026-03-12T11:11:26Z

introduce and complete DAO interface contracts across core modules
align DAO implementations, caches and services with interface-based access
replace scattered DAO factory usage with centralized database-aware bean resolution

- introduce and complete DAO interface contracts across core modules - align DAO implementations, caches and services with interface-based access - replace scattered DAO factory usage with centralized database-aware bean resolution

coderabbitai · 2026-03-12T11:11:37Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fdd37612-66e1-4d1a-9e3e-f6d5456f62ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feature/#3249_Refactor_DAO_layer_for_modular_architecture

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-03-12T11:13:11Z

Java Script Mocha Unit Test Results

268 tests ±0 268 ✅ ±0 3s ⏱️ ±0s
70 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 678ee67. ± Comparison against base commit 015f144.

♻️ This comment has been updated with latest results.

github-actions · 2026-03-12T11:13:26Z

Java JUnit Test Results

3 843 tests ±0 3 843 ✅ ±0 55s ⏱️ -2s
135 suites ±0 0 💤 ±0
135 files ±0 0 ❌ ±0

Results for commit 678ee67. ± Comparison against base commit 015f144.

♻️ This comment has been updated with latest results.

Patrykb0802 · 2026-03-24T17:53:53Z

@coderabbitai review

coderabbitai · 2026-03-24T17:54:02Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Limraj · 2026-03-24T17:43:54Z

-                                else{
-                                    StreamUtils.transfer(rs.getBlob(fieldName).getBinaryStream(),out);
-                                }
+                                StreamUtils.transfer(ApplicationBeans.getBinaryDataHandler().getBinaryStream(rs, fieldName), out);


What's the point of this change? We try to minimize changes unless they're necessary.

Limraj · 2026-03-24T17:46:56Z

    @Override
-    public DatabaseType getType() {
-        return DatabaseType.DERBY;
+    public String getTypeKey() {


We never do things like that. A specific type is always better than a generic one, and it contains a lot of information about what it is and how to use it. We also don't change the old API.

Limraj · 2026-03-24T17:48:00Z

-							if (is == null)
-								return null;
-						}
+						InputStream is = ApplicationBeans.getBinaryDataHandler().getBinaryStream(rs, 1);


What is the purpose of this change? We strive to minimize changes unless they are necessary.

Limraj · 2026-03-24T17:49:28Z

 				upgrade.upgrade();
-				new SystemSettingsDAO().setValue(
+				ISystemSettingsDAO systemSettingsDAO = org.scada_lts.web.beans.ApplicationBeans.getSystemSettingsDAOBean();
+				systemSettingsDAO.setValue(


getSystemSettingsDAOBean -> getSystemSettingsDaoBean (camel)

Limraj · 2026-03-24T17:51:11Z

 	}

-	abstract public DatabaseType getType();
+	abstract public String getTypeKey();


We never make such changes, the concrete enum type is perfectly suited for this task and this type should remain in use. Additionally, we are not changing the API in such a radical way.

Limraj · 2026-03-24T19:18:56Z

+                    // Keep DAO query free from NOT IN to avoid SQL-dialect issues.
                    List<DataPointVO>  points = dataPointService.getDataPoints(searchDataPointJson.getKeywordSearch(),
-                            searchDataPointJson.getExcludeIds(), searchDataPointJson.isStartsWith(),
+                            null, searchDataPointJson.isStartsWith(),


Have you changed the logic here? Exclude id should appear in both cases. This is optimization.

Limraj · 2026-03-24T19:20:26Z

+                                || (searchDataPointJson.getSettable() != null);
+                boolean hasExcludeIds =
+                        searchDataPointJson.getExcludeIds() != null && !searchDataPointJson.getExcludeIds().isEmpty();
+                if(!hasTypeOrSettableFilter && !hasExcludeIds) {


Changing the logic. This is an optimization because we can't filter the sql query by the settable, dataType field, so we only apply the optimization if it's not set.

Limraj · 2026-03-24T19:35:58Z

 				new int[] {
-						Common.getEnvironmentProfile().getString("db.type")
-								.equals("postgres") ? Types.BINARY : Types.BLOB,
+						ApplicationBeans.getBinaryDataHandler().getBinarySqlType(),


Instead of creating a separate class to only set the type, the type can be passed to a method in the enum

Limraj · 2026-03-24T19:36:29Z


+	private static DatabaseAccess resolveDatabaseAccess(String dbKey) {
+		String normalized = normalizeDbKey(dbKey);
+		String beanName = "databaseAccess-" + normalized;


Basing bean customization on your own naming conventions hard code is not a good practice, it is better to base it on the type, so that Spring itself determines the correct bean.

Limraj · 2026-03-24T19:37:12Z

-			data.put("filedataCount", 0);
+		String dbType = checkTypeDB();
+		if (dbType.equalsIgnoreCase("mysql") || dbType.equalsIgnoreCase("postgres")) {
+			double size = ApplicationBeans.getSystemSettingsDAOBean().getDataBaseSize();


getDatabaseSize();

Limraj · 2026-04-13T16:46:04Z

        DataPointService dataPointService = new DataPointService();
        DataPointVO dataPoint = dataPointService.getDataPoint(dataPointId);
-        DwrResponseI18n response = super.toggleDataPoint(dataPoint);
+        DwrResponseI18n response = super.toggleDataPointInternal(dataPoint);


Why was the name changed?

Limraj · 2026-04-13T16:47:09Z

-		ViewHierarchyService service = ApplicationBeans.getViewHierarchyServiceBean();
-
-		cache = service.getAll();
+		cache = new ViewHierarchyService().getAll();


ViewHierarchyService service = new ViewHierarchyService(); cache = service.getAll();

Limraj · 2026-04-13T16:52:40Z

    }
+
+    @Override
+    public List<PointEventDetectorVO> getPointEventDetectors(long limit, int offset) {


Why such changes? Besides, there's a reason the init method refers directly to the DAO; it shouldn't be accessible outside the init method; caching doesn't work there.

Limraj · 2026-04-13T17:21:16Z

-            ConfigurationDB.useOracle11gDB();
-        } else {
-            ConfigurationDB.useDerbyDB();
+        String dbKey = DatabaseType.from(databaseType).getKey();


use switch with enum:

DatabaseType dbKey = DatabaseType.from(databaseType); switch (dbKey) { case MYSQL: ConfigurationDB.useMysqlDB(); break; case POSTGRES: ConfigurationDB.usePostgresDB(); break; case MSSQL: ConfigurationDB.useMssqlDB(); break; case ORACLE11G: ConfigurationDB.useOracle11gDB(); break; default: ConfigurationDB.useDerbyDB(); break; }

But it can be moved to enum under one method e.g. use() and different implementations depending on the enum instance:

public enum DatabaseType { DERBY { @Override DatabaseAccess getImpl() { return new DerbyAccess(); } @Override public void use() { ConfigurationDB.useDerbyDB(); } }, MSSQL { @Override DatabaseAccess getImpl() { return new MSSQLAccess(); } @Override public void use() { ConfigurationDB.useMssqlDB(); } }, MYSQL { @Override DatabaseAccess getImpl() { return new MySQLAccess(); } @Override public InputStream getBinaryStream(ResultSet rs, int columnIndex) throws SQLException { Blob blob = rs.getBlob(columnIndex); return blob == null ? null : blob.getBinaryStream(); } @Override public InputStream getBinaryStream(ResultSet rs, String columnLabel) throws SQLException { Blob blob = rs.getBlob(columnLabel); return blob == null ? null : blob.getBinaryStream(); } @Override public void use() { ConfigurationDB.useMysqlDB(); } }, POSTGRES { @Override DatabaseAccess getImpl() { return new PostgreSQLAccess(); } @Override public int getBinarySqlType() { return Types.BINARY; } @Override public void use() { ConfigurationDB.usePostgresDB(); } }, ORACLE11G { @Override DatabaseAccess getImpl() { return new Oracle11GAccess(); } @Override public void use() { ConfigurationDB.useOracle11gDB(); } }; public String getKey() { return name().toLowerCase(); } public static DatabaseType from(String dbType) { if (dbType == null) { throw new IllegalArgumentException("Unknown database type: null"); } String normalized = dbType.trim().toLowerCase(); switch (normalized) { case "postgresql": case "pgsql": case "pg": return POSTGRES; case "mssqlserver": case "mssql": return MSSQL; case "mysqlserver": case "mysql": return MYSQL; default: return DatabaseType.valueOf(normalized.toUpperCase()); } } public InputStream getBinaryStream(ResultSet rs, int columnIndex) throws SQLException { return rs.getBinaryStream(columnIndex); } public InputStream getBinaryStream(ResultSet rs, String columnLabel) throws SQLException { return rs.getBinaryStream(columnLabel); } public int getBinarySqlType() { return Types.BLOB; } abstract DatabaseAccess getImpl(); public abstract void use(); }

then:

public void setDatabaseType(String database) { DatabaseType databaseType = DatabaseType.from(database); databaseType.use(); }

Limraj · 2026-04-13T17:28:24Z

+
+    List<PointValue> getLatestPointValues(int dataPointId, int limit);
+
+    List<PointValue> getLatestPointValues(int dataPointId, int limit, long before);


What's the purpose of expanding this interface with new methods? There are already a lot of changes; here, we need to limit them to only the essentials.

Limraj · 2026-04-13T17:30:07Z

+package org.scada_lts.dao;
+
+public interface ISystemSettingsDAO {
+    public abstract void setValue(java.lang.String arg0, java.lang.String arg1);


Where are the variable names in these methods? This is an important element of information about how to use this method, what to do, and what to pass.

Limraj · 2026-04-13T17:30:51Z

    default void init() {}

-    View findByName(String name);
+    List<View> findAll();


We should not add new methods in this task.

Limraj · 2026-04-13T17:31:46Z

 			LOG.trace("insertPointLink(PointLinkVO pointLink) pointLink:" + pointLink.toString());
 		}

-		KeyHolder keyHolder = new GeneratedKeyHolder();


Is this implementation not working?

Limraj · 2026-04-13T17:34:13Z

@@ -1,273 +1,367 @@
 {
  "name": "scada-lts-webapp",


You need to revert WebContent/resources/package-lock.json

Limraj · 2026-04-13T17:59:20Z

-  <bean id="dataSourceDAO" class="org.scada_lts.dao.DataSourceDAO"/>
-  <bean id="userDAO" class="org.scada_lts.dao.UserDAO"/>
+  <bean id="dataPointUserDAO-mysql" class="org.scada_lts.dao.DataPointUserDAO"/>
+  <bean id="dataPointUserDAO" class="org.scada_lts.factory.DatabaseAwareBeanFactory">


Instead of using this class, the logic for getting the prefix etc. can be included, for example, in the ApplicationBeans class.

instead:

<bean id="dataPointUserDAO-mysql" class="org.scada_lts.dao.DataPointUserDAO"/> <bean id="dataPointUserDAO" class="org.scada_lts.factory.DatabaseAwareBeanFactory"> <property name="prefix" value="dataPointUserDAO"/> </bean>

we only use:
<bean id="dataPointUserDAO-mysql" class="org.scada_lts.dao.DataPointUserDAO"/>

then in ApplicationBeans:

public static IDataPointUserDAO getDataPointUserDaoBean() { return getBean("dataPointUserDAO-"+resolveDbKey(), IDataPointUserDAO.class); }

edit: Okay, that's not enough, you would also need to load this context, just like other contexts, and include it in the contexts from ApplicationBeans, without any complicated logic.

Patch with corrections to your approach:
modular_refactor.patch

For this to work, you need to add the bean to the XML context with a specific database:
<bean id="getDatabaseBeans" class="org.scada_lts.web.beans.GetDatabaseBeans"/>

But i am suggest:

<bean id="dataPointUserDAO" class="${scadalts.database.dao.dataPointUserDao}" />

and in env.properties add:
scadalts.database.dao.dataPointUserDao=org.scada_lts.dao.DataPointUserDAO

We avoid complex logic, manual synchronization, loading additional contexts etc., spring takes care of everything, and to add a new library, just include the lib and indicate the path to the dao in env.properties and that's it.

Limraj · 2026-04-13T18:00:37Z

+            ':scadalts-ui:copyDataSourceJsSourceMap',
+            ':scadalts-ui:copyModernWatchListJsSourceMap',
+            ':scadalts-ui:copyPointHierarchyJsSourceMap',
+            ':scadalts-ui:copyViewsJsSourceMap'


What's the rationale? Did it replace the cascading of these tasks?

Limraj · 2026-04-14T05:33:24Z

+                boolean hasTypeOrSettableFilter =
+                        (searchDataPointJson.getDataTypes() != null && !searchDataPointJson.getDataTypes().isEmpty())
+                                || (searchDataPointJson.getSettable() != null);
+                if(!hasTypeOrSettableFilter) {


Just restore this method to its previous state. What's the point of doing this in the code? What about double negations?

Limraj · 2026-04-14T05:52:38Z

 * @author grzegorz bylica Abil'I.T. development team, sdt@abilit.eu
 * 
 */
-@Service


This annotation needs to come back:
org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'viewHierarchyAPI' defined in file [/Users/kamiljarmusik/Documents/dev/apache-tomcat-9.0.75/webapps/Scada-LTS/WEB-INF/classes/org/scada_lts/web/mvc/api/ViewHierarchyAPI.class]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.scada_lts.service.ViewHierarchyService' available: expected at least 1 bean which qualifies as autowire candidate. Dependency annotations: {}

Rollback

…9_Refactor_DAO_layer_for_modular_architecture

Limraj · 2026-04-14T11:46:00Z

    }

-    @Override
    public View findByName(String name) {


Why was the @Override annotation removed?

- Corrected PointEventDetectorDaoWithCache; - Added utils classes: CacheUtils, ObjectsPaginationUtils;

#3249 Refactor DAO layer for modular architecture

d308d8f

- introduce and complete DAO interface contracts across core modules - align DAO implementations, caches and services with interface-based access - replace scattered DAO factory usage with centralized database-aware bean resolution

Patrykb0802 requested review from Limraj and SoftQ as code owners March 12, 2026 11:11

#3249 Refactor DAO layer for modular architecture

b2ffeb2

Limraj reviewed Mar 24, 2026

View reviewed changes

#3249 Refactor DAO layer for modular architecture

6b3e823

Limraj reviewed Apr 13, 2026

View reviewed changes

Limraj reviewed Apr 14, 2026

View reviewed changes

Patrykb0802 added 3 commits April 14, 2026 09:29

#3249 Refactor DAO layer for modular architecture

b305502

Rollback

Merge remote-tracking branch 'origin/release/2.8.1' into feature/#324…

5bc0703

…9_Refactor_DAO_layer_for_modular_architecture

#3249 Refactor DAO layer for modular architecture

e666e40

Limraj reviewed Apr 14, 2026

View reviewed changes

Limraj and others added 2 commits April 14, 2026 16:48

#3249 Refactor DAO layer for modular architecture

11f7a5b

- Corrected PointEventDetectorDaoWithCache; - Added utils classes: CacheUtils, ObjectsPaginationUtils;

#3249 Refactor DAO layer for modular architecture

678ee67


		List<PointValue> getLatestPointValues(int dataPointId, int limit);

		List<PointValue> getLatestPointValues(int dataPointId, int limit, long before);

Conversation

Patrykb0802 commented Mar 12, 2026

Uh oh!

coderabbitai Bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Java Script Mocha Unit Test Results

Uh oh!

github-actions Bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Java JUnit Test Results

Uh oh!

Patrykb0802 commented Mar 24, 2026

Uh oh!

coderabbitai Bot commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Limraj Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Limraj Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Mar 12, 2026 •

edited

Loading

github-actions Bot commented Mar 12, 2026 •

edited

Loading

github-actions Bot commented Mar 12, 2026 •

edited

Loading

Limraj Apr 13, 2026 •

edited

Loading

Limraj Apr 14, 2026 •

edited

Loading