From 0a14d17d7adb90167ddeea42b09f03823062a410 Mon Sep 17 00:00:00 2001 From: Aurora Lahtela <24460436+AuroraLS3@users.noreply.github.com> Date: Sun, 25 Sep 2022 10:30:13 +0300 Subject: [PATCH] Fixed issues related to MySQL ONLY_FULL_GROUP_BY MySQL breaks GROUP BY syntax of standard SQL, and allows arbitrary columns with aggregate functions. The ONLY_FULL_GROUP_BY stops this, instead raising an error. Since the SQL was originally designed with this mode on, restricting the SQL broke the code in a few places. Adding the extra group by clauses solves the issue without effecting query results. These issues will be caught by MySQLTest in the future, since the issues could be reproduced by enabling ONLY_FULL_GROUP_BY mode. Affects issues: - Fixed #2619 --- .../ExtensionServerTableDataQuery.java | 7 +++-- .../queries/PerServerAggregateQueries.java | 6 ++-- .../queries/ServerAggregateQueries.java | 2 +- .../analysis/ActivityIndexQueries.java | 29 ++++++++++--------- .../analysis/NetworkActivityIndexQueries.java | 20 ++++++------- .../queries/objects/SessionQueries.java | 2 +- .../database/queries/objects/TPSQueries.java | 19 +++++++++--- .../queries/objects/WorldTimesQueries.java | 2 +- .../plan/storage/database/MySQLTest.java | 19 ++++++++++++ .../queries/ActivityIndexQueriesTest.java | 2 +- 10 files changed, 71 insertions(+), 37 deletions(-) diff --git a/Plan/common/src/main/java/com/djrapitops/plan/extension/implementation/storage/queries/ExtensionServerTableDataQuery.java b/Plan/common/src/main/java/com/djrapitops/plan/extension/implementation/storage/queries/ExtensionServerTableDataQuery.java index fc18efc11..21c5419ea 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/extension/implementation/storage/queries/ExtensionServerTableDataQuery.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/extension/implementation/storage/queries/ExtensionServerTableDataQuery.java @@ -79,7 +79,7 @@ public class ExtensionServerTableDataQuery implements Query> fetchPlayerGroups() { String selectLimitedNumberOfPlayerUUIDsByLastSeenDate = SELECT + - UsersTable.USER_UUID + ",MAX(" + SessionsTable.SESSION_END + ") as last_seen" + + UsersTable.USER_UUID + "," + + "MAX(" + SessionsTable.SESSION_END + ") as last_seen" + FROM + SessionsTable.TABLE_NAME + INNER_JOIN + UsersTable.TABLE_NAME + " u on u." + UsersTable.ID + '=' + SessionsTable.TABLE_NAME + '.' + SessionsTable.USER_ID + - GROUP_BY + SessionsTable.TABLE_NAME + '.' + SessionsTable.USER_ID + + GROUP_BY + UsersTable.USER_UUID + ORDER_BY + "last_seen DESC LIMIT ?"; String sql = SELECT + diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/PerServerAggregateQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/PerServerAggregateQueries.java index 04fbf8a3e..2e1eaaca0 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/PerServerAggregateQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/PerServerAggregateQueries.java @@ -57,7 +57,7 @@ public class PerServerAggregateQueries { FROM + SessionsTable.TABLE_NAME + INNER_JOIN + ServerTable.TABLE_NAME + " se on se." + ServerTable.ID + '=' + SessionsTable.TABLE_NAME + '.' + SessionsTable.SERVER_ID + WHERE + SessionsTable.USER_ID + "=" + UsersTable.SELECT_USER_ID + - GROUP_BY + SessionsTable.SERVER_ID; + GROUP_BY + ServerTable.SERVER_UUID; return new QueryStatement<>(sql) { @Override public void prepare(PreparedStatement statement) throws SQLException { @@ -103,7 +103,7 @@ public class PerServerAggregateQueries { FROM + SessionsTable.TABLE_NAME + INNER_JOIN + ServerTable.TABLE_NAME + " se on se." + ServerTable.ID + '=' + SessionsTable.TABLE_NAME + '.' + SessionsTable.SERVER_ID + WHERE + SessionsTable.USER_ID + "=" + UsersTable.SELECT_USER_ID + - GROUP_BY + SessionsTable.SERVER_ID; + GROUP_BY + ServerTable.SERVER_UUID; return getQueryForCountOf(playerUUID, sql, "kill_count"); } @@ -113,7 +113,7 @@ public class PerServerAggregateQueries { FROM + SessionsTable.TABLE_NAME + INNER_JOIN + ServerTable.TABLE_NAME + " se on se." + ServerTable.ID + '=' + SessionsTable.TABLE_NAME + '.' + SessionsTable.SERVER_ID + WHERE + SessionsTable.USER_ID + "=" + UsersTable.SELECT_USER_ID + - GROUP_BY + SessionsTable.SERVER_ID; + GROUP_BY + ServerTable.SERVER_UUID; return getQueryForCountOf(playerUUID, sql, "death_count"); } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/ServerAggregateQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/ServerAggregateQueries.java index 33f5025b9..5743712c1 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/ServerAggregateQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/ServerAggregateQueries.java @@ -89,7 +89,7 @@ public class ServerAggregateQueries { String sql = SELECT + "COUNT(1) as c, " + ServerTable.SERVER_UUID + FROM + UserInfoTable.TABLE_NAME + INNER_JOIN + ServerTable.TABLE_NAME + " s on s." + ServerTable.ID + '=' + UserInfoTable.TABLE_NAME + '.' + UserInfoTable.SERVER_ID + - GROUP_BY + UserInfoTable.SERVER_ID; + GROUP_BY + ServerTable.SERVER_UUID; return new QueryAllStatement<>(sql, 100) { @Override diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/ActivityIndexQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/ActivityIndexQueries.java index 433447f25..a96067cee 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/ActivityIndexQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/ActivityIndexQueries.java @@ -82,8 +82,8 @@ public class ActivityIndexQueries { public static String selectActivityIndexSQL() { String selectActivePlaytimeSQL = SELECT + - "ux." + UserInfoTable.USER_ID + ",COALESCE(active_playtime,0) AS active_playtime" + - FROM + UserInfoTable.TABLE_NAME + " ux" + + "ax_ux." + UserInfoTable.USER_ID + ",COALESCE(active_playtime,0) AS active_playtime" + + FROM + UserInfoTable.TABLE_NAME + " ax_ux" + LEFT_JOIN + '(' + SELECT + SessionsTable.USER_ID + ",SUM(" + SessionsTable.SESSION_END + '-' + SessionsTable.SESSION_START + '-' + SessionsTable.AFK_TIME + ") as active_playtime" + FROM + SessionsTable.TABLE_NAME + @@ -91,17 +91,17 @@ public class ActivityIndexQueries { AND + SessionsTable.SESSION_END + ">=?" + AND + SessionsTable.SESSION_START + "<=?" + GROUP_BY + SessionsTable.USER_ID + - ") sx on sx." + SessionsTable.USER_ID + "=ux." + UserInfoTable.USER_ID; + ") ax_sx on ax_sx." + SessionsTable.USER_ID + "=ax_ux." + UserInfoTable.USER_ID; String selectThreeWeeks = selectActivePlaytimeSQL + UNION_ALL + selectActivePlaytimeSQL + UNION_ALL + selectActivePlaytimeSQL; return SELECT + - "5.0 - 5.0 * AVG(1.0 / (?/2.0 * (q1.active_playtime*1.0/?) +1.0)) as activity_index," + - "u." + UsersTable.ID + " as " + SessionsTable.USER_ID + ',' + - "u." + UsersTable.USER_UUID + - FROM + '(' + selectThreeWeeks + ") q1" + - INNER_JOIN + UsersTable.TABLE_NAME + " u on u." + UsersTable.ID + "=q1." + UserInfoTable.USER_ID + - GROUP_BY + "q1." + UserInfoTable.USER_ID; + "5.0 - 5.0 * AVG(1.0 / (?/2.0 * (ax_q1.active_playtime*1.0/?) +1.0)) as activity_index," + + "ax_u." + UsersTable.ID + " as user_id," + + "ax_u." + UsersTable.USER_UUID + + FROM + '(' + selectThreeWeeks + ") ax_q1" + + INNER_JOIN + UsersTable.TABLE_NAME + " ax_u on ax_u." + UsersTable.ID + "=ax_q1." + UserInfoTable.USER_ID + + GROUP_BY + "ax_u." + UsersTable.ID + ",ax_u." + UsersTable.USER_UUID; } public static void setSelectActivityIndexSQLParameters(PreparedStatement statement, int index, long playtimeThreshold, ServerUUID serverUUID, long date) throws SQLException { @@ -122,13 +122,16 @@ public class ActivityIndexQueries { public static Query fetchActivityGroupCount(long date, ServerUUID serverUUID, long playtimeThreshold, double above, double below) { String selectActivityIndex = selectActivityIndexSQL(); - String selectCount = SELECT + "COUNT(1) as count, COALESCE(activity_index, 0) as activity_index" + + String selectIndexes = SELECT + "COALESCE(activity_index, 0) as activity_index" + FROM + UserInfoTable.TABLE_NAME + " u" + LEFT_JOIN + '(' + selectActivityIndex + ") q2 on q2." + SessionsTable.USER_ID + "=u." + UserInfoTable.USER_ID + WHERE + "u." + UserInfoTable.SERVER_ID + "=" + ServerTable.SELECT_SERVER_ID + - AND + "u." + UserInfoTable.REGISTERED + "<=?" + - AND + "activity_index>=?" + - AND + "activity_index=?" + + AND + "i.activity_index(selectCount) { @Override diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/NetworkActivityIndexQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/NetworkActivityIndexQueries.java index bb14659c9..b7700a319 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/NetworkActivityIndexQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/analysis/NetworkActivityIndexQueries.java @@ -85,10 +85,10 @@ public class NetworkActivityIndexQueries { WHERE + ServerTable.SERVER_UUID + " IN ('" + new TextStringBuilder().appendWithSeparators(onServers, "','") + "')"; String selectActivePlaytimeSQL = SELECT + - "ux." + UsersTable.ID + "," + - "ux." + UsersTable.USER_UUID + "," + + "ax_ux." + UsersTable.ID + "," + + "ax_ux." + UsersTable.USER_UUID + "," + "COALESCE(active_playtime,0) AS active_playtime" + - FROM + UsersTable.TABLE_NAME + " ux" + + FROM + UsersTable.TABLE_NAME + " ax_ux" + LEFT_JOIN + '(' + SELECT + SessionsTable.USER_ID + ",SUM(" + SessionsTable.SESSION_END + '-' + SessionsTable.SESSION_START + '-' + SessionsTable.AFK_TIME + ") as active_playtime" + FROM + SessionsTable.TABLE_NAME + @@ -96,17 +96,17 @@ public class NetworkActivityIndexQueries { AND + SessionsTable.SESSION_START + "<=?" + (onServers.isEmpty() ? "" : AND + SessionsTable.SERVER_ID + " IN (" + selectServerIds + ")") + GROUP_BY + SessionsTable.USER_ID + - ") sx on sx." + SessionsTable.USER_ID + "=ux." + UsersTable.ID; + ") ax_sx on ax_sx." + SessionsTable.USER_ID + "=ax_ux." + UsersTable.ID; String selectThreeWeeks = selectActivePlaytimeSQL + UNION_ALL + selectActivePlaytimeSQL + UNION_ALL + selectActivePlaytimeSQL; return SELECT + - "5.0 - 5.0 * AVG(1.0 / (?/2.0 * (q1.active_playtime*1.0/?) +1.0)) as activity_index," + - "u." + UsersTable.ID + " as " + SessionsTable.USER_ID + ',' + - "u." + UsersTable.USER_UUID + - FROM + '(' + selectThreeWeeks + ") q1" + - INNER_JOIN + UsersTable.TABLE_NAME + " u on u." + UsersTable.ID + "=q1." + UsersTable.ID + - GROUP_BY + "q1." + UsersTable.ID; + "5.0 - 5.0 * AVG(1.0 / (?/2.0 * (ax_q1.active_playtime*1.0/?) +1.0)) as activity_index," + + "ax_u." + UsersTable.ID + " as user_id," + + "ax_u." + UsersTable.USER_UUID + + FROM + '(' + selectThreeWeeks + ") ax_q1" + + INNER_JOIN + UsersTable.TABLE_NAME + " ax_u on ax_u." + UsersTable.ID + "=ax_q1." + UsersTable.ID + + GROUP_BY + "ax_u." + UsersTable.ID + ",ax_u." + UsersTable.USER_UUID; } public static void setSelectActivityIndexSQLParameters(PreparedStatement statement, int index, long playtimeThreshold, long date) throws SQLException { diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/SessionQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/SessionQueries.java index fd05897de..6d408bf96 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/SessionQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/SessionQueries.java @@ -761,7 +761,7 @@ public class SessionQueries { INNER_JOIN + ServerTable.TABLE_NAME + " s on s." + ServerTable.ID + '=' + SessionsTable.TABLE_NAME + '.' + SessionsTable.SERVER_ID + WHERE + SessionsTable.SESSION_END + ">=?" + AND + SessionsTable.SESSION_START + "<=?" + - GROUP_BY + "s." + ServerTable.ID; + GROUP_BY + "s." + ServerTable.ID + ",s." + ServerTable.NAME; return new QueryStatement<>(sql, 100) { @Override public void prepare(PreparedStatement statement) throws SQLException { diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/TPSQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/TPSQueries.java index 8b73fbda1..c328b835d 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/TPSQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/TPSQueries.java @@ -482,28 +482,39 @@ public class TPSQueries { public static Query> fetchLatestServerStartTime(ServerUUID serverUUID, long dataGapThreshold) { String selectPreviousRowNumber = SELECT + "-1+ROW_NUMBER() over (ORDER BY " + DATE + ") AS previous_rn, " + + SERVER_ID + ',' + DATE + " AS d1" + FROM + TABLE_NAME + WHERE + SERVER_ID + '=' + ServerTable.SELECT_SERVER_ID + + GROUP_BY + SERVER_ID + ',' + DATE + ORDER_BY + "d1 DESC"; String selectRowNumber = SELECT + "ROW_NUMBER() over (ORDER BY " + DATE + ") AS rn, " + + SERVER_ID + ',' + DATE + " AS previous_date" + FROM + TABLE_NAME + WHERE + SERVER_ID + '=' + ServerTable.SELECT_SERVER_ID + + GROUP_BY + SERVER_ID + ',' + DATE + ORDER_BY + "previous_date DESC"; - String selectFirstEntryDate = SELECT + "MIN(" + DATE + ") as start_time" + + + String selectFirstEntryDate = SELECT + + "MIN(" + DATE + ") as start_time," + + SERVER_ID + " as server_id" + FROM + TABLE_NAME + - WHERE + SERVER_ID + '=' + ServerTable.SELECT_SERVER_ID; + WHERE + SERVER_ID + '=' + ServerTable.SELECT_SERVER_ID + + GROUP_BY + SERVER_ID; + // Finds the start time since difference between d1 and previous date is a gap, // so d1 is always first entry after a gap in the data. MAX finds the latest. // Union ensures if there are no gaps to use the first date recorded. String selectStartTime = SELECT + - "MAX(d1) AS start_time" + + "MAX(d1) AS start_time," + + "t1." + SERVER_ID + " as server_id" + FROM + "(" + selectPreviousRowNumber + ") t1" + INNER_JOIN + - "(" + selectRowNumber + ") t2 ON t1.previous_rn=t2.rn" + + "(" + selectRowNumber + ") t2 ON t1.previous_rn=t2.rn AND t2." + SERVER_ID + "=t1." + SERVER_ID + WHERE + "d1 - previous_date > ?" + + GROUP_BY + "t1." + SERVER_ID + UNION + selectFirstEntryDate; return new QueryStatement<>(selectStartTime) { diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/WorldTimesQueries.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/WorldTimesQueries.java index 868f8045d..7a3a942a7 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/WorldTimesQueries.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/queries/objects/WorldTimesQueries.java @@ -135,7 +135,7 @@ public class WorldTimesQueries { SELECT_WORLD_TIMES_JOIN_WORLD_NAME + INNER_JOIN + ServerTable.TABLE_NAME + " s on " + WorldTimesTable.TABLE_NAME + '.' + WorldTimesTable.SERVER_ID + "=s." + ServerTable.ID + WHERE + WorldTimesTable.TABLE_NAME + '.' + WorldTimesTable.USER_ID + "=" + UsersTable.SELECT_USER_ID + - GROUP_BY + WORLD_COLUMN + ',' + WorldTimesTable.TABLE_NAME + '.' + WorldTimesTable.SERVER_ID; + GROUP_BY + WORLD_COLUMN + ",s." + ServerTable.SERVER_UUID; return new QueryStatement<>(sql, 1000) { @Override diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/MySQLTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/MySQLTest.java index 614bd5113..17a5265f3 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/MySQLTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/MySQLTest.java @@ -24,8 +24,10 @@ import com.djrapitops.plan.identification.ServerInfo; import com.djrapitops.plan.identification.ServerUUID; import com.djrapitops.plan.settings.config.PlanConfig; import com.djrapitops.plan.storage.database.queries.ExtensionsDatabaseTest; +import com.djrapitops.plan.storage.database.queries.QueryAllStatement; import com.djrapitops.plan.storage.database.queries.filter.QueryFilters; import com.djrapitops.plan.storage.database.transactions.StoreServerInformationTransaction; +import com.djrapitops.plan.storage.database.transactions.Transaction; import com.djrapitops.plan.storage.database.transactions.commands.RemoveEverythingTransaction; import com.djrapitops.plan.storage.database.transactions.init.CreateTablesTransaction; import org.junit.jupiter.api.AfterAll; @@ -42,6 +44,8 @@ import utilities.TestConstants; import utilities.TestErrorLogger; import java.nio.file.Path; +import java.sql.ResultSet; +import java.sql.SQLException; import java.util.Optional; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -77,6 +81,21 @@ class MySQLTest implements DatabaseTest, DatabaseTestAggregate { Assumptions.assumeTrue(mysql.isPresent()); database = mysql.get(); database.executeTransaction(new CreateTablesTransaction()); + // Enables more strict query mode to prevent errors from it going unnoticed. + database.executeTransaction(new Transaction() { + @Override + protected void performOperations() { + String currentSQLMode = query(new QueryAllStatement<>("SELECT @@GLOBAL.sql_mode as mode") { + @Override + public String processResults(ResultSet set) throws SQLException { + return set.next() ? set.getString("mode") : null; + } + }); + if (!currentSQLMode.contains("ONLY_FULL_GROUP_BY")) { + execute("SET GLOBAL sql_mode=(SELECT CONCAT(@@GLOBAL.sql_mode, ',ONLY_FULL_GROUP_BY'))"); + } + } + }); } @BeforeEach diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/ActivityIndexQueriesTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/ActivityIndexQueriesTest.java index 8d5ecd4be..3367b83be 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/ActivityIndexQueriesTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/ActivityIndexQueriesTest.java @@ -247,7 +247,7 @@ public interface ActivityIndexQueriesTest extends DatabaseTestPreparer { assertNotNull(result); } - @RepeatedTest(25) + @RepeatedTest(5) default void countRegularPlayers() { storeSessions(session -> true); long playtimeThreshold = TimeUnit.MILLISECONDS.toMillis(1L);