Make unit stack filter return a serializable list. (#11892)

game-app/game-core/src/main/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilter.java

@@ -24,12 +24,13 @@ public class UnitStackingLimitFilter {
   public static final String PLACEMENT_LIMIT = "placementLimit";
 
   /**
-   * Returns the subset of units that are valid with respect to any stacking limits in effect.
+   * Returns the subset of units that are valid with respect to any stacking limits in effect. The
+   * returned list is mutable and serializable.
    *
    * <p>Note: The passed list of units should have already been filtered for placement restrictions
    * as otherwise this could return a subset of units that cannot be placed for other reasons.
    */
-  public static Collection<Unit> filterUnits(
+  public static List<Unit> filterUnits(
       final Collection<Unit> units,
       final String limitType,
       final GamePlayer owner,
@@ -40,7 +41,7 @@ public static Collection<Unit> filterUnits(
   /**
    * Same as above, but allows passing `existingUnitsToBePlaced` that have already been selected.
    */
-  public static Collection<Unit> filterUnits(
+  public static List<Unit> filterUnits(
       final Collection<Unit> units,
       final String limitType,
       final GamePlayer owner,
@@ -79,7 +80,10 @@ public static Collection<Unit> filterUnits(
         unitsAllowedSoFar.add(unit);
       }
     }
-    return unitsAllowedSoFar.subList(existingUnitsToBePlaced.size(), unitsAllowedSoFar.size());
+    // Remove the existing units from the list before returning it. Don't return a sublist as it's
+    // not serializable.
+    unitsAllowedSoFar.subList(0, existingUnitsToBePlaced.size()).clear();
+    return unitsAllowedSoFar;
   }
 
   /**

game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitTest.java → game-app/game-core/src/test/java/games/strategy/triplea/delegate/move/validation/UnitStackingLimitFilterTest.java

@@ -1,51 +1,65 @@
 package games.strategy.triplea.delegate.move.validation;
 
-import static games.strategy.triplea.delegate.move.validation.UnitStackingLimitFilter.PLACEMENT_LIMIT;
 import static games.strategy.triplea.delegate.move.validation.UnitStackingLimitFilter.filterUnits;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.collection.IsEmptyCollection.empty;
 import static org.triplea.java.collections.CollectionUtils.countMatches;
 
+import games.strategy.engine.data.GamePlayer;
+import games.strategy.engine.data.Territory;
 import games.strategy.engine.data.Unit;
 import games.strategy.triplea.delegate.AbstractDelegateTestCase;
 import games.strategy.triplea.delegate.Matches;
+import java.io.Serializable;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import org.junit.jupiter.api.Test;
 
-class UnitStackingLimitTest extends AbstractDelegateTestCase {
+class UnitStackingLimitFilterTest extends AbstractDelegateTestCase {
+
+  private static List<Unit> callFilterUnits(
+      Collection<Unit> units, GamePlayer owner, Territory t, Collection<Unit> existingUnits) {
+    var result =
+        filterUnits(units, UnitStackingLimitFilter.PLACEMENT_LIMIT, owner, t, existingUnits);
+    assertThat(result, instanceOf(Serializable.class));
+    return result;
+  }
+
+  private static List<Unit> callFilterUnits(Collection<Unit> units, GamePlayer owner, Territory t) {
+    var result = filterUnits(units, UnitStackingLimitFilter.PLACEMENT_LIMIT, owner, t);
+    assertThat(result, instanceOf(Serializable.class));
+    return result;
+  }
 
   @Test
   void testUnitAttachmentStackingLimit() {
     // we can place 4
     List<Unit> fourTanks = armour.create(4, british);
-    assertThat(filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk), is(fourTanks));
+    assertThat(callFilterUnits(fourTanks, british, uk), is(fourTanks));
 
     // the same four tanks are returned, even if we pass some existing units.
-    assertThat(
-        filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk, infantry.create(2, british)),
-        is(fourTanks));
+    assertThat(callFilterUnits(fourTanks, british, uk, infantry.create(2, british)), is(fourTanks));
     // and only 2 are returned if we pass 2 existing tanks
-    assertThat(
-        filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk, armour.create(2, british)),
-        hasSize(2));
+    assertThat(callFilterUnits(fourTanks, british, uk, armour.create(2, british)), hasSize(2));
 
     // we can't place 5 per the unit attachment's placementLimit
     List<Unit> fiveTanks = armour.create(5, british);
-    assertThat(filterUnits(fiveTanks, PLACEMENT_LIMIT, british, uk), hasSize(4));
+    assertThat(callFilterUnits(fiveTanks, british, uk), hasSize(4));
 
     // only 2 can be placed if 2 are already there
     uk.getUnitCollection().addAll(armour.create(2, british));
-    assertThat(filterUnits(fourTanks, PLACEMENT_LIMIT, british, uk), hasSize(2));
+    assertThat(callFilterUnits(fourTanks, british, uk), hasSize(2));
 
     // but we can include other units that don't have stacking limits in the list
     // note: still with the two tanks already in the uk
     List<Unit> twoInfantryAndFourTanks = infantry.create(2, british);
     twoInfantryAndFourTanks.addAll(fourTanks);
     assertThat(twoInfantryAndFourTanks, hasSize(6));
-    var result = filterUnits(twoInfantryAndFourTanks, PLACEMENT_LIMIT, british, uk);
+    var result = callFilterUnits(twoInfantryAndFourTanks, british, uk);
     assertThat(result, hasSize(4));
     assertThat(countMatches(result, Matches.unitIsOfType(infantry)), is(2));
     assertThat(countMatches(result, Matches.unitIsOfType(armour)), is(2));
@@ -55,38 +69,38 @@ void testUnitAttachmentStackingLimit() {
   void testClassicAaStackingLimit() {
     // No more aa guns to be placed in UK.
     List<Unit> units = aaGun.create(2, british);
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), empty());
+    assertThat(callFilterUnits(units, british, uk), empty());
 
     // Remove the aa gun in UK, now one can be placed.
     uk.getUnitCollection().removeIf(Matches.unitIsOfType(aaGun));
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), hasSize(1));
+    assertThat(callFilterUnits(units, british, uk), hasSize(1));
   }
 
   @Test
   void testPlayerAttachmentStackingLimit() {
     // we can place 3 battleships
     List<Unit> units = battleship.create(3, british);
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(units));
+    assertThat(callFilterUnits(units, british, uk), is(units));
 
     // but not 4
     units = battleship.create(4, british);
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), hasSize(3));
+    assertThat(callFilterUnits(units, british, uk), hasSize(3));
 
     // we can also place 2 battleships and a carrier
     units = battleship.create(2, british);
     units.addAll(carrier.create(1, british));
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(units));
+    assertThat(callFilterUnits(units, british, uk), is(units));
 
     // but not 2 battleships and 2 carriers
     units.addAll(carrier.create(1, british));
     var expected = units.subList(0, 3);
     assertThat(expected, hasSize(3));
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(expected));
+    assertThat(callFilterUnits(units, british, uk), is(expected));
 
     // and that the filtered units returned are in order
     Collections.shuffle(units);
     expected = units.subList(0, 3);
     assertThat(expected, hasSize(3));
-    assertThat(filterUnits(units, PLACEMENT_LIMIT, british, uk), is(expected));
+    assertThat(callFilterUnits(units, british, uk), is(expected));
   }
 }