[jdbc] Add support for case sensitive table names reflecting item names 1:1

[jlaur]

Add support for case sensitive tables named 1:1 by corresponding items, eliminating the need for an items management/mapping table.

Additionally:

• Fix migration for MySQL when Multiple Statement Execution Support is not configured/supported. See below.
• Fix migration when tableNamePrefix is changed. All migration paths are supported now.
• Fix migration stopping on first unknown item.
• Minor refactoring for readability (in context of changes) which also happened to remove some checkstyle warnings.

Fixes #11906

MySQL and Multiple Statement Execution Support

When Multiple Statement Execution Support is not configured/supported, the following exception would be thrown when migrating tables:

2022-10-16 14:56:32.114 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::query queryString = ALTER TABLE Item1 RENAME TO newitem;ALTER TABLE Item2 RENAME TO another_item;
2022-10-16 14:56:32.115 [ERROR] [org.knowm.yank.Yank                 ] - Error in SQL query!!!
java.sql.SQLException: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ALTER TABLE Item2 RENAME TO another_item' at line 1 Query: ALTER TABLE Item1 RENAME TO newitem;ALTER TABLE Item2 RENAME TO another_item; Parameters: []
	at org.apache.commons.dbutils.AbstractQueryRunner.rethrow(AbstractQueryRunner.java:392) ~[bundleFile:?]
	at org.apache.commons.dbutils.QueryRunner.update(QueryRunner.java:491) ~[bundleFile:?]
	at org.apache.commons.dbutils.QueryRunner.update(QueryRunner.java:457) ~[bundleFile:?]
	at org.knowm.yank.Yank.execute(Yank.java:194) [bundleFile:?]
	at org.knowm.yank.Yank.execute(Yank.java:177) [bundleFile:?]
	at org.openhab.persistence.jdbc.db.JdbcBaseDAO.doUpdateItemTableNames(JdbcBaseDAO.java:311) [bundleFile:?]
	at org.openhab.persistence.jdbc.internal.JdbcMapper.updateItemTableNames(JdbcMapper.java:148) [bundleFile:?]
	at org.openhab.persistence.jdbc.internal.JdbcMapper.formatTableNames(JdbcMapper.java:343) [bundleFile:?]
	at org.openhab.persistence.jdbc.internal.JdbcMapper.checkDBSchema(JdbcMapper.java:257) [bundleFile:?]
	at org.openhab.persistence.jdbc.internal.JdbcPersistenceService.updateConfig(JdbcPersistenceService.java:235) [bundleFile:?]
	at org.openhab.persistence.jdbc.internal.JdbcPersistenceService.activate(JdbcPersistenceService.java:89) [bundleFile:?]

References

• MySQL: Identifier Case Sensitivity

Testing

I have manually tested most combinations of migration paths:

S = Source, T = Target
Prefix = tableNamePrefix, Real = tableUseRealItemNames, Case = tableCaseSensitiveItemNames, Digit = tableIdDigitCount

S:Prefix	S:Real	S:Case	S:Digit	S:Example	T:Prefix	T:Real	T:Case	T:Digit	T:Result
Item	false	false	4	Item0001	item	false	false	4	item0001
Item	false	false	4	Item0001	Item	false	false	3	Item001
Item	false	false	4	Item0001	Item	true	false	4	myitem_0001
Item	false	false	4	Item0001	Item	true	true	4	MyItem
Item	true	false	4	myitem_0001	Item	true	true	4	MyItem
Item	true	false	4	myitem_0001	Item	false	false	4	Item0001
Item	true	true	4	MyItem	Item	false	false	4	Item0001
Item	true	true	4	MyItem	Item	true	false	4	myitem_0001

All of the above on a test database both with a copy of my production schema consisting of more than 200 tables, as well as small dedicated test scenarios with tricky naming etc.

Additionally I have performed tests of creating new items and tables with different configurations.

All tests have been performed only in MySQL. A forum user has tested with MariaDB.

JAR for testing: https://drive.google.com/file/d/1_CmJtYa2pwrN_LlrWu1H75muOW6seam7/view?usp=sharing

Note: Place the corresponding database driver into the addons directory. For example, download the MySQL connector and rename it to mysql-connector-java.jar.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/jdbc-persistence-with-tableuserealitemnames-and-tableiddigitcount-0/40552/8

[cweitkamp]

