[jdbc] JDBC persistence ignores operator set in FilterQuery

[splatch]

Given that FilterQuery makes it possible to specify additional operator I am convinced that ignoring it is a bug. There is currently no way to tell JDBC persistence to be inclusive.

The generated query always uses > for begin date and < for end date. There are cases where these should be => or <=.

[jlaur]

Probably this is the logic in question:

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

Lines 515 to 528 in cf241f8

	protected String resolveTimeFilter(FilterCriteria filter, ZoneId timeZone) {
	String filterString = "";
	if (filter.getBeginDate() != null) {
	filterString += filterString.isEmpty() ? " WHERE" : " AND";
	filterString += " TIME>'" + JDBC_DATE_FORMAT.format(filter.getBeginDate().withZoneSameInstant(timeZone))
	+ "'";
	}
	if (filter.getEndDate() != null) {
	filterString += filterString.isEmpty() ? " WHERE" : " AND";
	filterString += " TIME<'" + JDBC_DATE_FORMAT.format(filter.getEndDate().withZoneSameInstant(timeZone))
	+ "'";
	}
	return filterString;
	}

and this test:

openhab-addons/bundles/org.openhab.persistence.jdbc/src/test/java/org/openhab/persistence/jdbc/internal/db/JdbcBaseDAOTest.java

Lines 202 to 211 in cf241f8

	@Test
	public void testHistItemFilterQueryProviderWithStartAndEndDateReturnsDeleteQueryWithWhereClauseDescendingOrder() {
	filter.setBeginDate(parseDateTimeString("2022-01-10T15:01:44"));
	filter.setEndDate(parseDateTimeString("2022-01-15T15:01:44"));
	String sql = jdbcBaseDAO.histItemFilterQueryProvider(filter, 0, DB_TABLE_NAME, "TEST", UTC_ZONE_ID);
	assertThat(sql, is("SELECT time, value FROM " + DB_TABLE_NAME + " WHERE TIME>'" //
	+ JdbcBaseDAO.JDBC_DATE_FORMAT.format(filter.getBeginDate()) + "'" //
	+ " AND TIME<'" + JdbcBaseDAO.JDBC_DATE_FORMAT.format(filter.getEndDate()) + "' ORDER BY time DESC"));
	}

[splatch]

Yup, these are places I been patching in order to get boundaries.

[jlaur]

But unfortunately the current implementation seems correct, at least according to these comments in core implementation:
https://github.com/openhab/openhab-core/blob/06c0e905357a2478ef4fdccae3a6fcff14c7a7be/bundles/org.openhab.core.persistence/src/main/java/org/openhab/core/persistence/FilterCriteria.java#L70-L74

These are only comments, though, not official documentation. @cweitkamp - do you know if these comments are to be interpreted literally?

The operator is for comparing state, not dates:
https://www.openhab.org/javadoc/latest/org/openhab/core/persistence/filtercriteria

beginDate and endDate seem to be interpreted differently here:

openhab-addons/bundles/org.openhab.persistence.rrd4j/src/main/java/org/openhab/persistence/rrd4j/internal/RRD4jPersistenceService.java

Lines 314 to 318 in cf241f8

	if (!Double.isNaN(value) && (((ts >= start) && (ts <= end)) || (start == end))) {
	RRD4jItem rrd4jItem = new RRD4jItem(itemName, mapToState(value, item, unit),
	ZonedDateTime.ofInstant(Instant.ofEpochSecond(ts), ZoneId.systemDefault()));
	items.add(rrd4jItem);
	}

And here:

openhab-addons/bundles/org.openhab.persistence.jpa/src/main/java/org/openhab/persistence/jpa/internal/JpaPersistenceService.java

Lines 204 to 215 in cf241f8

	boolean hasBeginDate = false;
	boolean hasEndDate = false;
	String queryString = "SELECT n FROM " + JpaPersistentItem.class.getSimpleName()
	+ " n WHERE n.realName = :itemName";
	if (filter.getBeginDate() != null) {
	queryString += " AND n.timestamp >= :beginDate";
	hasBeginDate = true;
	}
	if (filter.getEndDate() != null) {
	queryString += " AND n.timestamp <= :endDate";
	hasEndDate = true;
	}

And here:

openhab-addons/bundles/org.openhab.persistence.mongodb/src/main/java/org/openhab/persistence/mongodb/internal/MongoDBPersistenceService.java

Lines 370 to 379 in cf241f8

	BasicDBObject dateQueries = new BasicDBObject();
	if (filter.getBeginDate() != null) {
	dateQueries.put("$gte", Date.from(filter.getBeginDate().toInstant()));
	}
	if (filter.getEndDate() != null) {
	dateQueries.put("$lte", Date.from(filter.getEndDate().toInstant()));
	}
	if (!dateQueries.isEmpty()) {
	query.put(FIELD_TIMESTAMP, dateQueries);
	}

So I would agree that we can change to "TIME>=" and "TIME<=" in lines 519 and 524, is that also how you patched it?

If this would be the outcome, I would also open a PR towards core to change those comments.

[jlaur]

@splatch - you could have a look at this draft pull request: #13734.