Repository: urlaubsverwaltung/urlaubsverwaltung
Pull request #4747 (2024-06-21 09:46:43)

Unify handler interface cleanup
<!--

Thanks for contributing to the Urlaubsverwaltung.
Please review the following notes before submitting you pull request.

Please look for other issues or pull requests which already work on this topic. Is somebody already on it? Do you need to synchronize?

# Security Vulnerabilities

🛑 STOP! 🛑 If your contribution fixes a security vulnerability, please do not submit it.
Instead, please open a report of a security vulnerability via
[Report a security vulnerability](https://github.com/urlaubsverwaltung/urlaubsverwaltung/security/advisories/new)


# Describing Your Changes

If, having reviewed the notes above, you're ready to submit your pull request, please
provide a brief description of the proposed changes.

If they:
 🐞 fix a bug, please describe the broken behaviour and how the changes fix it.
    Please label with 'type: bug' and 'status: new'
    
 🎁 make an enhancement, please describe the new functionality and why you believe it's useful.
    Please label with 'type: enhancement' and 'status: new'
 
If your pull request relates to any existing issues,
please reference them by using the issue number prefixed with #.

-->


sonarcloud[bot]: ## [![Quality Gate Failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-failed-20px.png 'Quality Gate Failed')](https://sonarcloud.io/dashboard?id=urlaubsverwaltung_urlaubsverwaltung&pullRequest=4747) **Quality Gate failed**  
Failed conditions  
![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/failed-16px.png '') [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=urlaubsverwaltung_urlaubsverwaltung&pullRequest=4747) (required ≥ A)  
  
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=urlaubsverwaltung_urlaubsverwaltung&pullRequest=4747)

##   
![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/light_bulb-16px.png '') Catch issues before they fail your Quality Gate with our IDE extension ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/sonarlint-16px.png '') [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)



Patch:
diff --git a/src/main/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdvice.java b/src/main/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdvice.java
index b3da12fb2..1626e52ef 100644
--- a/src/main/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdvice.java
+++ b/src/main/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdvice.java
@@ -3,11 +3,10 @@ package org.synyx.urlaubsverwaltung.infobanner;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import org.springframework.lang.NonNull;
-import org.springframework.lang.Nullable;
-import org.springframework.web.servlet.HandlerInterceptor;
 import org.springframework.web.servlet.ModelAndView;
+import org.synyx.urlaubsverwaltung.web.DataProviderInterface;
 
