Review code

6 files changed, 26 insertions, 2 deletions

diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java
index 0c8a2f59..978be4d0 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/ernb/DummyErnbClient.java
@@ -11,6 +11,7 @@ public class DummyErnbClient implements IErnbClient {
 
   @Override
   public ArrayList<RegisterResult> searchWithPersonIdentifer(String personIdentifer) {
+    // NOTE: Are we using Mockito or these fixed strings for testing, why are those defined here?
     switch (personIdentifer) {
       case "a12345":
       case "a12345-":
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java
index 51d6952f..b5e8551b 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/DeSpecificDetailSearchProcessor.java
@@ -10,6 +10,7 @@ public class DeSpecificDetailSearchProcessor extends ICountrySpecificDetailSearc
 
   @Override
   public boolean canHandle(String countryCode, SimpleEidasData eidData) {
+    // NOTE: Please extract constant for "de"
     if (!countryCode.equalsIgnoreCase("de")) {
       return false;
     }
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java
index 3d6b35e9..b9ab2ceb 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/handler/ICountrySpecificDetailSearchProcessor.java
@@ -31,6 +31,7 @@ import org.springframework.beans.factory.annotation.Autowired;
 
 public abstract class ICountrySpecificDetailSearchProcessor {
 
+  // NOTE: Please use constructor injection
 
   protected IErnbClient ernbClient;
   protected IZmrClient zmrClient;
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java
index 1580603c..20ee9325 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/tasks/InitialSearchTask.java
@@ -66,6 +66,8 @@ import java.util.Map;
  */
 @Slf4j
 @Component("InitialSearchTask")
+// NOTE: General: Please rebase git commit and squash them where useful, i.e. "remove unused import" should
+//       not be a separate commit.
 public class InitialSearchTask extends AbstractAuthServletTask {
 
   private List<ICountrySpecificDetailSearchProcessor> handlers = new ArrayList<>();
@@ -92,6 +94,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
       final ILightResponse eidasResponse = authProcessData
           .getGenericDataFromSession(Constants.DATA_FULL_EIDAS_RESPONSE, ILightResponse.class);
 
+      // NOTE: Why is eidas first converted to a map, and then to a SimpleEidasData?
       final Map<String, Object> simpleAttrMap = convertEidasAttrToSimpleMap(
           eidasResponse.getAttributes().getAttributeMap());
 
@@ -106,14 +109,17 @@ public class InitialSearchTask extends AbstractAuthServletTask {
     }
   }
 
+  // NOTE: Please rename methods ... "step2" doesn't tell the reader anything
   private String step2(SimpleEidasData eidData) throws TaskExecutionException {
     String personIdentifier = eidData.getPseudonym();
+    // NOTE: Is that comment really necessary?
     //search in register(step 2)
     MergedRegisterSearchResult result = searchInZmrAndErnp(personIdentifier);
     if (result.getResultCount() == 0) {
       return step5(result, eidData);
     } else if (result.getResultCount() == 1) {
       return step3(result, eidData);
+      // NOTE: Why is that code commented-out?
     } //else if (result.getResultCount() > 1) {
     throw new TaskExecutionException(pendingReq, "Initial search - Kitt Process necessary.",
         new ManualFixNecessaryException(personIdentifier));
@@ -179,7 +185,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
     log.debug("Update " + result + " with " + eidData);
     //TODO
 
-
+    // NOTE: Sometimes the bpk is returned, sometimes "105"?
     return result.getBpk();
   }
 
@@ -265,6 +271,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
 
     log.debug("Automerge " + initialSearchResult + " with " + eidData + " " + mdsSearchResult);
     //TODO
+    // NOTE: Sometimes the bpk is returned, sometimes "105"?
     return "105";
   }
 
@@ -329,6 +336,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
 
   //just for testing
   //TODO is there a nicer solution?
+  // There is: Constructor Injection see https://reflectoring.io/constructor-injection/ or https://www.baeldung.com/constructor-injection-in-spring
   @Autowired
   public void setErnbClient(IErnbClient ernbClient) {
     this.ernbClient = ernbClient;
@@ -342,6 +350,7 @@ public class InitialSearchTask extends AbstractAuthServletTask {
   @Autowired
   public void setHandlers(List<ICountrySpecificDetailSearchProcessor> handlers) {
     this.handlers = handlers;
+    // NOTE: There's a typo in "registrated"
     log.info("# " + handlers.size() + " country specific detail search services are registrated");
   }
 
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java
index 0f3436d8..3af2e39e 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/main/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/zmr/DummyZmrClient.java
@@ -11,6 +11,7 @@ public class DummyZmrClient implements IZmrClient {
 
   @Override
   public ArrayList<RegisterResult> searchWithPersonIdentifer(String personIdentifer) {
+    // NOTE: Are we using Mockito or these fixed strings for testing, why are those defined here?
     switch (personIdentifer) {
       case "a12345":
       case "a12345-":
diff --git a/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java b/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java
index 64a73bda..d366fefc 100644
--- a/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java
+++ b/eidas_modules/authmodule-eIDAS-v2/src/test/java/at/asitplus/eidas/specific/modules/auth/eidas/v2/test/tasks/InitialSearchTaskFirstTest.java
@@ -62,9 +62,11 @@ public class InitialSearchTaskFirstTest {
   @InjectMocks
   private InitialSearchTask task;
 
+  // NOTE: Is defined as @Mock, but also manually mocked in "testNode100a" etc -- why?
   @Mock
   private IZmrClient zmrClient;
 
+  // NOTE: Is defined as @Mock, but also manually mocked in "testNode100a" etc -- why?
   @Mock
   private IErnbClient ernbClient;
 
@@ -92,7 +94,7 @@ public class InitialSearchTaskFirstTest {
    */
   @Before
   public void setUp() throws URISyntaxException, EaafStorageException {
-
+    // NOTE: PowerMockito should not be needed, as we don't want to test static and private methods
     task = PowerMockito.spy(task);
 
     httpReq = new MockHttpServletRequest("POST", "https://localhost/authhandler");
@@ -112,6 +114,7 @@ public class InitialSearchTaskFirstTest {
   /**
    * One match, but register update needed
    */
+  // NOTE: Why is the method named "testNode100a"?
   public void testNode100a() throws Exception {
 
     //Mock ZMR
@@ -119,6 +122,9 @@ public class InitialSearchTaskFirstTest {
     String randomBpk = RandomStringUtils.randomNumeric(6);
     zmrResult.add(new RegisterResult(randomBpk,"de/st/max123", "Max_new", "Mustermann", "2011-01-01"));
 
+    // NOTE: Are we using Mockito or these fixed strings in DummyZmrClient?
+    // NOTE: Please mock an interface, not a concrete class
+    // NOTE: But DummyZmrClient is also defined as a bean "ZmrClientForeIDAS" in "eidas_v2_auth.beans.xml"?
     zmrClient = Mockito.mock(DummyZmrClient.class);
     Mockito.when(zmrClient.searchWithPersonIdentifer("max123")).thenReturn(zmrResult);//"de/st/max123"???
     task.setZmrClient(zmrClient);
@@ -137,11 +143,13 @@ public class InitialSearchTaskFirstTest {
       Assert.assertTrue("Wrong bpk", bPk.equals(randomBpk));
 
     } catch (final TaskExecutionException e) {
+      // NOTE: assertTrue is probably the wrong method to use ... why catch the exception anyway?
       Assert.assertTrue("Wrong workflow, should not reach this point", false);
     }
   }
 
   @Test
+  // NOTE: Why is @DirtiesContext after each test necessary? What is changed in the context and why?
   @DirtiesContext
   /**
    * One match, but register update needed
@@ -563,6 +571,7 @@ public class InitialSearchTaskFirstTest {
 
       String bPk = (String)
           pendingReq.getSessionData(AuthProcessDataWrapper.class).getGenericDataFromSession(Constants.DATA_RESULT_MATCHING_BPK);
+      // NOTE: Why "105"? Extract in a constant
       Assert.assertTrue("Wrong bpk", bPk.equals("105"));
     } catch (final TaskExecutionException e) {
       Assert.assertTrue("Wrong workflow, should not reach this point", false);
@@ -578,6 +587,8 @@ public class InitialSearchTaskFirstTest {
 
   @NotNull
   private AuthenticationResponse buildDummyAuthResponseMaxMustermann() throws URISyntaxException {
+    // NOTE: Those strings "de/st/max123" seem to be somehow relevant, but where do we need to use that exact string again?
+    // NOTE: If not, why not using random strings?
     return buildDummyAuthResponse("Max", "Mustermann",
         "de/st/max123", "2011-01-01");
   }