From cd5195420803cf96eeca3c6f16af9f8e85ec5182 Mon Sep 17 00:00:00 2001 From: Oleg Agafonov Date: Thu, 19 Sep 2024 04:32:21 +0400 Subject: [PATCH] tests: added verify check for wrong predicates usage in filters (ClassCastException errors like #12774) --- .../client/dialog/TestCardRenderDialog.java | 74 +------------------ Mage.Sets/src/mage/cards/c/ConduitOfRuin.java | 8 +- .../mage/cards/e/ElugeTheShorelessSea.java | 8 +- .../mage/cards/m/MelekReforgedResearcher.java | 8 +- .../src/mage/cards/s/ShadowInTheWarp.java | 8 +- .../java/mage/verify/VerifyCardDataTest.java | 7 +- .../src/main/java/mage/filter/FilterCard.java | 2 + .../java/mage/filter/FilterPermanent.java | 6 +- .../main/java/mage/filter/FilterPlayer.java | 5 ++ .../java/mage/filter/FilterStackObject.java | 8 ++ .../java/mage/filter/predicate/Predicate.java | 1 - .../mage/filter/predicate/Predicates.java | 48 +++++++++++- Mage/src/main/java/mage/game/FakeGame.java | 63 ++++++++++++++++ Mage/src/main/java/mage/game/FakeMatch.java | 21 ++++++ 14 files changed, 176 insertions(+), 91 deletions(-) create mode 100644 Mage/src/main/java/mage/game/FakeGame.java create mode 100644 Mage/src/main/java/mage/game/FakeMatch.java diff --git a/Mage.Client/src/main/java/mage/client/dialog/TestCardRenderDialog.java b/Mage.Client/src/main/java/mage/client/dialog/TestCardRenderDialog.java index e209b2b57a2..dd171f36076 100644 --- a/Mage.Client/src/main/java/mage/client/dialog/TestCardRenderDialog.java +++ b/Mage.Client/src/main/java/mage/client/dialog/TestCardRenderDialog.java @@ -27,9 +27,7 @@ import mage.counters.Counter; import mage.counters.CounterType; import mage.designations.CitysBlessing; import mage.designations.Monarch; -import mage.game.Game; -import mage.game.GameException; -import mage.game.GameImpl; +import mage.game.*; import mage.game.command.Dungeon; import mage.game.command.Emblem; import mage.game.command.Plane; @@ -270,8 +268,8 @@ public class TestCardRenderDialog extends MageDialog { // prepare fake game and players without real match // it's a workaround with minimum code and data init - this.match = new TestMatch(); - this.game = new TestGame(MultiplayerAttackOption.MULTIPLE, RangeOfInfluence.ALL, MulliganType.GAME_DEFAULT.getMulligan(0), 20, 7); + this.match = new FakeMatch(); + this.game = new FakeGame(); Deck deck = new Deck(); Player playerYou = new StubPlayer("player1", RangeOfInfluence.ALL); playerYou.addDesignation(new CitysBlessing()); @@ -977,68 +975,4 @@ public class TestCardRenderDialog extends MageDialog { private javax.swing.JSpinner spinnerCardIconsAdditionalAmount; private javax.swing.JSpinner spinnerCardIconsMaxVisible; // End of variables declaration//GEN-END:variables -} - -class TestGame extends GameImpl { - - private int numPlayers; - - public TestGame(MultiplayerAttackOption attackOption, RangeOfInfluence range, Mulligan mulligan, int startLife, int startHandSize) { - super(attackOption, range, mulligan, 60, startLife, startHandSize); - } - - public TestGame(final TestGame game) { - super(game); - this.numPlayers = game.numPlayers; - } - - @Override - public MatchType getGameType() { - return new TestGameType(); - } - - @Override - public int getNumPlayers() { - return numPlayers; - } - - @Override - public TestGame copy() { - return new TestGame(this); - } - -} - -class TestGameType extends MatchType { - - public TestGameType() { - this.name = "Test Game Type"; - this.maxPlayers = 10; - this.minPlayers = 3; - this.numTeams = 0; - this.useAttackOption = true; - this.useRange = true; - this.sideboardingAllowed = true; - } - - protected TestGameType(final TestGameType matchType) { - super(matchType); - } - - @Override - public TestGameType copy() { - return new TestGameType(this); - } -} - -class TestMatch extends MatchImpl { - - public TestMatch() { - super(new MatchOptions("fake match", "fake game type", true, 2)); - } - - @Override - public void startGame() throws GameException { - throw new IllegalStateException("Can't start fake match"); - } -} +} \ No newline at end of file diff --git a/Mage.Sets/src/mage/cards/c/ConduitOfRuin.java b/Mage.Sets/src/mage/cards/c/ConduitOfRuin.java index 7e8ce759195..40bffb1b60d 100644 --- a/Mage.Sets/src/mage/cards/c/ConduitOfRuin.java +++ b/Mage.Sets/src/mage/cards/c/ConduitOfRuin.java @@ -99,12 +99,12 @@ class ConduitOfRuinWatcher extends Watcher { } } -class FirstCastCreatureSpellPredicate implements ObjectSourcePlayerPredicate { +class FirstCastCreatureSpellPredicate implements ObjectSourcePlayerPredicate { @Override - public boolean apply(ObjectSourcePlayer input, Game game) { - if (input.getObject() instanceof Card - && ((Card) input.getObject()).isCreature(game)) { + public boolean apply(ObjectSourcePlayer input, Game game) { + if (input.getObject() != null + && input.getObject().isCreature(game)) { ConduitOfRuinWatcher watcher = game.getState().getWatcher(ConduitOfRuinWatcher.class); return watcher != null && watcher.creatureSpellsCastThisTurn(input.getPlayerId()) == 0; } diff --git a/Mage.Sets/src/mage/cards/e/ElugeTheShorelessSea.java b/Mage.Sets/src/mage/cards/e/ElugeTheShorelessSea.java index 977ffa25e05..bfe5524a816 100644 --- a/Mage.Sets/src/mage/cards/e/ElugeTheShorelessSea.java +++ b/Mage.Sets/src/mage/cards/e/ElugeTheShorelessSea.java @@ -93,13 +93,13 @@ public final class ElugeTheShorelessSea extends CardImpl { } } -enum ElugeTheShorelessSeaPredicate implements ObjectSourcePlayerPredicate { +enum ElugeTheShorelessSeaPredicate implements ObjectSourcePlayerPredicate { instance; @Override - public boolean apply(ObjectSourcePlayer input, Game game) { - if (input.getObject() instanceof Card && - ((Card) input.getObject()).isInstantOrSorcery(game)) { + public boolean apply(ObjectSourcePlayer input, Game game) { + if (input.getObject() != null && + input.getObject().isInstantOrSorcery(game)) { ElugeTheShorelessSeaWatcher watcher = game.getState().getWatcher(ElugeTheShorelessSeaWatcher.class); return watcher != null && watcher.getInstantOrSorcerySpellsCastThisTurn(input.getPlayerId()) == 0; diff --git a/Mage.Sets/src/mage/cards/m/MelekReforgedResearcher.java b/Mage.Sets/src/mage/cards/m/MelekReforgedResearcher.java index 6dc069b4072..6fde3c71897 100644 --- a/Mage.Sets/src/mage/cards/m/MelekReforgedResearcher.java +++ b/Mage.Sets/src/mage/cards/m/MelekReforgedResearcher.java @@ -71,13 +71,13 @@ public final class MelekReforgedResearcher extends CardImpl { } } -enum MelekReforgedResearcherPredicate implements ObjectSourcePlayerPredicate { +enum MelekReforgedResearcherPredicate implements ObjectSourcePlayerPredicate { instance; @Override - public boolean apply(ObjectSourcePlayer input, Game game) { - if (input.getObject() instanceof Card && - ((Card) input.getObject()).isInstantOrSorcery(game)) { + public boolean apply(ObjectSourcePlayer input, Game game) { + if (input.getObject() != null && + input.getObject().isInstantOrSorcery(game)) { MelekReforgedResearcherWatcher watcher = game.getState().getWatcher(MelekReforgedResearcherWatcher.class); return watcher != null && watcher.getInstantOrSorcerySpellsCastThisTurn(input.getPlayerId()) == 0; diff --git a/Mage.Sets/src/mage/cards/s/ShadowInTheWarp.java b/Mage.Sets/src/mage/cards/s/ShadowInTheWarp.java index f237f78721f..adb6eecdab8 100644 --- a/Mage.Sets/src/mage/cards/s/ShadowInTheWarp.java +++ b/Mage.Sets/src/mage/cards/s/ShadowInTheWarp.java @@ -88,12 +88,12 @@ class ShadowInTheWarpWatcher extends Watcher { } } -class FirstCastCreatureSpellPredicate implements ObjectSourcePlayerPredicate { +class FirstCastCreatureSpellPredicate implements ObjectSourcePlayerPredicate { @Override - public boolean apply(ObjectSourcePlayer input, Game game) { - if (input.getObject() instanceof Card - && ((Card) input.getObject()).isCreature(game)) { + public boolean apply(ObjectSourcePlayer input, Game game) { + if (input.getObject() != null + && input.getObject().isCreature(game)) { ShadowInTheWarpWatcher watcher = game.getState().getWatcher(ShadowInTheWarpWatcher.class); return watcher != null && watcher.creatureSpellsCastThisTurn(input.getPlayerId()) == 0; } diff --git a/Mage.Verify/src/test/java/mage/verify/VerifyCardDataTest.java b/Mage.Verify/src/test/java/mage/verify/VerifyCardDataTest.java index eeee3d91ce1..d7d8717eb27 100644 --- a/Mage.Verify/src/test/java/mage/verify/VerifyCardDataTest.java +++ b/Mage.Verify/src/test/java/mage/verify/VerifyCardDataTest.java @@ -2769,6 +2769,9 @@ public class VerifyCardDataTest { public void test_checkCardConstructors() { // create all cards, can catch additional verify and runtime checks from abilities and effects // example: wrong code usage errors + // + // warning, look at stack trace logs, not card names -- some error code can be hidden in static methods and can be called from un-related cards + // use test_showCardInfo for detailed errors Collection errorsList = new ArrayList<>(); Collection sets = Sets.getInstance().values(); for (ExpansionSet set : sets) { @@ -2777,7 +2780,7 @@ public class VerifyCardDataTest { Card card = CardImpl.createCard(setInfo.getCardClass(), new CardSetInfo(setInfo.getName(), set.getCode(), setInfo.getCardNumber(), setInfo.getRarity(), setInfo.getGraphicInfo())); if (card == null) { - errorsList.add("Error: broken constructor " + setInfo.getCardClass()); + errorsList.add("Error: can't create card - " + setInfo.getCardClass() + " - see logs for errors"); continue; } if (!card.getExpansionSetCode().equals(set.getCode())) { @@ -2785,7 +2788,7 @@ public class VerifyCardDataTest { } } catch (Throwable e) { // CardImpl.createCard don't throw exceptions (only error logs), so that logs are useless here - logger.error("Error: can't create card " + setInfo.getName() + ": " + e.getMessage(), e); + errorsList.add("Error: can't create card - " + setInfo.getCardClass() + " - see logs for errors"); } } } diff --git a/Mage/src/main/java/mage/filter/FilterCard.java b/Mage/src/main/java/mage/filter/FilterCard.java index c4d2f98183c..ccdb72f6e52 100644 --- a/Mage/src/main/java/mage/filter/FilterCard.java +++ b/Mage/src/main/java/mage/filter/FilterCard.java @@ -73,7 +73,9 @@ public class FilterCard extends FilterObject { throw new UnsupportedOperationException("You may not modify a locked filter"); } + // verify check checkPredicateIsSuitableForCardFilter(predicate); + Predicates.makeSurePredicateCompatibleWithFilter(predicate, Card.class); extraPredicates.add(predicate); } diff --git a/Mage/src/main/java/mage/filter/FilterPermanent.java b/Mage/src/main/java/mage/filter/FilterPermanent.java index 0177accc9f8..9eaee653392 100644 --- a/Mage/src/main/java/mage/filter/FilterPermanent.java +++ b/Mage/src/main/java/mage/filter/FilterPermanent.java @@ -5,6 +5,7 @@ import mage.constants.SubType; import mage.filter.predicate.ObjectSourcePlayer; import mage.filter.predicate.ObjectSourcePlayerPredicate; import mage.filter.predicate.Predicate; +import mage.filter.predicate.Predicates; import mage.game.Game; import mage.game.permanent.Permanent; @@ -12,7 +13,6 @@ import java.util.ArrayList; import java.util.List; import java.util.Set; import java.util.UUID; -import java.util.stream.Collectors; /** * @author North @@ -64,6 +64,10 @@ public class FilterPermanent extends FilterObject implements FilterIn if (isLockedFilter()) { throw new UnsupportedOperationException("You may not modify a locked filter"); } + + // verify check + Predicates.makeSurePredicateCompatibleWithFilter(predicate, Permanent.class); + extraPredicates.add(predicate); } diff --git a/Mage/src/main/java/mage/filter/FilterPlayer.java b/Mage/src/main/java/mage/filter/FilterPlayer.java index 0fa56b3a2a7..766cba8253b 100644 --- a/Mage/src/main/java/mage/filter/FilterPlayer.java +++ b/Mage/src/main/java/mage/filter/FilterPlayer.java @@ -4,6 +4,7 @@ import mage.abilities.Ability; import mage.filter.predicate.ObjectSourcePlayer; import mage.filter.predicate.ObjectSourcePlayerPredicate; import mage.filter.predicate.Predicate; +import mage.filter.predicate.Predicates; import mage.game.Game; import mage.players.Player; @@ -36,6 +37,10 @@ public class FilterPlayer extends FilterImpl { if (isLockedFilter()) { throw new UnsupportedOperationException("You may not modify a locked filter"); } + + // verify check + Predicates.makeSurePredicateCompatibleWithFilter(predicate, Player.class); + extraPredicates.add(predicate); return this; } diff --git a/Mage/src/main/java/mage/filter/FilterStackObject.java b/Mage/src/main/java/mage/filter/FilterStackObject.java index bfdd56d54fd..aa3b506c1ae 100644 --- a/Mage/src/main/java/mage/filter/FilterStackObject.java +++ b/Mage/src/main/java/mage/filter/FilterStackObject.java @@ -1,10 +1,13 @@ package mage.filter; import mage.abilities.Ability; +import mage.cards.Card; import mage.filter.predicate.ObjectSourcePlayer; import mage.filter.predicate.ObjectSourcePlayerPredicate; import mage.filter.predicate.Predicate; +import mage.filter.predicate.Predicates; import mage.game.Game; +import mage.game.stack.Spell; import mage.game.stack.StackObject; import java.util.ArrayList; @@ -43,6 +46,11 @@ public class FilterStackObject extends FilterObject { if (isLockedFilter()) { throw new UnsupportedOperationException("You may not modify a locked filter"); } + + // verify check + // Spell implements Card interface, so it can use some default predicates like owner + Predicates.makeSurePredicateCompatibleWithFilter(predicate, StackObject.class, Spell.class, Card.class); + extraPredicates.add(predicate); } diff --git a/Mage/src/main/java/mage/filter/predicate/Predicate.java b/Mage/src/main/java/mage/filter/predicate/Predicate.java index e4e64de3963..6b7b5382f31 100644 --- a/Mage/src/main/java/mage/filter/predicate/Predicate.java +++ b/Mage/src/main/java/mage/filter/predicate/Predicate.java @@ -1,4 +1,3 @@ - package mage.filter.predicate; import java.io.Serializable; diff --git a/Mage/src/main/java/mage/filter/predicate/Predicates.java b/Mage/src/main/java/mage/filter/predicate/Predicates.java index 0f2a0c9cbbd..bf3213c8290 100644 --- a/Mage/src/main/java/mage/filter/predicate/Predicates.java +++ b/Mage/src/main/java/mage/filter/predicate/Predicates.java @@ -2,6 +2,8 @@ package mage.filter.predicate; import mage.game.Game; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -11,7 +13,7 @@ import java.util.List; * *

All methods returns serializable predicates as long as they're given serializable parameters.

* - * @author North + * @author North, JayDi85 */ public final class Predicates { @@ -246,4 +248,48 @@ public final class Predicates { extraPredicates.forEach(p -> collectAllComponents(p, res)); } } + + /** + * Verify check: try to find filters usage + * Example use case: Player predicate was used for Permanent filter + * Example error: java.lang.ClassCastException: mage.game.permanent.PermanentToken cannot be cast to mage.players.Player + */ + public static void makeSurePredicateCompatibleWithFilter(Predicate predicate, Class... compatibleClasses) { + List list = new ArrayList<>(); + Predicates.collectAllComponents(predicate, list); + list.forEach(p -> { + Class predicateGenericParamClass = findGenericParam(predicate); + if (predicateGenericParamClass == null) { + throw new IllegalArgumentException("Somthing wrong. Can't find predicate's generic param for " + predicate.getClass()); + } + if (Arrays.stream(compatibleClasses).anyMatch(f -> predicateGenericParamClass.isAssignableFrom(f))) { + // predicate is fine + } else { + // How-to fix: use correct predicates (same type, e.g. getControllerPredicate() instead getPlayerPredicate()) + throw new IllegalArgumentException(String.format( + "Wrong code usage: predicate [%s] with generic param [%s] can't be added to filter, allow only %s", + predicate.getClass(), + predicateGenericParamClass, + Arrays.toString(compatibleClasses) + )); + } + }); + } + + private static Class findGenericParam(Predicate predicate) { + Type[] interfaces = predicate.getClass().getGenericInterfaces(); + for (Type type : interfaces) { + if (type instanceof ParameterizedType) { + ParameterizedType parameterizedType = (ParameterizedType) type; + Type[] actualTypeArguments = parameterizedType.getActualTypeArguments(); + if (actualTypeArguments.length > 0) { + Type actualType = actualTypeArguments[0]; + if (actualType instanceof Class) { + return (Class) actualType; + } + } + } + } + return null; + } } diff --git a/Mage/src/main/java/mage/game/FakeGame.java b/Mage/src/main/java/mage/game/FakeGame.java new file mode 100644 index 00000000000..7bee3465e6a --- /dev/null +++ b/Mage/src/main/java/mage/game/FakeGame.java @@ -0,0 +1,63 @@ +package mage.game; + +import mage.constants.MultiplayerAttackOption; +import mage.constants.RangeOfInfluence; +import mage.game.match.MatchType; +import mage.game.mulligan.MulliganType; + +/** + * Fake game for tests and data check, do nothing. + * + * @author JayDi85 + */ +public class FakeGame extends GameImpl { + + private int numPlayers; + + public FakeGame() { + super(MultiplayerAttackOption.MULTIPLE, RangeOfInfluence.ALL, MulliganType.GAME_DEFAULT.getMulligan(0), 60, 20, 7); + } + + public FakeGame(final FakeGame game) { + super(game); + this.numPlayers = game.numPlayers; + } + + @Override + public MatchType getGameType() { + return new FakeGameType(); + } + + @Override + public int getNumPlayers() { + return numPlayers; + } + + @Override + public FakeGame copy() { + return new FakeGame(this); + } + +} + +class FakeGameType extends MatchType { + + public FakeGameType() { + this.name = "Test Game Type"; + this.maxPlayers = 10; + this.minPlayers = 3; + this.numTeams = 0; + this.useAttackOption = true; + this.useRange = true; + this.sideboardingAllowed = true; + } + + protected FakeGameType(final FakeGameType matchType) { + super(matchType); + } + + @Override + public FakeGameType copy() { + return new FakeGameType(this); + } +} \ No newline at end of file diff --git a/Mage/src/main/java/mage/game/FakeMatch.java b/Mage/src/main/java/mage/game/FakeMatch.java new file mode 100644 index 00000000000..44cb401b961 --- /dev/null +++ b/Mage/src/main/java/mage/game/FakeMatch.java @@ -0,0 +1,21 @@ +package mage.game; + +import mage.game.match.MatchImpl; +import mage.game.match.MatchOptions; + +/** + * Fake match for tests and data check, do nothing. + * + * @author JayDi85 + */ +public class FakeMatch extends MatchImpl { + + public FakeMatch() { + super(new MatchOptions("fake match", "fake game type", true, 2)); + } + + @Override + public void startGame() throws GameException { + throw new IllegalStateException("Can't start fake match"); + } +}