Refactoring (mageddo#604)

* revert

* refactoring

* refactoring test

* breaking tests

Author: mageddo

build.gradle

@@ -68,8 +68,8 @@ dependencies {
   implementation('dev.failsafe:failsafe:3.3.1')
   implementation("io.github.resilience4j:resilience4j-circuitbreaker:2.2.0")
 
-  implementation('com.github.docker-java:docker-java-core:3.4.0')
-  implementation('com.github.docker-java:docker-java-transport-httpclient5:3.4.0')
+  implementation('com.github.docker-java:docker-java-core:3.3.4')
+  implementation('com.github.docker-java:docker-java-transport-httpclient5:3.3.4')
 
   implementation('info.picocli:picocli:4.7.6')
   implementation('com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.17.1')

src/main/java/com/mageddo/dnsproxyserver/docker/dataprovider/ContainerFacade.java

@@ -4,13 +4,17 @@
 import com.github.dockerjava.api.model.Container;
 
 import java.util.List;
-import java.util.Optional;
+import java.util.stream.Stream;
 
 public interface ContainerFacade {
 
   Container findById(String containerId);
 
   List<Container> findActiveContainers();
 
-  Optional<InspectContainerResponse> inspect(String id);
+  InspectContainerResponse inspect(String id);
+
+  InspectContainerResponse safeInspect(String id);
+
+  Stream<InspectContainerResponse> inspectFilteringValidContainers(List<Container> containers);
 }

src/main/java/com/mageddo/dnsproxyserver/docker/dataprovider/ContainerFacadeDefault.java

@@ -13,7 +13,8 @@
 import javax.inject.Singleton;
 import java.util.Collections;
 import java.util.List;
-import java.util.Optional;
+import java.util.Objects;
+import java.util.stream.Stream;
 
 @Slf4j
 @Default
@@ -46,15 +47,29 @@ public List<Container> findActiveContainers() {
   }
 
   @Override
-  public Optional<InspectContainerResponse> inspect(String id) {
+  public InspectContainerResponse inspect(String id) {
+    return this.dockerClient.inspectContainerCmd(id).exec();
+  }
+
+  @Override
+  public InspectContainerResponse safeInspect(String id) {
     try {
-      return Optional.of(this.dockerClient.inspectContainerCmd(id).exec());
+      return this.inspect(id);
     } catch (NotFoundException e) {
       log.warn("status=container-not-found, action=inspect-container, containerId={}", id);
     } catch (Exception e) {
       log.warn("status=unexpected-exception, action=inspect-container, containerId={}, msg={}", id, e.getMessage(), e);
     }
+    return null;
+  }
 
-    return Optional.empty();
+  @Override
+  public Stream<InspectContainerResponse> inspectFilteringValidContainers(List<Container> containers) {
+    return containers
+      .stream()
+      .map(com.github.dockerjava.api.model.Container::getId)
+      .map(this::safeInspect)
+      .filter(Objects::nonNull)
+      ;
   }
 }

src/main/java/com/mageddo/dnsproxyserver/solver/docker/dataprovider/ContainerDAODefault.java

@@ -26,9 +26,8 @@ public class ContainerDAODefault implements ContainerDAO {
 
   @Override
   public List<Container> findActiveContainersMatching(HostnameQuery query) {
-    return this.containerFacade.findActiveContainers()
-      .stream()
-      .flatMap(it -> this.containerFacade.inspect(it.getId()).stream())
+    final var containersToFilter = this.containerFacade.findActiveContainers();
+    return this.containerFacade.inspectFilteringValidContainers(containersToFilter)
       .filter(ContainerHostnameMatcher.buildPredicate(query))
       .map(ContainerMapper::of)
       .toList();

src/main/java/com/mageddo/dnsproxyserver/solver/docker/dataprovider/DpsContainerDAODefault.java

@@ -52,10 +52,8 @@ public Container findDPSContainer() {
     } else {
       log.debug("dpsContainersFound={}", containers.size());
     }
-    return containers
-      .stream()
+    return this.containerFacade.inspectFilteringValidContainers(containers)
       .findFirst()
-      .flatMap(it -> this.containerFacade.inspect(it.getId()))
       .map(ContainerMapper::of)
       .orElse(null);
   }

src/test/java/com/mageddo/dnsproxyserver/docker/ContainerFacadeDefaultTest.java renamed to src/test/java/com/mageddo/dnsproxyserver/docker/dataprovider/ContainerFacadeDefaultCompTest.java

@@ -1,28 +1,24 @@
-package com.mageddo.dnsproxyserver.docker;
+package com.mageddo.dnsproxyserver.docker.dataprovider;
 
 import com.github.dockerjava.api.DockerClient;
-import com.github.dockerjava.api.exception.NotFoundException;
 import com.github.dockerjava.api.model.Container;
-import com.mageddo.dnsproxyserver.docker.dataprovider.ContainerFacadeDefault;
-import testing.templates.docker.ContainerTemplates;
-import testing.templates.docker.DockerClientTemplates;
-
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.junit.jupiter.MockitoExtension;
+import testing.templates.docker.ContainerTemplates;
+import testing.templates.docker.DockerClientTemplates;
 import testing.templates.docker.InspectContainerResponseTemplates;
 
 import java.util.ArrayList;
-import java.util.Optional;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.spy;
 
 @ExtendWith(MockitoExtension.class)
-class ContainerFacadeDefaultTest {
+class ContainerFacadeDefaultCompTest {
 
   ContainerFacadeDefault dao;
 
@@ -31,7 +27,7 @@ class ContainerFacadeDefaultTest {
   @BeforeEach
   void before(){
     this.dockerClient = DockerClientTemplates.buildSpy();
-    this.dao = new ContainerFacadeDefault(this.dockerClient);
+    this.dao = spy(new ContainerFacadeDefault(this.dockerClient));
   }
 
   @Test
@@ -108,45 +104,11 @@ void mustInspectContainerById(){
     ;
 
     // act
-    final var inspectResponse = this.dao.inspect(containerId).orElseThrow();
+    final var inspectResponse = this.dao.inspect(containerId);
 
     // assert
     assertEquals(expected, inspectResponse);
   }
 
-  @Test
-  void mustNotThrowErrorWhenInspectContainerNotFound(){
-    // arrange
-    final var containerId = "a39bba9a8bab2899";
 
-    final var inspectContainerCmd = this.dockerClient.inspectContainerCmd(containerId);
-    doThrow(new NotFoundException("Container not found"))
-      .when(inspectContainerCmd)
-      .exec()
-    ;
-
-    // act
-    final var container = this.dao.inspect(containerId);
-
-    // assert
-    assertEquals(Optional.empty(), container);
-  }
-
-  @Test
-  void mustNotThrowErrorWhenInspectContainerFails(){
-    // arrange
-    final var containerId = "a39bba9a8bab28aa";
-
-    final var inspectContainerCmd = this.dockerClient.inspectContainerCmd(containerId);
-    doThrow(new NullPointerException("Unexpected failure"))
-      .when(inspectContainerCmd)
-      .exec()
-    ;
-
-    // act
-    final var container = this.dao.inspect(containerId);
-
-    // assert
-    assertEquals(Optional.empty(), container);
-  }
 }

src/test/java/com/mageddo/dnsproxyserver/docker/dataprovider/ContainerFacadeDefaultTest.java

@@ -0,0 +1,77 @@
+package com.mageddo.dnsproxyserver.docker.dataprovider;
+
+import com.github.dockerjava.api.exception.NotFoundException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Spy;
+import org.mockito.junit.jupiter.MockitoExtension;
+import testing.templates.docker.ContainerTemplates;
+import testing.templates.docker.InspectContainerResponseTemplates;
+
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+
+@ExtendWith(MockitoExtension.class)
+class ContainerFacadeDefaultTest {
+
+  @Spy
+  @InjectMocks
+  ContainerFacadeDefault facade;
+
+  @Test
+  void mustNotThrowErrorWhenInspectContainerGetNotFound() {
+    // arrange
+    final var containerId = "a39bba9a8bab2899";
+
+    doThrow(new NotFoundException("Container not found"))
+      .when(this.facade).inspect(containerId)
+    ;
+
+    // act
+    final var container = this.facade.safeInspect(containerId);
+
+    // assert
+    assertNull(container);
+  }
+
+  @Test
+  void mustNotThrowErrorWhenInspectContainerFails() {
+    // arrange
+    final var containerId = "a39bba9a8bab28aa";
+
+    doThrow(new NullPointerException("Unexpected failure"))
+      .when(this.facade).inspect(containerId)
+    ;
+
+    // act
+    final var container = this.facade.safeInspect(containerId);
+
+    // assert
+    assertNull(container);
+  }
+
+  @Test
+  void mustFilterNullContainerInspections() {
+    final var c1 = ContainerTemplates.buildRegularContainerCoffeeMakerCheckout();
+    final var c2 = ContainerTemplates.buildDpsContainer();
+    final var containers = List.of(c1, c2);
+
+    doReturn(InspectContainerResponseTemplates.withDpsLabel())
+      .when(this.facade).safeInspect(c1.getId())
+    ;
+
+    doReturn(null)
+      .when(this.facade).safeInspect(c2.getId())
+    ;
+
+    final var filtered = this.facade.inspectFilteringValidContainers(containers)
+      .toList();
+
+    assertEquals(1, filtered.size());
+  }
+}