From 81d862cc4ae80bcb40452016bd8ebe890e282397 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 28 Oct 2024 10:19:56 -0700 Subject: [PATCH 1/4] Junit test for timestamps on French SQL Server --- .../labkey/api/data/SqlExecutingSelector.java | 7 +- .../org/labkey/api/data/TableSelector.java | 11 +- .../mssql/MicrosoftSqlServer2016Dialect.java | 167 +++++++++++++++++- 3 files changed, 170 insertions(+), 15 deletions(-) diff --git a/api/src/org/labkey/api/data/SqlExecutingSelector.java b/api/src/org/labkey/api/data/SqlExecutingSelector.java index be54655089c..d0c02d5f280 100644 --- a/api/src/org/labkey/api/data/SqlExecutingSelector.java +++ b/api/src/org/labkey/api/data/SqlExecutingSelector.java @@ -56,12 +56,7 @@ public abstract class SqlExecutingSelector columns, @Nullable Filter filter, @Nullable Sort sort, boolean stableColumnOrdering) + protected TableSelector(@NotNull TableInfo table, @Nullable Connection conn, Collection columns, @Nullable Filter filter, @Nullable Sort sort, boolean stableColumnOrdering) { - super(table.getSchema().getScope()); + super(table.getSchema().getScope(), conn); _table = Objects.requireNonNull(table); _columns = columns; _filter = filter; @@ -81,7 +82,7 @@ rely on column order (we return the values from the first one). */ public TableSelector(@NotNull TableInfo table, Collection columns, @Nullable Filter filter, @Nullable Sort sort) { - this(table, columns, filter, sort, isStableOrdered(columns)); + this(table, null, columns, filter, sort, isStableOrdered(columns)); } // Select all columns from a table, with no filter or sort @@ -111,13 +112,13 @@ public TableSelector(@NotNull TableInfo table, @Nullable Filter filter, @Nullabl */ public TableSelector(@NotNull TableInfo table, Set columnNames, @Nullable Filter filter, @Nullable Sort sort) { - this(table, columnInfosList(table, columnNames), filter, sort, isStableOrdered(columnNames)); + this(table, null, columnInfosList(table, columnNames), filter, sort, isStableOrdered(columnNames)); } // Select a single column public TableSelector(@NotNull ColumnInfo column, @Nullable Filter filter, @Nullable Sort sort) { - this(column.getParentTable(), Collections.singleton(column), filter, sort, true); // Single column is stable ordered + this(column.getParentTable(), null, Collections.singleton(column), filter, sort, true); // Single column is stable ordered } // Select a single column from all rows diff --git a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java index b28bcdc4302..596e9d9b471 100644 --- a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java +++ b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java @@ -15,16 +15,30 @@ */ package org.labkey.bigiron.mssql; +import jakarta.servlet.ServletException; import org.apache.commons.lang3.time.FastDateFormat; import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.junit.Assert; import org.junit.Test; +import org.labkey.api.data.ColumnInfo; +import org.labkey.api.data.CompareType; import org.labkey.api.data.ConnectionWrapper; +import org.labkey.api.data.CoreSchema; import org.labkey.api.data.DbScope; +import org.labkey.api.data.Filter; +import org.labkey.api.data.SQLFragment; +import org.labkey.api.data.SimpleFilter; +import org.labkey.api.data.Sort; +import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.SqlSelector; +import org.labkey.api.data.TableInfo; +import org.labkey.api.data.TableSelector; import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.data.dialect.StatementWrapper; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.query.FieldKey; import org.labkey.api.util.logging.LogHelper; import java.sql.CallableStatement; @@ -35,8 +49,10 @@ import java.sql.Timestamp; import java.sql.Types; import java.util.Calendar; +import java.util.Collections; import java.util.Date; -import java.util.Map; +import java.util.GregorianCalendar; +import java.util.Set; public class MicrosoftSqlServer2016Dialect extends MicrosoftSqlServer2014Dialect { @@ -51,9 +67,16 @@ public void prepare(DbScope scope) { super.prepare(scope); - Map map = new SqlSelector(scope, "SELECT language, date_format FROM sys.dm_exec_sessions WHERE session_id = @@spid").getMap(); - _language = (String) map.get("language"); - _dateFormat = (String) map.get("date_format"); + try + { + LanguageSettings settings = getLanguageSettings(scope, scope.getConnection()); + _language = settings.getLanguage(); + _dateFormat = settings.getDate_format(); + } + catch (SQLException e) + { + throw new RuntimeException(e); + } // This seems to be the only string format acceptable for sending Timestamps, but unfortunately it's ambiguous; // SQL Server interprets the "MM-dd" portion based on the database's regional settings. So we must query the @@ -70,6 +93,48 @@ public void prepare(DbScope scope) LOG.info("\n Language: {}\n DateFormat: {}", _language, _dateFormat); } + // TODO: Turn this into a record on 24.11 (24.7 SqlSelector doesn't support records) + public static class LanguageSettings + { + String _language; + String _date_format; + + public String getLanguage() + { + return _language; + } + + public void setLanguage(String language) + { + _language = language; + } + + public String getDate_format() + { + return _date_format; + } + + public void setDate_format(String date_format) + { + _date_format = date_format; + } + + @Override + public String toString() + { + return "LanguageSettings{" + + "_language='" + _language + '\'' + + ", _date_format='" + _date_format + '\'' + + '}'; + } + } + + private static LanguageSettings getLanguageSettings(DbScope scope, Connection conn) + { + return new SqlSelector(scope, conn, "SELECT language, date_format FROM sys.dm_exec_sessions WHERE session_id = @@spid") + .getObject(LanguageSettings.class); + } + @Override public StatementWrapper getStatementWrapper(ConnectionWrapper conn, Statement stmt) { @@ -274,5 +339,99 @@ private void test(TimestampStatementWrapper wrapper, String expected, String tes Timestamp ts = Timestamp.valueOf(test); Assert.assertEquals(expected, wrapper.convert(ts)); } + + @Test + public void testCompareClauses() throws SQLException, ServletException + { + // Issue 51472 pointed out issues with Timestamp conversions on French SQL Server. Primary fixes were in + // the DateCompareClause subclasses, so put them through their paces here. + + DbScope labKeyScope = DbScope.getLabKeyScope(); + // Clone the LabKey scope so it has its own SqlDialect that we can prepare every time we set the language + DbScope scope = new DbScope(labKeyScope.getDataSourceName(), labKeyScope.getLabKeyDataSource()) + { + private final Connection _connection = getWrapped(); + + @Override + public Connection getConnection() + { + // Hand out an un-pooled connection since we might set language and don't want that to persist outside this test + return _connection; + } + + private Connection getWrapped() throws SQLException + { + return new ConnectionWrapper(getUnpooledConnection(), this, null, DbScope.ConnectionType.Transaction, null); + } + }; + + TableInfo containers = CoreSchema.getInstance().getTableInfoContainers(); + ColumnInfo created = containers.getColumn("Created"); + + try (Connection conn = scope.getConnection()) + { + setLanguage(scope, conn, "English"); + testMultipleFilters(conn, containers, created.getFieldKey()); + + if (scope.getSqlDialect().isSqlServer()) + { + setLanguage(scope, conn, "French"); + testMultipleFilters(conn, containers, created.getFieldKey()); + } + } + } + + private void setLanguage(DbScope scope, Connection conn, String language) + { + SqlDialect dialect = scope.getSqlDialect(); + if (dialect.isSqlServer()) + { + new SqlExecutor(scope, conn).execute("SET LANGUAGE " + language); + dialect.prepare(scope); + LOG.info(getLanguageSettings(DbScope.getLabKeyScope(), conn)); + } + } + + private void testMultipleFilters(Connection conn, TableInfo table, FieldKey date) + { + Calendar cal = new GregorianCalendar(); + cal.add(Calendar.DATE, -30); + Date startDate = cal.getTime(); + + testFilter(conn, table, date, startDate, CompareType.DATE_EQUAL); + testFilter(conn, table, date, startDate, CompareType.DATE_NOT_EQUAL); + testFilter(conn, table, date, startDate, CompareType.DATE_GTE); + testFilter(conn, table, date, startDate, CompareType.DATE_GT); + testFilter(conn, table, date, startDate, CompareType.DATE_LT); + testFilter(conn, table, date, startDate, CompareType.DATE_LTE); + } + + // We don't care about the row counts, just that each query executes without any exceptions + private void testFilter(Connection conn, TableInfo table, FieldKey fk, Object value, CompareType type) + { + SimpleFilter filter = new SimpleFilter(fk, value, type); + + new TestTableSelector(table, conn, Collections.singleton(table.getColumn(fk)), filter, null).getRowCount(); + + // This mimics the query that UserManager.getActiveDaysCount() generates + SQLFragment sql = new SQLFragment("SELECT * FROM (SELECT CAST(") + .append(fk.getName()) + .append(" AS DATE) AS ") + .append(fk.getName()) + .append(" FROM ") + .append(table.getSelectName()) + .append(") x ") + .append(filter.getSQLFragment(table.getSqlDialect())); + + new SqlSelector(table.getSchema().getScope(), conn, sql).getRowCount(); + } + + private static class TestTableSelector extends TableSelector + { + public TestTableSelector(@NotNull TableInfo table, @NotNull Connection conn, Set columns, @Nullable Filter filter, @Nullable Sort sort) + { + super(table, conn, columns, filter, sort, true); + } + } } } From b3479cc895119b761b6a10a394688277b3c04441 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 28 Oct 2024 13:20:11 -0700 Subject: [PATCH 2/4] Shift exception type --- .../labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java index 596e9d9b471..029d5838772 100644 --- a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java +++ b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java @@ -28,6 +28,7 @@ import org.labkey.api.data.CoreSchema; import org.labkey.api.data.DbScope; import org.labkey.api.data.Filter; +import org.labkey.api.data.RuntimeSQLException; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Sort; @@ -75,7 +76,7 @@ public void prepare(DbScope scope) } catch (SQLException e) { - throw new RuntimeException(e); + throw new RuntimeSQLException(e); } // This seems to be the only string format acceptable for sending Timestamps, but unfortunately it's ambiguous; From fa19ff971ba082740d754b792b1609b7bf9c0a95 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 29 Oct 2024 06:59:15 -0700 Subject: [PATCH 3/4] Clear out connection to defeat memtracker --- .../mssql/MicrosoftSqlServer2016Dialect.java | 47 ++++++++++++------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java index 029d5838772..4426dde0098 100644 --- a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java +++ b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java @@ -349,22 +349,7 @@ public void testCompareClauses() throws SQLException, ServletException DbScope labKeyScope = DbScope.getLabKeyScope(); // Clone the LabKey scope so it has its own SqlDialect that we can prepare every time we set the language - DbScope scope = new DbScope(labKeyScope.getDataSourceName(), labKeyScope.getLabKeyDataSource()) - { - private final Connection _connection = getWrapped(); - - @Override - public Connection getConnection() - { - // Hand out an un-pooled connection since we might set language and don't want that to persist outside this test - return _connection; - } - - private Connection getWrapped() throws SQLException - { - return new ConnectionWrapper(getUnpooledConnection(), this, null, DbScope.ConnectionType.Transaction, null); - } - }; + TestScope scope = new TestScope(labKeyScope); TableInfo containers = CoreSchema.getInstance().getTableInfoContainers(); ColumnInfo created = containers.getColumn("Created"); @@ -380,6 +365,36 @@ private Connection getWrapped() throws SQLException testMultipleFilters(conn, containers, created.getFieldKey()); } } + + // Null out connection to prevent query profiler from holding onto it via this scope + scope.clearConnection(); + } + + private static class TestScope extends DbScope + { + private Connection _connection = getWrapped(); + + public TestScope(DbScope scope) throws ServletException, SQLException + { + super(scope.getDataSourceName(), scope.getLabKeyDataSource()); + } + + @Override + public Connection getConnection() + { + return _connection; + } + + private Connection getWrapped() throws SQLException + { + // Hand out an un-pooled connection since we might set language and don't want that to persist outside this test + return new ConnectionWrapper(getUnpooledConnection(), this, null, DbScope.ConnectionType.Transaction, null); + } + + private void clearConnection() + { + _connection = null; + } } private void setLanguage(DbScope scope, Connection conn, String language) From 372e708a88f04ede8ef9ef97761c6f3080b238e6 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 29 Oct 2024 11:36:44 -0700 Subject: [PATCH 4/4] Address some unclosed connections --- .../mssql/MicrosoftSqlServer2016Dialect.java | 65 ++++++++++++------- 1 file changed, 42 insertions(+), 23 deletions(-) diff --git a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java index 4426dde0098..bf24250cd81 100644 --- a/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java +++ b/bigiron/src/org/labkey/bigiron/mssql/MicrosoftSqlServer2016Dialect.java @@ -68,9 +68,9 @@ public void prepare(DbScope scope) { super.prepare(scope); - try + try (Connection conn = scope.getConnection()) { - LanguageSettings settings = getLanguageSettings(scope, scope.getConnection()); + LanguageSettings settings = getLanguageSettings(scope, conn); _language = settings.getLanguage(); _dateFormat = settings.getDate_format(); } @@ -347,32 +347,30 @@ public void testCompareClauses() throws SQLException, ServletException // Issue 51472 pointed out issues with Timestamp conversions on French SQL Server. Primary fixes were in // the DateCompareClause subclasses, so put them through their paces here. - DbScope labKeyScope = DbScope.getLabKeyScope(); - // Clone the LabKey scope so it has its own SqlDialect that we can prepare every time we set the language - TestScope scope = new TestScope(labKeyScope); - - TableInfo containers = CoreSchema.getInstance().getTableInfoContainers(); - ColumnInfo created = containers.getColumn("Created"); - - try (Connection conn = scope.getConnection()) + // Use a test scope that passes out an un-pooled connection so changing the language settings don't affect + // connections in the pool. This also gives us a SqlDialect we can prepare every time we set the language. + try (TestScope scope = new TestScope(DbScope.getLabKeyScope())) { - setLanguage(scope, conn, "English"); - testMultipleFilters(conn, containers, created.getFieldKey()); + TableInfo containers = CoreSchema.getInstance().getTableInfoContainers(); + ColumnInfo created = containers.getColumn("Created"); - if (scope.getSqlDialect().isSqlServer()) + try (Connection conn = scope.getConnection()) { - setLanguage(scope, conn, "French"); + setLanguage(scope, conn, "English"); testMultipleFilters(conn, containers, created.getFieldKey()); + + if (scope.getSqlDialect().isSqlServer()) + { + setLanguage(scope, conn, "French"); + testMultipleFilters(conn, containers, created.getFieldKey()); + } } } - - // Null out connection to prevent query profiler from holding onto it via this scope - scope.clearConnection(); } - private static class TestScope extends DbScope + private static class TestScope extends DbScope implements AutoCloseable { - private Connection _connection = getWrapped(); + private TestConnectionWrapper _connection = getWrapped(); public TestScope(DbScope scope) throws ServletException, SQLException { @@ -385,16 +383,37 @@ public Connection getConnection() return _connection; } - private Connection getWrapped() throws SQLException + private TestConnectionWrapper getWrapped() throws SQLException { // Hand out an un-pooled connection since we might set language and don't want that to persist outside this test - return new ConnectionWrapper(getUnpooledConnection(), this, null, DbScope.ConnectionType.Transaction, null); + return new TestConnectionWrapper(getUnpooledConnection(), this); } - private void clearConnection() + @Override + public void close() throws SQLException { + _connection.closeConnection(); _connection = null; } + + private static class TestConnectionWrapper extends ConnectionWrapper + { + public TestConnectionWrapper(Connection conn, DbScope scope) + { + super(conn, scope, null, DbScope.ConnectionType.Transaction, null); + } + + @Override + public void close() + { + // No-op + } + + private void closeConnection() throws SQLException + { + super.close(); + } + } } private void setLanguage(DbScope scope, Connection conn, String language) @@ -404,7 +423,7 @@ private void setLanguage(DbScope scope, Connection conn, String language) { new SqlExecutor(scope, conn).execute("SET LANGUAGE " + language); dialect.prepare(scope); - LOG.info(getLanguageSettings(DbScope.getLabKeyScope(), conn)); + LOG.info(getLanguageSettings(scope, conn)); } }