Remove dependency cycle in ManigestRepositoryImpl

Change-Id: I20b21ba6fcb418a41a5fbb34949cf348d0c432cf

src/main/java/eu/erasmuswithoutpaper/registry/repository/ManifestRepository.java

@@ -2,6 +2,7 @@
 
 import java.util.List;
 
+import eu.erasmuswithoutpaper.registryclient.RegistryClient;
 import org.springframework.beans.factory.DisposableBean;
 
 import org.eclipse.jgit.api.errors.GitAPIException;
@@ -151,7 +152,7 @@ class ConfigurationException extends Exception {
    * @param contents A string with the catalogue contents to be stored.
    * @return <b>true</b> if the new content differs from the previous one.
    */
-  boolean putCatalogue(String contents);
+  boolean putCatalogue(String contents, RegistryClient client);
 
   /**
    * Store a new filtered version of the manifest.

src/main/java/eu/erasmuswithoutpaper/registry/repository/ManifestRepositoryImpl.java

@@ -27,7 +27,6 @@
 import eu.erasmuswithoutpaper.registryclient.RegistryClient.RefreshFailureException;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication;
-import org.springframework.context.annotation.Lazy;
 import org.springframework.stereotype.Service;
 
 import com.google.common.collect.Iterables;
@@ -60,17 +59,13 @@
  */
 @Service
 @SuppressFBWarnings("SIC_INNER_SHOULD_BE_STATIC_ANON")
-// FIXME @Lazy is for preventing cycle in dependencies with RealInternet i production profile.
-// This should be unraveled.
-@Lazy
 @ConditionalOnWebApplication
 public class ManifestRepositoryImpl implements ManifestRepository {
 
   private static final Logger logger = LoggerFactory.getLogger(ManifestRepositoryImpl.class);
 
   private final ManifestRepositoryImplProperties repoProperties;
   private final CatalogueDependantCache catcache;
-  private RegistryClient client = null;
   private final Git git;
 
   private final ReentrantReadWriteLock lock;
@@ -105,7 +100,6 @@ public ManifestRepositoryImpl(ManifestRepositoryImplProperties repoProperties,
       this.index = this.loadIndex().get();
     } else {
       this.index = new TreeSet<>();
-      this.deleteAll();
       this.flushIndex();
       this.commit("Upgrade repository structure");
     }
@@ -158,7 +152,7 @@ public boolean commit(String message) {
    * Remember to {@link #commit(String)} the changes afterwards, for logging purposes.
    * </p>
    */
-  public final void deleteAll() {
+  public final void deleteAll(RegistryClient client) {
     this.lock.writeLock().lock();
     try {
       Path path = this.repoProperties.getFileSystem().getPath(this.repoProperties.getPath());
@@ -202,7 +196,7 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOExce
       this.cachedCatalogueContent = null;
       this.index.clear();
       this.flushIndex();
-      this.onCatalogueContentChanged();
+      this.onCatalogueContentChanged(client);
     } catch (IOException e) {
       throw new RuntimeException(e);
     } finally {
@@ -393,13 +387,13 @@ public boolean push() throws TransportException, GitAPIException, ConfigurationE
   }
 
   @Override
-  public boolean putCatalogue(String contents) {
+  public boolean putCatalogue(String contents, RegistryClient client) {
     this.lock.writeLock().lock();
     try {
       boolean changed = this.writeFile(this.getPathForCatalogue(), contents);
       this.cachedCatalogueContent = contents;
       if (changed) {
-        this.onCatalogueContentChanged();
+        this.onCatalogueContentChanged(client);
       }
       return changed;
     } finally {
@@ -443,18 +437,6 @@ public void releaseWriteLock() {
     this.lock.writeLock().unlock();
   }
 
-  /**
-   * Once set, {@link ManifestRepositoryImpl} will refresh this {@link RegistryClient} whenever the
-   * catalogue is changed.
-   *
-   * @param client The client to be kept in sync.
-   */
-  @Autowired
-  public void setRegistryClient(RegistryClient client) {
-    this.client = client;
-    this.onCatalogueContentChanged();
-  }
-
   private void addToIndex(String url) {
     this.lock.writeLock().lock();
     try {
@@ -526,14 +508,12 @@ private Optional<SortedSet<String>> loadIndex() {
     return Optional.of(result);
   }
 
-  private void onCatalogueContentChanged() {
+  private void onCatalogueContentChanged(RegistryClient client) {
     this.catcache.clear();
-    if (this.client != null) {
-      try {
-        this.client.refresh();
-      } catch (RefreshFailureException e) {
-        logger.error("Local registry client refresh failed: " + e);
-      }
+    try {
+      client.refresh();
+    } catch (RefreshFailureException e) {
+      logger.error("Local registry client refresh failed: " + e);
     }
   }
 

src/main/java/eu/erasmuswithoutpaper/registry/updater/RegistryUpdaterImpl.java

@@ -437,7 +437,7 @@ private void updateTheCatalogue(boolean commit) {
 
       // Store it.
 
-      this.repo.putCatalogue(catalogueXml);
+      this.repo.putCatalogue(catalogueXml, registryClient);
       if (commit) {
         this.repo.commit("Update catalogue");
       }

src/test/java/eu/erasmuswithoutpaper/registry/repository/ManifestRepositoryTest.java

@@ -6,6 +6,7 @@
 import java.nio.charset.StandardCharsets;
 
 import eu.erasmuswithoutpaper.registry.WRTest;
+import eu.erasmuswithoutpaper.registryclient.RegistryClient;
 import org.springframework.beans.factory.annotation.Autowired;
 
 import org.junit.jupiter.api.AfterEach;
@@ -26,6 +27,9 @@ public static void setUpClass() {
     manifestUrl2 = "https://example.com/manifest2.xml";
   }
 
+  @Autowired
+  private RegistryClient client;
+
   @Autowired
   private ManifestRepositoryImpl repo;
 
@@ -34,26 +38,26 @@ public static void setUpClass() {
 
   @AfterEach
   public void tearDown() {
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
   }
 
   @Test
   public void testCatalogueDependantCache() {
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
 
-    this.repo.putCatalogue("1");
+    this.repo.putCatalogue("1", client);
     this.catcache.putCoverageMatrixHtml("1");
     assertThat(this.catcache.getCoverageMatrixHtml()).isEqualTo("1");
 
-    this.repo.putCatalogue("1");
+    this.repo.putCatalogue("1", client);
     assertThat(this.catcache.getCoverageMatrixHtml()).isEqualTo("1");
 
-    this.repo.putCatalogue("2");
+    this.repo.putCatalogue("2", client);
     assertThat(this.catcache.getCoverageMatrixHtml()).isNull();
     this.catcache.putCoverageMatrixHtml("2");
     assertThat(this.catcache.getCoverageMatrixHtml()).isEqualTo("2");
 
-    this.repo.putCatalogue("1");
+    this.repo.putCatalogue("1", client);
     assertThat(this.catcache.getCoverageMatrixHtml()).isNull();
   }
 
@@ -63,7 +67,7 @@ public void testCatalogueDependantCache() {
   @Test
   public void testCatalogueStorage() {
 
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
 
     // No catalogue should exist before this test is run.
 
@@ -84,10 +88,10 @@ public void testCatalogueStorage() {
     // should accept any string here.
 
     this.repo.commit("commit things which were changed before");
-    this.repo.putCatalogue("a string with the catalogue");
+    this.repo.putCatalogue("a string with the catalogue", client);
     boolean r = this.repo.commit("commit the catalogue");
     assertThat(r).isTrue();
-    this.repo.putCatalogue("a string with the catalogue");
+    this.repo.putCatalogue("a string with the catalogue", client);
     r = this.repo.commit("try to commit unchanged catalogue");
     assertThat(r).isFalse();
 
@@ -107,14 +111,14 @@ public void testCatalogueStorage() {
    * inconsistent.
    */
   public void testDeleteAll() {
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
     assertThat(this.repo.getAllFilePaths()).containsExactly("index.xml");
     this.repo.putOriginalManifest(manifestUrl1, "some string".getBytes(StandardCharsets.UTF_8));
     this.repo.putFilteredManifest(manifestUrl2, "some string");
     assertThat(this.repo.getAllFilePaths()).containsExactlyInAnyOrder("index.xml",
         "manifests/com/example.com/51d9ca82ce863381a7648647ab688b966f3b2260-filtered.xml",
         "manifests/com/example.com/bb937788ce84767ff64935e70c3856bd8c7bd16d.xml");
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
     assertThat(this.repo.getAllFilePaths()).containsExactly("index.xml");
   }
 
@@ -125,7 +129,7 @@ public void testDeleteAll() {
   @Test
   public void testManifestStorage() {
 
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
 
     // Attempt to get a nonexistent manifest from the repo.
 

src/test/java/eu/erasmuswithoutpaper/registry/updater/RegistryUpdaterTest.java

@@ -139,7 +139,7 @@ public void checkIfOfficialExamplesAreConsistent() {
   @BeforeEach
   public void setUp() {
     this.sourceProvider.clearSources();
-    this.repo.deleteAll();
+    this.repo.deleteAll(regClient);
     this.internet.clearURLs();
     this.internet.clearEmailsSent();
     this.lastCatalogue = null;

src/test/java/eu/erasmuswithoutpaper/registry/validators/AbstractApiTest.java

@@ -141,7 +141,7 @@ public void setUp() {
        * consistent with the certificates returned by the validator.
        */
       this.sourceProvider.clearSources();
-      this.repo.deleteAll();
+      this.repo.deleteAll(client);
       this.internet.clearAll();
 
       Map<String,String> registryManifests = this.selfManifestProvider.getManifests();

src/test/java/eu/erasmuswithoutpaper/registry/web/ApiControllerIntegrationTest.java

@@ -16,7 +16,7 @@
 import eu.erasmuswithoutpaper.registry.sourceprovider.ManifestSourceFactory;
 import eu.erasmuswithoutpaper.registry.sourceprovider.TestManifestSourceProvider;
 import eu.erasmuswithoutpaper.registry.updater.RegistryUpdaterTest;
-
+import eu.erasmuswithoutpaper.registryclient.RegistryClient;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.web.client.TestRestTemplate;
 import org.springframework.http.HttpStatus;
@@ -41,6 +41,9 @@ public class ApiControllerIntegrationTest extends WRIntegrationTest {
   @Autowired
   private EwpDocBuilder docBuilder;
 
+  @Autowired
+  private RegistryClient client;
+
   @Autowired
   private ManifestRepositoryImpl repo;
 
@@ -104,7 +107,7 @@ public void respondsValid500() {
    */
   @Test
   public void servesTheCatalogue() {
-    this.repo.putCatalogue("<xml/>");
+    this.repo.putCatalogue("<xml/>", client);
     ResponseEntity<String> response =
         this.template.getForEntity(this.baseURL + "/catalogue-v1.xml", String.class);
     assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
@@ -152,7 +155,7 @@ public void testScenario1() {
         Lists.newArrayList(new RestrictInstitutionsCovered("^uw\\.edu\\.pl$"))));
     this.manifestSourceProvider.addSource(manifestFactory.newRegularSource(urlSE,
         Lists.newArrayList(new RestrictInstitutionsCovered("^.+\\.se$"))));
-    this.repo.deleteAll();
+    this.repo.deleteAll(client);
     // this.internet.putURL(urlSelf, this.apiController.getSelfManifest().getBody());
 
     /* Call the refresh URL and verify its response status. */
@@ -267,7 +270,7 @@ public void testScenario1() {
   }
 
   private int forceReload(String manifestUrl) {
-    MultiValueMap<String, String> params = new LinkedMultiValueMap<String, String>();
+    MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
     params.add("url", manifestUrl);
     ResponseEntity<String> response =
         this.template.postForEntity(this.baseURL + "/reload", params, String.class);