From 8bbbde4556a1604771db0af68ff9fefa92e0fc93 Mon Sep 17 00:00:00 2001 From: Aurora Lahtela <24460436+AuroraLS3@users.noreply.github.com> Date: Sat, 14 May 2022 11:00:34 +0300 Subject: [PATCH] Fix registering plan_user row if missing when required There was one error code for MySQL that used wrong message context to detect the missing user_id properly. Wrote tests for all the extra functionality ensuring no exceptions occur. Affects issues: - Fixed #2361 - Fixed #2343 --- .../delivery/domain/PlayerIdentifier.java | 4 +- .../exceptions/database/DBOpException.java | 2 +- .../plan/storage/database/SQLDB.java | 1 + .../database/sql/tables/UsersTable.java | 3 +- .../ExtensionTableRowValueLengthPatch.java | 5 -- .../database/transactions/patches/Patch.java | 4 + .../patches/UsersTableNameLengthPatch.java | 34 +++++++ .../queries/GeolocationQueriesTest.java | 28 ++++++ .../database/queries/PingQueriesTest.java | 37 +++++++- .../database/queries/SessionQueriesTest.java | 88 ++++++++++++++++++- 10 files changed, 189 insertions(+), 17 deletions(-) create mode 100644 Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/UsersTableNameLengthPatch.java diff --git a/Plan/common/src/main/java/com/djrapitops/plan/delivery/domain/PlayerIdentifier.java b/Plan/common/src/main/java/com/djrapitops/plan/delivery/domain/PlayerIdentifier.java index 2848bbd937..664ce15e4c 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/delivery/domain/PlayerIdentifier.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/delivery/domain/PlayerIdentifier.java @@ -37,7 +37,7 @@ public String getName() { } public boolean isSame(PlayerIdentifier that) { - return Objects.equals(getUuid(), that.getUuid()) && Objects.equals(getName(), that.getName()); + return Objects.equals(this, that); } @Override @@ -45,7 +45,7 @@ public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; PlayerIdentifier that = (PlayerIdentifier) o; - return Objects.equals(getUuid(), that.getUuid()) && Objects.equals(getName(), that.getName()); + return Objects.equals(getUuid(), that.getUuid()); } @Override diff --git a/Plan/common/src/main/java/com/djrapitops/plan/exceptions/database/DBOpException.java b/Plan/common/src/main/java/com/djrapitops/plan/exceptions/database/DBOpException.java index 6225433e45..b31bd8d215 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/exceptions/database/DBOpException.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/exceptions/database/DBOpException.java @@ -132,7 +132,7 @@ public static DBOpException forCause(String sql, SQLException e) { break; case 1048: // MySQL - context.related("Not null constraint violation") + context.related(CONSTRAINT_VIOLATION) .whatToDo("Report this error. NOT NULL constraint violation occurred."); break; default: diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java index 03197118dc..7093c462c9 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/SQLDB.java @@ -225,6 +225,7 @@ Patch[] patches() { new SessionsOptimizationPatch(), new UserInfoHostnameAllowNullPatch(), new RegisterDateMinimizationPatch(), + new UsersTableNameLengthPatch() }; } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/tables/UsersTable.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/tables/UsersTable.java index 82b2b2df88..ab351e4be1 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/tables/UsersTable.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/sql/tables/UsersTable.java @@ -31,6 +31,7 @@ * Patches that apply to this table: * {@link com.djrapitops.plan.storage.database.transactions.patches.Version10Patch} * {@link com.djrapitops.plan.storage.database.transactions.patches.RegisterDateMinimizationPatch} + * {@link com.djrapitops.plan.storage.database.transactions.patches.UsersTableNameLengthPatch} * * @author AuroraLS3 */ @@ -58,7 +59,7 @@ public static String createTableSQL(DBType dbType) { .column(ID, Sql.INT).primaryKey() .column(USER_UUID, Sql.varchar(36)).notNull().unique() .column(REGISTERED, Sql.LONG).notNull() - .column(USER_NAME, Sql.varchar(16)).notNull() + .column(USER_NAME, Sql.varchar(36)).notNull() .column(TIMES_KICKED, Sql.INT).notNull().defaultValue("0") .toString(); } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/ExtensionTableRowValueLengthPatch.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/ExtensionTableRowValueLengthPatch.java index 1c15163b2f..b89dd8e43b 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/ExtensionTableRowValueLengthPatch.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/ExtensionTableRowValueLengthPatch.java @@ -17,7 +17,6 @@ package com.djrapitops.plan.storage.database.transactions.patches; import com.djrapitops.plan.storage.database.DBType; -import com.djrapitops.plan.storage.database.queries.schema.MySQLSchemaQueries; import com.djrapitops.plan.storage.database.sql.building.Sql; import com.djrapitops.plan.storage.database.sql.tables.ExtensionPlayerTableValueTable; import com.djrapitops.plan.storage.database.sql.tables.ExtensionServerTableValueTable; @@ -44,10 +43,6 @@ public boolean hasBeenApplied() { && columnVarcharLength(serverTable, ExtensionServerTableValueTable.VALUE_5) >= 250; } - private int columnVarcharLength(String table, String column) { - return query(MySQLSchemaQueries.columnVarcharLength(table, column)); - } - @Override protected void applyPatch() { increaseLength(playerTable, ExtensionPlayerTableValueTable.VALUE_1); diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java index 35f0c3be4b..61f89f9cfc 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/Patch.java @@ -157,4 +157,8 @@ public Boolean processResults(ResultSet set) throws SQLException { } }); } + + protected int columnVarcharLength(String table, String column) { + return dbType == DBType.SQLITE ? Integer.MAX_VALUE : query(MySQLSchemaQueries.columnVarcharLength(table, column)); + } } diff --git a/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/UsersTableNameLengthPatch.java b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/UsersTableNameLengthPatch.java new file mode 100644 index 0000000000..42483132dc --- /dev/null +++ b/Plan/common/src/main/java/com/djrapitops/plan/storage/database/transactions/patches/UsersTableNameLengthPatch.java @@ -0,0 +1,34 @@ +/* + * This file is part of Player Analytics (Plan). + * + * Plan is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License v3 as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Plan is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Plan. If not, see . + */ +package com.djrapitops.plan.storage.database.transactions.patches; + +import com.djrapitops.plan.storage.database.DBType; +import com.djrapitops.plan.storage.database.sql.building.Sql; +import com.djrapitops.plan.storage.database.sql.tables.UsersTable; + +public class UsersTableNameLengthPatch extends Patch { + @Override + public boolean hasBeenApplied() { + return dbType == DBType.SQLITE || // SQLite does not limit varchar lengths + columnVarcharLength(UsersTable.TABLE_NAME, UsersTable.USER_NAME) >= 36; + } + + @Override + protected void applyPatch() { + execute("ALTER TABLE " + UsersTable.TABLE_NAME + " MODIFY " + UsersTable.USER_NAME + " " + Sql.varchar(36)); + } +} diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/GeolocationQueriesTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/GeolocationQueriesTest.java index 3c8b53ed95..76c213eb88 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/GeolocationQueriesTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/GeolocationQueriesTest.java @@ -17,6 +17,7 @@ package com.djrapitops.plan.storage.database.queries; import com.djrapitops.plan.delivery.domain.DateObj; +import com.djrapitops.plan.gathering.domain.BaseUser; import com.djrapitops.plan.gathering.domain.GeoInfo; import com.djrapitops.plan.gathering.domain.Ping; import com.djrapitops.plan.storage.database.Database; @@ -27,6 +28,7 @@ import com.djrapitops.plan.storage.database.transactions.commands.RemoveEverythingTransaction; import com.djrapitops.plan.storage.database.transactions.events.GeoInfoStoreTransaction; import com.djrapitops.plan.storage.database.transactions.events.PingStoreTransaction; +import com.djrapitops.plan.storage.database.transactions.events.PlayerRegisterTransaction; import com.djrapitops.plan.storage.database.transactions.events.PlayerServerRegisterTransaction; import org.junit.jupiter.api.Test; import utilities.RandomData; @@ -40,6 +42,32 @@ public interface GeolocationQueriesTest extends DatabaseTestPreparer { + @Test + default void geoInfoStoreTransactionOutOfOrderDoesNotFailDueToMissingUser() { + List expected = RandomData.randomGeoInfo(); + for (GeoInfo geoInfo : expected) { + save(playerUUID, geoInfo); + } + + List result = db().query(GeoInfoQueries.fetchAllGeoInformation()).get(playerUUID); + assertEquals(expected, result); + } + + @Test + default void geoInfoStoreTransactionOutOfOrderUpdatesUserInformation() { + List geoInfos = RandomData.randomGeoInfo(); + for (GeoInfo geoInfo : geoInfos) { + save(playerUUID, geoInfo); + } + + long registerDate = RandomData.randomTime(); + db().executeTransaction(new PlayerRegisterTransaction(playerUUID, () -> registerDate, TestConstants.PLAYER_ONE_NAME)); + + Optional expected = Optional.of(new BaseUser(playerUUID, TestConstants.PLAYER_ONE_NAME, registerDate, 0)); + Optional result = db().query(BaseUserQueries.fetchBaseUserOfPlayer(playerUUID)); + assertEquals(expected, result); + } + @Test default void geoInformationIsStored() { db().executeTransaction(new PlayerServerRegisterTransaction(playerUUID, RandomData::randomTime, diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/PingQueriesTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/PingQueriesTest.java index 311ebc49ac..051034e47f 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/PingQueriesTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/PingQueriesTest.java @@ -17,20 +17,20 @@ package com.djrapitops.plan.storage.database.queries; import com.djrapitops.plan.delivery.domain.DateObj; +import com.djrapitops.plan.gathering.domain.BaseUser; import com.djrapitops.plan.gathering.domain.Ping; import com.djrapitops.plan.storage.database.DatabaseTestPreparer; +import com.djrapitops.plan.storage.database.queries.objects.BaseUserQueries; import com.djrapitops.plan.storage.database.queries.objects.PingQueries; import com.djrapitops.plan.storage.database.transactions.commands.RemoveEverythingTransaction; import com.djrapitops.plan.storage.database.transactions.events.PingStoreTransaction; +import com.djrapitops.plan.storage.database.transactions.events.PlayerRegisterTransaction; import com.djrapitops.plan.storage.database.transactions.events.PlayerServerRegisterTransaction; import org.junit.jupiter.api.Test; import utilities.RandomData; import utilities.TestConstants; -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.UUID; +import java.util.*; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -42,6 +42,35 @@ private void prepareForPingStorage() { TestConstants.PLAYER_ONE_NAME, serverUUID(), TestConstants.GET_PLAYER_HOSTNAME)); } + @Test + default void pingStoreTransactionOutOfOrderDoesNotFailDueToMissingUser() { + DateObj saved = RandomData.randomIntDateObject(); + int value = saved.getValue(); + db().executeTransaction(new PingStoreTransaction(playerUUID, serverUUID(), + Collections.singletonList(saved) + )); + + Map> expected = Collections.singletonMap(playerUUID, Collections.singletonList( + new Ping(saved.getDate(), serverUUID(), value, value, value) + )); + Map> fetched = db().query(PingQueries.fetchAllPingData()); + assertEquals(expected, fetched); + } + + + @Test + default void pingStoreTransactionOutOfOrderUpdatesUserInformation() { + db().executeTransaction(new PingStoreTransaction(playerUUID, serverUUID(), + Collections.singletonList(RandomData.randomIntDateObject()) + )); + long registerDate = RandomData.randomTime(); + db().executeTransaction(new PlayerRegisterTransaction(playerUUID, () -> registerDate, TestConstants.PLAYER_ONE_NAME)); + + Optional expected = Optional.of(new BaseUser(playerUUID, TestConstants.PLAYER_ONE_NAME, registerDate, 0)); + Optional result = db().query(BaseUserQueries.fetchBaseUserOfPlayer(playerUUID)); + assertEquals(expected, result); + } + @Test default void singlePingIsStored() { prepareForPingStorage(); diff --git a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/SessionQueriesTest.java b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/SessionQueriesTest.java index b9ee4dbc55..4b8bec4601 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/SessionQueriesTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/storage/database/queries/SessionQueriesTest.java @@ -19,14 +19,12 @@ import com.djrapitops.plan.delivery.domain.TablePlayer; import com.djrapitops.plan.delivery.domain.container.PlayerContainer; import com.djrapitops.plan.delivery.domain.mutators.SessionsMutator; -import com.djrapitops.plan.gathering.domain.FinishedSession; -import com.djrapitops.plan.gathering.domain.PlayerKill; -import com.djrapitops.plan.gathering.domain.PlayerKills; -import com.djrapitops.plan.gathering.domain.WorldTimes; +import com.djrapitops.plan.gathering.domain.*; import com.djrapitops.plan.identification.Server; import com.djrapitops.plan.identification.ServerUUID; import com.djrapitops.plan.storage.database.DatabaseTestPreparer; import com.djrapitops.plan.storage.database.queries.containers.PlayerContainerQuery; +import com.djrapitops.plan.storage.database.queries.objects.BaseUserQueries; import com.djrapitops.plan.storage.database.queries.objects.KillQueries; import com.djrapitops.plan.storage.database.queries.objects.SessionQueries; import com.djrapitops.plan.storage.database.queries.objects.WorldTimesQueries; @@ -36,7 +34,9 @@ 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.events.PlayerRegisterTransaction; import com.djrapitops.plan.storage.database.transactions.events.PlayerServerRegisterTransaction; +import com.djrapitops.plan.storage.database.transactions.events.SessionEndTransaction; import com.djrapitops.plan.storage.database.transactions.events.WorldNameStoreTransaction; import com.djrapitops.plan.utilities.java.Maps; import net.playeranalytics.plugin.scheduling.TimeAmount; @@ -53,6 +53,86 @@ public interface SessionQueriesTest extends DatabaseTestPreparer { + @Test + default void sessionStoreTransactionOutOfOrderDoesNotFailDueToMissingMainUser() { + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[0])); + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[1])); + db().executeTransaction(new PlayerServerRegisterTransaction(player2UUID, RandomData::randomTime, + TestConstants.PLAYER_TWO_NAME, serverUUID(), TestConstants.GET_PLAYER_HOSTNAME)); + + FinishedSession session = RandomData.randomSession(serverUUID(), worlds, playerUUID, player2UUID); + + db().executeTransaction(new SessionEndTransaction(session)); + + Map> sessions = db().query(SessionQueries.fetchSessionsOfPlayer(playerUUID)); + List savedSessions = sessions.get(serverUUID()); + + assertNotNull(savedSessions); + assertEquals(1, savedSessions.size()); + + assertEquals(session, savedSessions.get(0)); + } + + @Test + default void sessionStoreTransactionOutOfOrderDoesNotFailDueToMissingKilledUser() { + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[0])); + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[1])); + db().executeTransaction(new PlayerServerRegisterTransaction(playerUUID, RandomData::randomTime, + TestConstants.PLAYER_ONE_NAME, serverUUID(), TestConstants.GET_PLAYER_HOSTNAME)); + + FinishedSession session = RandomData.randomSession(serverUUID(), worlds, playerUUID, player2UUID); + db().executeTransaction(new SessionEndTransaction(session)); + + Map> sessions = db().query(SessionQueries.fetchSessionsOfPlayer(playerUUID)); + List savedSessions = sessions.get(serverUUID()); + + assertNotNull(savedSessions); + assertEquals(1, savedSessions.size()); + + // Query doesn't fetch kills since the uuid for killed player is missing + session.getExtraData(PlayerKills.class) + .map(PlayerKills::asList) + .ifPresent(List::clear); + + assertEquals(session, savedSessions.get(0)); + } + + @Test + default void killsAreAvailableAfter2ndUserRegisterEvenIfOutOfOrder() { + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[0])); + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[1])); + db().executeTransaction(new PlayerServerRegisterTransaction(playerUUID, RandomData::randomTime, + TestConstants.PLAYER_ONE_NAME, serverUUID(), TestConstants.GET_PLAYER_HOSTNAME)); + FinishedSession session = RandomData.randomSession(serverUUID(), worlds, playerUUID, player2UUID); + db().executeTransaction(new SessionEndTransaction(session)); + // killed user is registered after session already ended. + db().executeTransaction(new PlayerServerRegisterTransaction(player2UUID, RandomData::randomTime, + TestConstants.PLAYER_TWO_NAME, serverUUID(), TestConstants.GET_PLAYER_HOSTNAME)); + + Map> sessions = db().query(SessionQueries.fetchSessionsOfPlayer(playerUUID)); + List savedSessions = sessions.get(serverUUID()); + + assertNotNull(savedSessions); + assertEquals(1, savedSessions.size()); + + assertEquals(session, savedSessions.get(0)); + } + + @Test + default void sessionStoreTransactionOutOfOrderUpdatesUserInformation() { + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[0])); + db().executeTransaction(new WorldNameStoreTransaction(serverUUID(), worlds[1])); + FinishedSession session = RandomData.randomSession(serverUUID(), worlds, playerUUID, player2UUID); + db().executeTransaction(new SessionEndTransaction(session)); + + long registerDate = RandomData.randomTime(); + db().executeTransaction(new PlayerRegisterTransaction(playerUUID, () -> registerDate, TestConstants.PLAYER_ONE_NAME)); + + Optional expected = Optional.of(new BaseUser(playerUUID, TestConstants.PLAYER_ONE_NAME, registerDate, 0)); + Optional result = db().query(BaseUserQueries.fetchBaseUserOfPlayer(playerUUID)); + assertEquals(expected, result); + } + @Test default void sessionPlaytimeIsCalculatedCorrectlyAfterStorage() { prepareForSessionSave();