-class InfoBannerControllerAdvice implements HandlerInterceptor {
+class InfoBannerControllerAdvice implements DataProviderInterface {
 
     private final InfoBannerConfigProperties properties;
 
@@ -16,15 +15,9 @@ class InfoBannerControllerAdvice implements HandlerInterceptor {
     }
 
     @Override
-    public void postHandle(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler, @Nullable ModelAndView modelAndView) throws Exception {
-
-        if (modelAndView != null && modelAndView.hasView() && !redirectOrForward(modelAndView)) {
+    public void postHandle(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler, ModelAndView modelAndView) {
+        if (addDataIf(modelAndView)) {
             modelAndView.getModelMap().addAttribute("infoBannerText", properties.text().de());
         }
     }
-
-    private static boolean redirectOrForward(ModelAndView modelAndView) {
-        final String viewName = modelAndView.getViewName();
-        return viewName != null && (viewName.startsWith("redirect") || viewName.startsWith("forward"));
-    }
 }
diff --git a/src/main/java/org/synyx/urlaubsverwaltung/user/LocaleModelInterceptor.java b/src/main/java/org/synyx/urlaubsverwaltung/user/LocaleModelInterceptor.java
index 661cb2b73..a340b2ac3 100644
--- a/src/main/java/org/synyx/urlaubsverwaltung/user/LocaleModelInterceptor.java
+++ b/src/main/java/org/synyx/urlaubsverwaltung/user/LocaleModelInterceptor.java
@@ -5,35 +5,20 @@ import jakarta.servlet.http.HttpServletResponse;
 import org.springframework.context.i18n.LocaleContextHolder;
 import org.springframework.lang.NonNull;
 import org.springframework.stereotype.Component;
-import org.springframework.web.servlet.HandlerInterceptor;
 import org.springframework.web.servlet.ModelAndView;
+import org.synyx.urlaubsverwaltung.web.DataProviderInterface;
 
 import java.util.Locale;
 
 @Component
-class LocaleModelInterceptor implements HandlerInterceptor {
+class LocaleModelInterceptor implements DataProviderInterface {
 
     @Override
     public void postHandle(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler, ModelAndView modelAndView) {
-        if (attributeRequired(modelAndView)) {
+        if (addDataIf(modelAndView)) {
             final Locale currentLocale = LocaleContextHolder.getLocale();
             modelAndView.addObject("locale", currentLocale);
             modelAndView.addObject("language", currentLocale.toLanguageTag());
         }
     }
-
-    private boolean attributeRequired(ModelAndView modelAndView) {
-
-        if (modelAndView == null) {
-            return false;
-        }
-
-        final String viewName = modelAndView.getViewName();
-        if (viewName == null) {
-            return false;
-        }
-
-        return !viewName.startsWith("forward:") &&
-            !viewName.startsWith("redirect:");
-    }
 }
diff --git a/src/main/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProvider.java b/src/main/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProvider.java
index d910dff7d..61e36b68d 100644
--- a/src/main/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProvider.java
+++ b/src/main/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProvider.java
@@ -4,14 +4,14 @@ import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import org.springframework.lang.NonNull;
 import org.springframework.stereotype.Component;
-import org.springframework.web.servlet.HandlerInterceptor;
 import org.springframework.web.servlet.ModelAndView;
+import org.synyx.urlaubsverwaltung.web.DataProviderInterface;
 
 import java.security.Principal;
 import java.util.Optional;
 
 @Component
-public class UserThemeDataProvider implements HandlerInterceptor {
+public class UserThemeDataProvider implements DataProviderInterface {
 
     private final UserSettingsServiceImpl userSettingsService;
 
@@ -21,7 +21,7 @@ public class UserThemeDataProvider implements HandlerInterceptor {
 
     @Override
     public void postHandle(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler, ModelAndView modelAndView) {
-        if (themeIsNeeded(modelAndView)) {
+        if (addDataIf(modelAndView)) {
 
             final Principal userPrincipal = request.getUserPrincipal();
 
@@ -32,21 +32,6 @@ public class UserThemeDataProvider implements HandlerInterceptor {
         }
     }
 
-    private boolean themeIsNeeded(ModelAndView modelAndView) {
-
-        if (modelAndView == null) {
-            return false;
-        }
-
-        final String viewName = modelAndView.getViewName();
-        if (viewName == null) {
-            return false;
-        }
-
-        return !viewName.startsWith("forward:") &&
-            !viewName.startsWith("redirect:");
-    }
-
     private Optional<Theme> getTheme(Principal principal) {
         final String username = principal.getName();
         return userSettingsService.findThemeForUsername(username);
diff --git a/src/main/java/org/synyx/urlaubsverwaltung/web/DataProviderInterface.java b/src/main/java/org/synyx/urlaubsverwaltung/web/DataProviderInterface.java
new file mode 100644
index 000000000..79ab126d1
--- /dev/null
+++ b/src/main/java/org/synyx/urlaubsverwaltung/web/DataProviderInterface.java
@@ -0,0 +1,22 @@
+package org.synyx.urlaubsverwaltung.web;
+
+import org.springframework.web.servlet.HandlerInterceptor;
+import org.springframework.web.servlet.ModelAndView;
+
+public interface DataProviderInterface extends HandlerInterceptor {
+
+    default boolean addDataIf(ModelAndView modelAndView) {
+
+        if (modelAndView == null) {
+            return false;
+        }
+
+        final String viewName = modelAndView.getViewName();
+        if (viewName == null) {
+            return false;
+        }
+
+        return !viewName.startsWith("forward:")
+            && !viewName.startsWith("redirect:");
+    }
+}
diff --git a/src/main/java/org/synyx/urlaubsverwaltung/web/FrameDataProvider.java b/src/main/java/org/synyx/urlaubsverwaltung/web/FrameDataProvider.java
index 3c56e5b7f..2f2cf23ee 100644
--- a/src/main/java/org/synyx/urlaubsverwaltung/web/FrameDataProvider.java
+++ b/src/main/java/org/synyx/urlaubsverwaltung/web/FrameDataProvider.java
@@ -6,7 +6,6 @@ import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.lang.NonNull;
 import org.springframework.stereotype.Component;
-import org.springframework.web.servlet.HandlerInterceptor;
 import org.springframework.web.servlet.ModelAndView;
 import org.synyx.urlaubsverwaltung.overtime.OvertimeSettings;
 import org.synyx.urlaubsverwaltung.person.Person;
@@ -27,7 +26,7 @@ import static org.synyx.urlaubsverwaltung.person.Role.SICK_NOTE_VIEW;
  * Interceptor to add menu specific information to all requests
  */
 @Component
-public class FrameDataProvider implements HandlerInterceptor {
+public class FrameDataProvider implements DataProviderInterface {
 
     private final PersonService personService;
     private final SettingsService settingsService;
@@ -46,7 +45,7 @@ public class FrameDataProvider implements HandlerInterceptor {
     @Override
     public void postHandle(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler, ModelAndView modelAndView) {
 
-        if (modelAndView != null && menuIsShown(modelAndView)) {
+        if (addDataIf(modelAndView)) {
 
             final Person signedInUserInModel = (Person) modelAndView.getModelMap().get("signedInUser");
             final Person user = Objects.requireNonNullElseGet(signedInUserInModel, personService::getSignedInUser);
@@ -104,18 +103,6 @@ public class FrameDataProvider implements HandlerInterceptor {
         return new NavigationDto(elements);
     }
 
-    private boolean menuIsShown(ModelAndView modelAndView) {
-
-        final String viewName = modelAndView.getViewName();
-        if (viewName == null) {
-            return false;
-        }
-
-        return !viewName.startsWith("forward:")
-            && !viewName.startsWith("redirect:")
-            && !viewName.startsWith("login");
-    }
-
     private boolean popupMenuEnabled(Person signedInUser) {
         return signedInUser.hasRole(OFFICE) || overtimeEnabled(signedInUser) || isAllowedToAddOrSubmitSickNote(signedInUser);
     }
diff --git a/src/main/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProvider.java b/src/main/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProvider.java
index f546c3714..c51d18d3a 100644
--- a/src/main/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProvider.java
+++ b/src/main/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProvider.java
@@ -2,8 +2,8 @@ package org.synyx.urlaubsverwaltung.web;
 
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
+import org.springframework.lang.NonNull;
 import org.springframework.stereotype.Component;
-import org.springframework.web.servlet.HandlerInterceptor;
 import org.springframework.web.servlet.ModelAndView;
 import org.synyx.urlaubsverwaltung.web.html.PreloadLink;
 
@@ -13,7 +13,7 @@ import java.util.Map;
 import static java.util.stream.Collectors.toMap;
 
 @Component
-class PreloadAssetProvider implements HandlerInterceptor {
+class PreloadAssetProvider implements DataProviderInterface {
 
     private final AssetManifestService assetManifestService;
 
@@ -22,8 +22,8 @@ class PreloadAssetProvider implements HandlerInterceptor {
     }
 
     @Override
-    public void postHandle(HttpServletRequest request, HttpServletResponse response, Object handler, ModelAndView modelAndView) throws Exception {
-        if (modelAndView != null && shouldAddAssets(modelAndView)) {
+    public void postHandle(@NonNull HttpServletRequest request, @NonNull HttpServletResponse response, @NonNull Object handler, ModelAndView modelAndView) {
+        if (addDataIf(modelAndView)) {
             final Map<String, List<PreloadLink>> assets = assetManifestService.getAssets(request.getContextPath())
                 .entrySet()
                 .stream()
@@ -36,14 +36,4 @@ class PreloadAssetProvider implements HandlerInterceptor {
     private static List<PreloadLink> toPreloadAsset(Asset asset) {
         return asset.getDependencies().stream().map(d -> new PreloadLink("script", d)).toList();
     }
-
-    private boolean shouldAddAssets(ModelAndView modelAndView) {
-
-        final String viewName = modelAndView.getViewName();
-        if (viewName == null) {
-            return false;
-        }
-
-        return !viewName.startsWith("redirect:");
-    }
 }


Test patch:
diff --git a/src/test/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdviceTest.java b/src/test/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdviceTest.java
index 03cd1e0e2..8c488d6ce 100644
--- a/src/test/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdviceTest.java
+++ b/src/test/java/org/synyx/urlaubsverwaltung/infobanner/InfoBannerControllerAdviceTest.java
@@ -29,15 +29,15 @@ class InfoBannerControllerAdviceTest {
     }
 
     @Test
-    void ensureModelAttributeIsNotSetWhenModelAndViewHasNoView() throws Exception {
+    void ensureModelAttributeIsNotSetWhenModelAndViewHasNoView() {
         final ModelAndView modelAndView = new ModelAndView();
         sut.postHandle(null, null, null, modelAndView);
         assertThat(modelAndView.getModel()).doesNotContainEntry("infoBannerText", "info text");
     }
 
     @ParameterizedTest
-    @ValueSource(strings = {"redirect", "forward"})
-    void ensureModelAttributeIsNotSetWhenViewNameStartsWith(String prefix) throws Exception {
+    @ValueSource(strings = {"forward::view-name", "redirect::view-name"})
+    void ensureModelAttributeIsNotSetWhenViewNameStartsWith(String prefix) {
 
         final ModelAndView modelAndView = new ModelAndView();
         modelAndView.setViewName(prefix);
@@ -47,7 +47,7 @@ class InfoBannerControllerAdviceTest {
     }
 
     @Test
-    void ensureModelAttribute() throws Exception {
+    void ensureModelAttribute() {
         final ModelAndView modelAndView = new ModelAndView("any-viewname");
         sut.postHandle(null, null, null, modelAndView);
         assertThat(modelAndView.getModel()).containsEntry("infoBannerText", "info text");
diff --git a/src/test/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProviderTest.java b/src/test/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProviderTest.java
index 273ca7f28..5daf27362 100644
--- a/src/test/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProviderTest.java
+++ b/src/test/java/org/synyx/urlaubsverwaltung/user/UserThemeDataProviderTest.java
@@ -64,18 +64,6 @@ class UserThemeDataProviderTest {
         assertThat(modelAndView.getModelMap()).containsEntry("theme", "system");
     }
 
-    @Test
-    void addSystemDefaultThemeForLogin() {
-        final ModelAndView modelAndView = new ModelAndView();
-        modelAndView.setViewName("login");
-
-        final MockHttpServletRequest request = new MockHttpServletRequest();
-        request.setLocalName(Locale.GERMAN.getLanguage());
-
-        sut.postHandle(request, null, null, modelAndView);
-        assertThat(modelAndView.getModelMap()).containsEntry("theme", "system");
-    }
-
     @ParameterizedTest
     @ValueSource(strings = {"forward:", "redirect:"})
     @NullSource
diff --git a/src/test/java/org/synyx/urlaubsverwaltung/web/FrameDataProviderTest.java b/src/test/java/org/synyx/urlaubsverwaltung/web/FrameDataProviderTest.java
index 43baf1ceb..511d830e3 100644
--- a/src/test/java/org/synyx/urlaubsverwaltung/web/FrameDataProviderTest.java
+++ b/src/test/java/org/synyx/urlaubsverwaltung/web/FrameDataProviderTest.java
@@ -347,7 +347,7 @@ class FrameDataProviderTest {
     }
 
     @ParameterizedTest
-    @ValueSource(strings = {"redirect:", "login"})
+    @ValueSource(strings = {"forward::view-name", "redirect::view-name"})
     @NullSource
     void postHandleDoNotAddGravatar(String viewName) {
 
diff --git a/src/test/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProviderTest.java b/src/test/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProviderTest.java
new file mode 100644
index 000000000..e4435a399
--- /dev/null
+++ b/src/test/java/org/synyx/urlaubsverwaltung/web/PreloadAssetProviderTest.java
@@ -0,0 +1,46 @@
+package org.synyx.urlaubsverwaltung.web;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.springframework.web.servlet.ModelAndView;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.verifyNoInteractions;
+
+@ExtendWith(MockitoExtension.class)
+class PreloadAssetProviderTest {
+
+    private PreloadAssetProvider sut;
+
+    @Mock
+    private AssetManifestService assetManifestService;
+
+    @BeforeEach
+    void setUp() {
+        sut = new PreloadAssetProvider(assetManifestService);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"forward:", "redirect:"})
+    @NullSource
+    void doNotAssets(String viewName) {
+
+        final ModelAndView modelAndView = new ModelAndView();
+        modelAndView.setViewName(viewName);
+
+        sut.postHandle(null, null, null, modelAndView);
+        assertThat(modelAndView.getModelMap().get("assets")).isNull();
+    }
+
+    @Test
+    void doNotAssetsIfModelAndViewIsNull() {
+        sut.postHandle(null, null, null, null);
+        verifyNoInteractions(assetManifestService);
+    }
+}