Merge pull request #219 from entur/fix-minimum-max-age

fix: max-age should not be less than 0
entur · Aug 24, 2023 · 044d979 · 044d979
2 parents b5055f3 + 049f916
commit 044d979
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 57 deletions.
diff --git a/src/main/java/org/entur/lamassu/controller/GBFSFeedController.java b/src/main/java/org/entur/lamassu/controller/GBFSFeedController.java
@@ -1,6 +1,5 @@
 package org.entur.lamassu.controller;
 
-import java.lang.reflect.InvocationTargetException;
 import java.net.URI;
 import java.util.List;
 import java.util.NoSuchElementException;
@@ -12,7 +11,6 @@
 import org.entur.gbfs.v2_3.gbfs.GBFSFeed;
 import org.entur.gbfs.v2_3.gbfs.GBFSFeedName;
 import org.entur.gbfs.v2_3.gbfs.GBFSFeeds;
-import org.entur.gbfs.v2_3.gbfs.GBFSGbfs;
 import org.entur.gbfs.v2_3.geofencing_zones.GBFSGeofencingZones;
 import org.entur.gbfs.v2_3.station_information.GBFSStationInformation;
 import org.entur.gbfs.v2_3.station_status.GBFSStationStatus;
@@ -31,6 +29,7 @@
 import org.entur.lamassu.model.provider.FeedProvider;
 import org.entur.lamassu.service.FeedProviderService;
 import org.entur.lamassu.service.SystemDiscoveryService;
+import org.entur.lamassu.util.CacheUtil;
 import org.jetbrains.annotations.NotNull;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