I am looking forward to this fix. Currently I have the slight feeling that it will break a lot of users persistence when they are already using the tableUseRealItemNames=true configuration but still having table names with number prefixes. The new implementation creates name without prefix and probably creates a new table with that name because it is not able to match it to the existing table. Do you think we can set up a cleaning script to rename existing tables and their associated entries in the items table?

Furthermore I would like to question if the mentioned items table is needed anymore when setting this configuration to true.

[jlaur]

I am looking forward to this fix. Currently I have the slight feeling that it will break a lot of users persistence when they are already using the tableUseRealItemNames=true configuration but still having table names with number prefixes. The new implementation creates name without prefix and probably creates a new table with that name because it is not able to match it to the existing table.

Good point, this needs to be verified (I have not started any testing yet):

openhab-addons/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/JdbcMapper.java

Lines 320 to 377 in 37a91ab

	private void formatTableNames() {
	boolean tmpinit = initialized;
	if (tmpinit) {
	initialized = false;
	}
	Map<Integer, String> tableIds = new HashMap<>();
	//
	for (ItemsVO vo : getItemIDTableNames()) {
	String t = getTableName(vo.getItemid(), vo.getItemname());
	sqlTables.put(vo.getItemname(), t);
	tableIds.put(vo.getItemid(), t);
	}
	//
	List<ItemsVO> al = getItemTables();
	String oldName = "";
	String newName = "";
	List<ItemVO> oldNewTablenames = new ArrayList<>();
	for (int i = 0; i < al.size(); i++) {
	int id = -1;
	oldName = al.get(i).getTable_name();
	logger.info("JDBC::formatTableNames: found Table Name= {}", oldName);
	if (oldName.startsWith(conf.getTableNamePrefix()) && !oldName.contains("_")) {
	id = Integer.parseInt(oldName.substring(conf.getTableNamePrefix().length()));
	logger.info("JDBC::formatTableNames: found Table with Prefix '{}' Name= {} id= {}",
	conf.getTableNamePrefix(), oldName, (id));
	} else if (oldName.contains("_")) {
	id = Integer.parseInt(oldName.substring(oldName.lastIndexOf("_") + 1));
	logger.info("JDBC::formatTableNames: found Table Name= {} id= {}", oldName, (id));
	}
	logger.info("JDBC::formatTableNames: found Table id= {}", id);
	newName = tableIds.get(id);
	logger.info("JDBC::formatTableNames: found Table newName= {}", newName);
	if (newName != null) {
	if (!oldName.equalsIgnoreCase(newName)) {
	oldNewTablenames.add(new ItemVO(oldName, newName));
	logger.info("JDBC::formatTableNames: Table '{}' will be renamed to '{}'", oldName, newName);
	} else {
	logger.info("JDBC::formatTableNames: Table oldName='{}' newName='{}' nothing to rename", oldName,
	newName);
	}
	} else {
	logger.error("JDBC::formatTableNames: Table '{}' could NOT be renamed to '{}'", oldName, newName);
	break;
	}
	}
	updateItemTableNames(oldNewTablenames);
	logger.info("JDBC::formatTableNames: Finished updating {} item table names", oldNewTablenames.size());
	initialized = tmpinit;
	}

Do you think we can set up a cleaning script to rename existing tables and their associated entries in the items table?

This should be possible by using the existing configuration option rebuildTableNames:
https://www.openhab.org/addons/persistence/jdbc/#configuration

But now it needs to support also migration from old to new behavior.

Furthermore I would like to question if the mentioned items table is needed anymore when setting this configuration to true.

Agreed, by principle it shouldn't be needed, although it might require some additional effort to get rid of. Also, case sensitivity would need to be checked - I believe I saw some example in the forum thread with lower case table names while having camel case item names.

[jlaur]

@cweitkamp - as I've been working my way through this, getting more acquainted with the code and also started testing things, I have now realized a few things and could use some feedback.

Even though current implementation of tableUseRealItemNames is inconsistent with the documentation, you have a valid point that users might be using this, so we need backwards compatibility. Therefore I would suggest to fix the documentation and keep current behavior when using just tableUseRealItemNames. In the latest commit I introduced tablePreserveCase, but now I would suggest to change/simplify this so we don't have many strange combinations of options, but instead just this:

• tableUseRealItemNames = true, tableCaseSensitiveDirectMapping = false: Backwards compatible behavior, e.g. myitem_0001.
• tableUseRealItemNames = true, tableCaseSensitiveDirectMapping = true: Number suffix is omitted and item name is case sensitive, e.g. MyItem.

