Clean up updateMap futures and make tests deterministic (#2123)

src/main/java/com/faforever/client/api/FafApiAccessor.java

@@ -97,7 +97,7 @@ public interface FafApiAccessor {
 
   Optional<MapVersion> findMapByFolderName(String folderName);
 
-  Optional<MapVersion> getLatestVersionMap(String mapFolderName);
+  Optional<MapVersion> getMapLatestVersion(String mapFolderName);
 
   List<com.faforever.client.api.dto.Player> getPlayersByIds(Collection<Integer> playerIds);
 

src/main/java/com/faforever/client/api/FafApiAccessorImpl.java

@@ -380,7 +380,7 @@ FILTER, rsql(qBuilder().string("filename").eq(format(FILENAME_TEMPLATE, folderNa
   }
 
   @Override
-  public Optional<MapVersion> getLatestVersionMap(String mapFolderName) {
+  public Optional<MapVersion> getMapLatestVersion(String mapFolderName) {
     String queryFilter = rsql(qBuilder()
         .string("filename").eq(format(FILENAME_TEMPLATE, mapFolderName))
         .and()

src/main/java/com/faforever/client/api/MockFafApiAccessor.java

@@ -211,7 +211,7 @@ public Optional<MapVersion> findMapByFolderName(String folderName) {
   }
 
   @Override
-  public Optional<MapVersion> getLatestVersionMap(String mapFolderName) {
+  public Optional<MapVersion> getMapLatestVersion(String mapFolderName) {
     return Optional.empty();
   }
 

src/main/java/com/faforever/client/game/CreateGameController.java

@@ -68,9 +68,9 @@
 @Slf4j
 public class CreateGameController implements Controller<Pane> {
 
-  private static final int MAX_RATING_LENGTH = 4;
   public static final String STYLE_CLASS_DUAL_LIST_CELL = "create-game-dual-list-cell";
   public static final PseudoClass PSEUDO_CLASS_INVALID = PseudoClass.getPseudoClass("invalid");
+  private static final int MAX_RATING_LENGTH = 4;
   private final MapService mapService;
   private final ModService modService;
   private final GameService gameService;
@@ -309,7 +309,7 @@ private void initRatingBoundaries() {
 
     minRankingTextField.textProperty().addListener((observable, oldValue, newValue) -> {
       Integer minRating = null;
-      if(!newValue.isEmpty()) {
+      if (!newValue.isEmpty()) {
         minRating = Integer.parseInt(newValue);
       }
 
@@ -397,15 +397,13 @@ public void onCreateButtonClicked() {
     if (mapService.isOfficialMap(selectedMap)) {
       hostGame(selectedMap);
     } else {
-      mapService.updateMapToLatestVersionIfNecessary(selectedMap)
-          .whenComplete((map, throwable) -> {
-            if (throwable != null) {
-              log.error("error when updating the map", throwable);
-              hostGame(selectedMap);
-            } else {
-              hostGame(map);
-            }
-          });
+      mapService.updateLatestVersionIfNecessary(selectedMap)
+          .exceptionally(throwable -> {
+            log.error("error when updating the map", throwable);
+            hostGame(selectedMap);
+            return null;
+          })
+          .thenAccept(this::hostGame);
     }
   }
 
@@ -422,7 +420,7 @@ private void hostGame(MapBean map) {
       minRating = Integer.parseInt(minRankingTextField.getText());
     }
 
-    if(!maxRankingTextField.getText().isEmpty()) {
+    if (!maxRankingTextField.getText().isEmpty()) {
       maxRating = Integer.parseInt(maxRankingTextField.getText());
     }
 

src/main/java/com/faforever/client/map/MapService.java

@@ -83,10 +83,9 @@
 @Service
 public class MapService implements InitializingBean, DisposableBean {
 
+  public static final String DEBUG = "debug";
   private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
   private static final String MAP_VERSION_REGEX = ".*[.v](?<version>\\d{4})$"; // Matches to an string like 'adaptive_twin_rivers.v0031'
-  public static final String DEBUG = "debug";
-
   private final PreferencesService preferencesService;
   private final TaskService taskService;
   private final ApplicationContext applicationContext;
@@ -105,6 +104,14 @@ public class MapService implements InitializingBean, DisposableBean {
   private final Map<Path, MapBean> pathToMap = new HashMap<>();
   private final ObservableList<MapBean> installedMaps = FXCollections.observableArrayList();
   private final Map<String, MapBean> mapsByFolderName = new HashMap<>();
+  @VisibleForTesting
+  Set<String> officialMaps = ImmutableSet.of(
+      "SCMP_001", "SCMP_002", "SCMP_003", "SCMP_004", "SCMP_005", "SCMP_006", "SCMP_007", "SCMP_008", "SCMP_009", "SCMP_010", "SCMP_011",
+      "SCMP_012", "SCMP_013", "SCMP_014", "SCMP_015", "SCMP_016", "SCMP_017", "SCMP_018", "SCMP_019", "SCMP_020", "SCMP_021", "SCMP_022",
+      "SCMP_023", "SCMP_024", "SCMP_025", "SCMP_026", "SCMP_027", "SCMP_028", "SCMP_029", "SCMP_030", "SCMP_031", "SCMP_032", "SCMP_033",
+      "SCMP_034", "SCMP_035", "SCMP_036", "SCMP_037", "SCMP_038", "SCMP_039", "SCMP_040", "X1MP_001", "X1MP_002", "X1MP_003", "X1MP_004",
+      "X1MP_005", "X1MP_006", "X1MP_007", "X1MP_008", "X1MP_009", "X1MP_010", "X1MP_011", "X1MP_012", "X1MP_014", "X1MP_017"
+  );
   private Thread directoryWatcherThread;
 
   @Inject
@@ -146,15 +153,6 @@ public MapService(PreferencesService preferencesService,
     });
   }
 
-  @VisibleForTesting
-  Set<String> officialMaps = ImmutableSet.of(
-      "SCMP_001", "SCMP_002", "SCMP_003", "SCMP_004", "SCMP_005", "SCMP_006", "SCMP_007", "SCMP_008", "SCMP_009", "SCMP_010", "SCMP_011",
-      "SCMP_012", "SCMP_013", "SCMP_014", "SCMP_015", "SCMP_016", "SCMP_017", "SCMP_018", "SCMP_019", "SCMP_020", "SCMP_021", "SCMP_022",
-      "SCMP_023", "SCMP_024", "SCMP_025", "SCMP_026", "SCMP_027", "SCMP_028", "SCMP_029", "SCMP_030", "SCMP_031", "SCMP_032", "SCMP_033",
-      "SCMP_034", "SCMP_035", "SCMP_036", "SCMP_037", "SCMP_038", "SCMP_039", "SCMP_040", "X1MP_001", "X1MP_002", "X1MP_003", "X1MP_004",
-      "X1MP_005", "X1MP_006", "X1MP_007", "X1MP_008", "X1MP_009", "X1MP_010", "X1MP_011", "X1MP_012", "X1MP_014", "X1MP_017"
-  );
-
   private static URL getDownloadUrl(String mapName, String baseUrl) {
     return noCatch(() -> new URL(format(baseUrl, urlFragmentEscaper().escape(mapName).toLowerCase(Locale.US))));
   }
@@ -262,7 +260,8 @@ private void removeMap(Path path) {
     installedMaps.remove(pathToMap.remove(path));
   }
 
-  private void addInstalledMap(Path path) throws MapLoadException {
+  @VisibleForTesting
+  void addInstalledMap(Path path) throws MapLoadException {
     try {
       MapBean mapBean = readMap(path);
       pathToMap.put(path, mapBean);
@@ -480,58 +479,35 @@ public CompletableFuture<Optional<MapBean>> findByMapFolderName(String folderNam
     return fafService.findMapByFolderName(folderName);
   }
 
-  public CompletableFuture<Optional<MapBean>> getLatestVersionMap(MapBean map) {
+  public CompletableFuture<MapBean> getMapLatestVersion(MapBean map) {
     String folderName = map.getFolderName();
-    if (containVersionControl(folderName)) {
-      return fafService.getLatestVersionMap(folderName);
+    if (containsVersionControl(folderName)) {
+      return fafService.getMapLatestVersion(folderName).thenApply(latestMap -> latestMap.orElse(map));
     }
-    return CompletableFuture.completedFuture(Optional.of(map));
+    return CompletableFuture.completedFuture(map);
   }
 
-  private boolean containVersionControl(String mapFolderName) {
+  private boolean containsVersionControl(String mapFolderName) {
     return Pattern.matches(MAP_VERSION_REGEX, mapFolderName);
   }
 
-  public CompletableFuture<Optional<MapBean>> getUpdatedMapIfExist(MapBean map) {
-    return getLatestVersionMap(map).thenApply(optional -> {
-      if (optional.isPresent()) {
-        MapBean latestMap = optional.get();
-        try {
-          if (isNewVersionMap(latestMap, map)) {
-            return Optional.of(latestMap);
-          }
-        } catch (CompareMapVersionException ex) {
-          logger.error("could not compare map versions", ex);
-        }
+  public CompletableFuture<MapBean> updateLatestVersionIfNecessary(MapBean map) {
+    return getMapLatestVersion(map).thenCompose(latestMap -> {
+      CompletableFuture<Void> downloadFuture;
+      if (!isInstalled(latestMap.getFolderName())) {
+        downloadFuture = download(latestMap.getFolderName());
+      } else {
+        downloadFuture = CompletableFuture.completedFuture(null);
       }
-      return Optional.empty();
-    });
-  }
-
-  private boolean isNewVersionMap(MapBean checkedMap, MapBean comparedMap) throws CompareMapVersionException {
-    if (!checkedMap.getDisplayName().equalsIgnoreCase(comparedMap.getDisplayName())) {
-      throw new CompareMapVersionException("cannot compare versions with different maps");
-    }
-    ComparableVersion v1 = checkedMap.getVersion();
-    ComparableVersion v2 = comparedMap.getVersion();
-    if (v1 == null || v2 == null) {
-      throw new CompareMapVersionException(format("cannot compare map versions, map '%s' has null version",
-          v1 == null ? checkedMap.getFolderName() : comparedMap.getFolderName()));
-    }
-    return v1.compareTo(v2) > 0;
-  }
-
-  public CompletableFuture<MapBean> updateMapToLatestVersionIfNecessary(MapBean map) {
-    return CompletableFuture.supplyAsync(() -> {
-      CheckForUpdateMapTask task = applicationContext.getBean(CheckForUpdateMapTask.class).setMap(map);
-      Optional<MapBean> optional = taskService.submitTask(task).getFuture().join();
-      if (optional.isPresent()) {
-        MapBean updatedMap = optional.get();
-        download(updatedMap.getFolderName()).join();
-        uninstallMap(map).join();
-        return updatedMap;
+      return downloadFuture.thenApply(aVoid -> latestMap);
+    }).thenCompose(latestMap -> {
+      CompletableFuture<Void> uninstallFuture;
+      if (!latestMap.getFolderName().equals(map.getFolderName())) {
+        uninstallFuture = uninstallMap(map);
+      } else {
+        uninstallFuture = CompletableFuture.completedFuture(null);
       }
-      return map;
+      return uninstallFuture.thenApply(aVoid -> latestMap);
     });
   }
 

src/main/java/com/faforever/client/remote/FafService.java

@@ -354,8 +354,8 @@ public CompletableFuture<Optional<MapBean>> findMapByFolderName(String folderNam
   }
 
   @Async
-  public CompletableFuture<Optional<MapBean>> getLatestVersionMap(String mapFolderName) {
-    return CompletableFuture.completedFuture(fafApiAccessor.getLatestVersionMap(mapFolderName)
+  public CompletableFuture<Optional<MapBean>> getMapLatestVersion(String mapFolderName) {
+    return CompletableFuture.completedFuture(fafApiAccessor.getMapLatestVersion(mapFolderName)
         .map(MapBean::fromMapVersionDto));
   }
 

src/test/java/com/faforever/client/api/FafApiAccessorImplTest.java

@@ -305,7 +305,7 @@ public void testGetLatestVersionMap() {
     when(restOperations.getForObject(startsWith("/data/mapVersion"), eq(List.class)))
         .thenReturn(Collections.singletonList(mapFromServer));
 
-    assertThat(instance.getLatestVersionMap(localMap.getFolderName()), is(Optional.of(mapFromServer)));
+    assertThat(instance.getMapLatestVersion(localMap.getFolderName()), is(Optional.of(mapFromServer)));
     String parameters = String.format("filter=filename==\"maps/%s.zip\";map.latestVersion.hidden==\"false\"", localMap.getFolderName());
     verify(restOperations).getForObject(contains(parameters), eq(List.class));
   }
@@ -317,7 +317,7 @@ public void testGetLatestVersionMapIfNoMapFromServer() {
     when(restOperations.getForObject(startsWith("/data/mapVersion"), eq(List.class)))
         .thenReturn(emptyList());
 
-    assertThat(instance.getLatestVersionMap(localMap.getFolderName()), is(Optional.empty()));
+    assertThat(instance.getMapLatestVersion(localMap.getFolderName()), is(Optional.empty()));
     String parameters = String.format("filter=filename==\"maps/%s.zip\";map.latestVersion.hidden==\"false\"", localMap.getFolderName());
     verify(restOperations).getForObject(contains(parameters), eq(List.class));
   }

src/test/java/com/faforever/client/game/CreateGameControllerTest.java

@@ -282,7 +282,7 @@ public void testCloseButtonTriggeredAfterCreatingGame() {
     instance.setOnCloseButtonClickedListener(closeAction);
 
     MapBean map = MapBuilder.create().defaultValues().get();
-    when(mapService.updateMapToLatestVersionIfNecessary(map)).thenReturn(completedFuture(map));
+    when(mapService.updateLatestVersionIfNecessary(map)).thenReturn(completedFuture(map));
     when(gameService.hostGame(any())).thenReturn(completedFuture(null));
 
     mapList.add(map);
@@ -303,7 +303,7 @@ public void testCreateGameWithSelectedModAndMap() {
 
     MapBean map = MapBuilder.create().defaultValues().get();
     when(mapService.isOfficialMap(map)).thenReturn(false);
-    when(mapService.updateMapToLatestVersionIfNecessary(map)).thenReturn(completedFuture(map));
+    when(mapService.updateLatestVersionIfNecessary(map)).thenReturn(completedFuture(map));
     when(gameService.hostGame(newGameInfoArgumentCaptor.capture())).thenReturn(completedFuture(null));
 
     mapList.add(map);
@@ -321,7 +321,7 @@ public void testCreateGameOnSelectedMapIfNoNewVersionMap() {
     ArgumentCaptor<NewGameInfo> captor = ArgumentCaptor.forClass(NewGameInfo.class);
     MapBean map = MapBuilder.create().defaultValues().get();
 
-    when(mapService.updateMapToLatestVersionIfNecessary(map)).thenReturn(completedFuture(map));
+    when(mapService.updateLatestVersionIfNecessary(map)).thenReturn(completedFuture(map));
     when(gameService.hostGame(captor.capture())).thenReturn(completedFuture(null));
 
     mapList.add(map);
@@ -338,7 +338,7 @@ public void testCreateGameOnUpdatedMapIfNewVersionMapExist() {
 
     MapBean outdatedMap = MapBuilder.create().defaultValues().folderName("test.v0001").get();
     MapBean updatedMap = MapBuilder.create().defaultValues().folderName("test.v0002").get();
-    when(mapService.updateMapToLatestVersionIfNecessary(outdatedMap)).thenReturn(completedFuture(updatedMap));
+    when(mapService.updateLatestVersionIfNecessary(outdatedMap)).thenReturn(completedFuture(updatedMap));
     when(gameService.hostGame(captor.capture())).thenReturn(completedFuture(null));
 
     mapList.add(outdatedMap);
@@ -370,7 +370,7 @@ public void testCreateGameOnSelectedMapImmediatelyIfThrowExceptionWhenUpdatingMa
     ArgumentCaptor<NewGameInfo> captor = ArgumentCaptor.forClass(NewGameInfo.class);
 
     MapBean map = MapBuilder.create().defaultValues().get();
-    when(mapService.updateMapToLatestVersionIfNecessary(map))
+    when(mapService.updateLatestVersionIfNecessary(map))
         .thenReturn(CompletableFuture.failedFuture(new RuntimeException("error when checking for update or updating map")));
     when(gameService.hostGame(captor.capture())).thenReturn(completedFuture(null));