@@ -115,7 +114,9 @@ public ResponseEntity<Object> getGbfsFeedForProvider(
       return ResponseEntity
         .ok()
         .cacheControl(
-          CacheControl.maxAge(getMaxAge(feedName, data), TimeUnit.SECONDS).cachePublic()
+          CacheControl
+            .maxAge(CacheUtil.getMaxAge(feedName, data), TimeUnit.SECONDS)
+            .cachePublic()
         )
         .body(data);
     } catch (IllegalArgumentException e) {
@@ -145,7 +146,9 @@ public Object getInternalGbfsFeedForProvider(
       return ResponseEntity
         .ok()
         .cacheControl(
-          CacheControl.maxAge(getMaxAge(feedName, data), TimeUnit.SECONDS).cachePublic()
+          CacheControl
+            .maxAge(CacheUtil.getMaxAge(feedName, data), TimeUnit.SECONDS)
+            .cachePublic()
         )
         .body(data);
     } catch (IllegalArgumentException e) {
@@ -266,7 +269,9 @@ public ResponseEntity<Object> getV3Feed(
       return ResponseEntity
         .ok()
         .cacheControl(
-          CacheControl.maxAge(getMaxAge(feedName, mapped), TimeUnit.SECONDS).cachePublic()
+          CacheControl
+            .maxAge(CacheUtil.getMaxAge(feedName, mapped), TimeUnit.SECONDS)
+            .cachePublic()
         )
         .body(mapped);
     } catch (IllegalArgumentException e) {
@@ -276,58 +281,6 @@ public ResponseEntity<Object> getV3Feed(
     }
   }
 
-  private int getMaxAge(GBFSFeedName feedName, Object data) {
-    int maxAge = 60;
-    try {
-      Integer lastUpdated = (Integer) feedName
-        .implementingClass()
-        .getMethod("getLastUpdated")
-        .invoke(data);
-      Integer ttl = (Integer) feedName
-        .implementingClass()
-        .getMethod("getTtl")
-        .invoke(data);
-
-      if (lastUpdated != null && ttl != null) {
-        maxAge = getCurrentTimeSeconds() - lastUpdated + ttl;
-      }
-    } catch (
-      IllegalAccessException | InvocationTargetException | NoSuchMethodException e
-    ) {
-      // log something?
-    }
-
-    return maxAge;
-  }
-
-  private int getMaxAge(org.entur.gbfs.v3_0_RC.gbfs.GBFSFeed.Name feedName, Object data) {
-    int maxAge = 60;
-    try {
-      Integer lastUpdated = (Integer) org.entur.gbfs.v3_0_RC.gbfs.GBFSFeedName
-        .implementingClass(feedName)
-        .getMethod("getLastUpdated")
-        .invoke(data);
-      Integer ttl = (Integer) org.entur.gbfs.v3_0_RC.gbfs.GBFSFeedName
-        .implementingClass(feedName)
-        .getMethod("getTtl")
-        .invoke(data);
-
-      if (lastUpdated != null && ttl != null) {
-        maxAge = getCurrentTimeSeconds() - lastUpdated + ttl;
-      }
-    } catch (
-      IllegalAccessException | InvocationTargetException | NoSuchMethodException e
-    ) {
-      // log something?
-    }
-
-    return maxAge;
-  }
-
-  private int getCurrentTimeSeconds() {
-    return (int) (java.lang.System.currentTimeMillis() / 1000L);
-  }
-
   @NotNull
   private Object getFeed(String systemId, String feed) {
     var feedName = GBFSFeedName.fromValue(feed);

diff --git a/src/main/java/org/entur/lamassu/util/CacheUtil.java b/src/main/java/org/entur/lamassu/util/CacheUtil.java
@@ -18,7 +18,9 @@
 
 package org.entur.lamassu.util;
 
+import java.lang.reflect.InvocationTargetException;
 import java.time.Instant;
+import org.entur.gbfs.v2_3.gbfs.GBFSFeedName;
 
 public class CacheUtil {
 
@@ -32,4 +34,51 @@ public static int getTtl(int lastUpdated, int ttl, int minimumTtl) {
   public static int getTtl(int lastUpdated, int ttl, int minimumTtl, int maximumTtl) {
     return Math.min(getTtl(lastUpdated, ttl, minimumTtl), maximumTtl);
   }
+
+  public static int getMaxAge(GBFSFeedName feedName, Object data) {
+    int maxAge = 60;
+    try {
+      Integer lastUpdated = (Integer) feedName
+        .implementingClass()
+        .getMethod("getLastUpdated")
+        .invoke(data);
+      Integer ttl = (Integer) feedName
+        .implementingClass()
+        .getMethod("getTtl")
+        .invoke(data);
+
+      maxAge = getTtl(lastUpdated, ttl, 0);
+    } catch (
+      IllegalAccessException | InvocationTargetException | NoSuchMethodException e
+    ) {
+      // log something?
+    }
+
+    return maxAge;
+  }
+
+  public static int getMaxAge(
+    org.entur.gbfs.v3_0_RC.gbfs.GBFSFeed.Name feedName,
+    Object data
+  ) {
+    int maxAge = 60;
+    try {
+      Integer lastUpdated = (Integer) org.entur.gbfs.v3_0_RC.gbfs.GBFSFeedName
+        .implementingClass(feedName)
+        .getMethod("getLastUpdated")
+        .invoke(data);
+      Integer ttl = (Integer) org.entur.gbfs.v3_0_RC.gbfs.GBFSFeedName
+        .implementingClass(feedName)
+        .getMethod("getTtl")
+        .invoke(data);
+
+      maxAge = getTtl(lastUpdated, ttl, 0);
+    } catch (
+      IllegalAccessException | InvocationTargetException | NoSuchMethodException e
+    ) {
+      // log something?
+    }
+
+    return maxAge;
+  }
 }
diff --git a/src/test/java/org/entur/lamassu/util/CacheUtilTest.java b/src/test/java/org/entur/lamassu/util/CacheUtilTest.java
@@ -21,6 +21,8 @@
 import static org.mockito.Mockito.mockStatic;
 
 import java.time.Instant;
+import org.entur.gbfs.v2_3.gbfs.GBFS;
+import org.entur.gbfs.v2_3.gbfs.GBFSFeedName;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
@@ -66,4 +68,26 @@ void getTtlReturnsMinimumTtlWhenLessThanMinimumTtl() {
   void getTtlReturnsCalculatedTtlWhenLargerThanMinimumTtl() {
     Assertions.assertEquals(20, CacheUtil.getTtl(now - 10, 30, 10));
   }
+
+  @Test
+  void getMaxAgeWorks() {
+    Assertions.assertEquals(
+      60,
+      CacheUtil.getMaxAge(
+        GBFSFeedName.GBFS,
+        new GBFS().withLastUpdated(1640000000 - 60).withTtl(120)
+      )
+    );
+  }
+
+  @Test
+  void getMaxAgeReturnsZeroWhenCalculatedTtlIsInThePast() {
+    Assertions.assertEquals(
+      0,
+      CacheUtil.getMaxAge(
+        GBFSFeedName.GBFS,
+        new GBFS().withLastUpdated(1540000000).withTtl(60)
+      )
+    );
+  }
 }