A better name for the new option would be needed. :-) The thinking is that without case sensitive naming of tables, we risk collisions since item names in openHAB are case sensitive, i.e. you can have both MyItem and myItem. So to really solve that, we would need a suffix of some sort anyway. Case sensitive table names can depend on database, OS, filesystem, database configuration and other things, but IMHO this would be required for this fix to make any sense. So either users can choose current behavior with number suffix, or if supported by their database, they can use direct mapping from item name to table name.

Now, the only problem left could be difference in rules between item naming and table naming. I didn't mange to find the regex for item names yet, but from the UI at least it seems to be quite restrictive, so I'm hoping this won't be a problem. If an item could be named my-item, for example, this would be stripped to myitem, and then we would again risk collisions.

I have provided unit tests documenting the issues and migration paths. Next, if we agree on some sensible approach, I would then adapt the code, tests and documentation accordingly, which shouldn't be much additional work at this point. I would also be prepared to drop the items table entirely when running in "new mode".

[jlaur]

Now found item name regex:
/~https://github.com/openhab/openhab-core/blob/792b7511bf9bb830ec48b8f81bb0437f1b7ce867/bundles/org.openhab.core/src/main/java/org/openhab/core/items/ItemUtil.java#L56-L58

As it is more restrictive than the current regex for table names, case sensitive table names will be safe from collisions.

[jlaur]

@cweitkamp - done. Backwards compatibility ensured and all migration paths supported.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/jdbc-persistence-with-tableuserealitemnames-and-tableiddigitcount-0/40552/11

[jlaur]

@openhab/add-ons-maintainers - I'm not sure whether this should be labelled as "bug" or "enhancement". For sure the documentation was misleading. In either case, would anyone be up for reviewing this? It would be really nice to finalize for the next milestone, so there would still be time for user feedback before the release.

tableUseRealItemNames

When set to true, real item names are used for table names

True, they were used, but lowercased and with a number suffix added (MyItem -> myitem_0001).

and tableNamePrefix is ignored.

True.

When set to false, the tableNamePrefix is used to generate table names with sequential numbers.

True. However, without using tableNamePrefix, there would also be sequential numbers.

tableIdDigitCount

when tableUseRealItemNames is false and thus table names are generated sequentially,

(which was also the case when tableUseRealItemNames was true)

this controls how many zero-padded digits are used in the table name.

True. But there would still be sequential numbers with padding according to this setting with tableUseRealItemNames being true.

[jlaur]

@cweitkamp - do you think you could find time to have a look at this?

[lolodomo]

@jlaur : having a look at the global result of the static code analysis (all bindings), I found that one. Maybe you could check,.

[jlaur]

@jlaur : having a look at the global result of the static code analysis (all bindings), I found that one. Maybe you could check,.

I already checked this in context of #13429, but I'm not sure what to do with it:

openhab-addons/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/dto/ItemsVO.java

Lines 97 to 104 in 813489c

	@Override
	public int hashCode() {
	final int prime = 31;
	int result = 1;
	result = prime * result + ((itemname == null) ? 0 : itemname.hashCode());
	result = prime * result + (itemid ^ (itemid >>> 32));
	return result;
	}

It looks like it was generated by Eclipse:
https://www.baeldung.com/java-hashcode

It will work according to implementation requirements, but of course it would be nice to get rid of this checkstyle warning.

[jlaur]

@lolodomo - would you be able to find time for reviewing this PR before M4? I have time the next few evenings for making corrections during review.

[lolodomo]

would you be able to find time for reviewing this PR before M4?

Probably only a limited time until next weekend.

[lolodomo]

I have not reviewed in details but looks good to me.
Just one question.

[lolodomo]

LGTM, thank you

[jlaur]

@jlaur : having a look at the global result of the static code analysis (all bindings), I found that one. Maybe you could check,.

I already checked this in context of #13429, but I'm not sure what to do with it:

openhab-addons/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/dto/ItemsVO.java

Lines 97 to 104 in 813489c

	@Override
	public int hashCode() {
	final int prime = 31;
	int result = 1;
	result = prime * result + ((itemname == null) ? 0 : itemname.hashCode());
	result = prime * result + (itemid ^ (itemid >>> 32));
	return result;
	}

It looks like it was generated by Eclipse: https://www.baeldung.com/java-hashcode

It will work according to implementation requirements, but of course it would be nice to get rid of this checkstyle warning.

@lolodomo - this is now addressed by commit 18892b8 in #13719.