Merge pull request opentripplanner#6525 from entur/better_trip_api

Change the TripRequest and builder to work with include lists instead of just the FilterValues

Author: leonardehrenfried

application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java

@@ -4,12 +4,12 @@
 import static java.util.Collections.emptyList;
 import static org.opentripplanner.apis.transmodel.mapping.SeverityMapper.getTransmodelSeverity;
 import static org.opentripplanner.apis.transmodel.mapping.TransitIdMapper.mapIDToDomain;
+import static org.opentripplanner.apis.transmodel.mapping.TransitIdMapper.mapIDsToDomain;
 import static org.opentripplanner.apis.transmodel.mapping.TransitIdMapper.mapIDsToDomainNullSafe;
 import static org.opentripplanner.apis.transmodel.model.EnumTypes.FILTER_PLACE_TYPE_ENUM;
 import static org.opentripplanner.apis.transmodel.model.EnumTypes.MULTI_MODAL_MODE;
 import static org.opentripplanner.apis.transmodel.model.EnumTypes.TRANSPORT_MODE;
 import static org.opentripplanner.apis.transmodel.model.scalars.DateTimeScalarFactory.createMillisecondsSinceEpochAsDateTimeStringScalar;
-import static org.opentripplanner.apis.transmodel.support.GqlUtil.toListNullSafe;
 import static org.opentripplanner.model.projectinfo.OtpProjectInfo.projectInfo;
 
 import graphql.Scalars;
@@ -30,7 +30,6 @@
 import graphql.schema.GraphQLScalarType;
 import graphql.schema.GraphQLSchema;
 import graphql.schema.SchemaTransformer;
-import java.time.LocalDate;
 import java.time.ZoneId;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -1216,28 +1215,11 @@ private GraphQLSchema create() {
               .build()
           )
           .dataFetcher(environment -> {
-            var authorities = FilterValues.ofEmptyIsEverything(
-              "authorities",
-              mapIDsToDomainNullSafe(environment.getArgument("authorities"))
-            );
-            var lineIds = FilterValues.ofEmptyIsEverything(
-              "lines",
-              mapIDsToDomainNullSafe(environment.getArgument("lines"))
-            );
-            var privateCodes = FilterValues.ofEmptyIsEverything(
-              "privateCodes",
-              toListNullSafe(environment.<List<String>>getArgument("privateCodes"))
-            );
-            var activeServiceDates = FilterValues.ofEmptyIsEverything(
-              "activeDates",
-              toListNullSafe(environment.<List<LocalDate>>getArgument("activeDates"))
-            );
-
-            TripRequest tripRequest = TripRequest.of()
-              .withAgencies(authorities)
-              .withRoutes(lineIds)
-              .withNetexInternalPlanningCodes(privateCodes)
-              .withServiceDates(activeServiceDates)
+            var tripRequest = TripRequest.of()
+              .withIncludeAgencies(mapIDsToDomain(environment.getArgument("authorities")))
+              .withIncludeRoutes(mapIDsToDomain(environment.getArgument("lines")))
+              .withIncludeNetexInternalPlanningCodes(environment.getArgument("privateCodes"))
+              .withIncludeServiceDates(environment.getArgument("activeDates"))
               .build();
 
             return GqlUtil.getTransitService(environment).getTrips(tripRequest);

application/src/main/java/org/opentripplanner/apis/transmodel/mapping/TransitIdMapper.java

@@ -45,6 +45,18 @@ public static List<FeedScopedId> mapIDsToDomainNullSafe(@Nullable Collection<Str
     return ids.stream().filter(StringUtils::hasValue).map(TransitIdMapper::mapIDToDomain).toList();
   }
 
+  /**
+   * Maps ids to feed-scoped ids.
+   * Return null if the collection of ids is null.
+   * If the collection of ids contains null or blank elements, they are ignored.
+   */
+  public static List<FeedScopedId> mapIDsToDomain(@Nullable Collection<String> ids) {
+    if (ids == null) {
+      return null;
+    }
+    return ids.stream().filter(StringUtils::hasValue).map(TransitIdMapper::mapIDToDomain).toList();
+  }
+
   public static FeedScopedId mapIDToDomain(String id) {
     if (id == null || id.isBlank()) {
       return null;

application/src/main/java/org/opentripplanner/apis/transmodel/support/GqlUtil.java

@@ -99,12 +99,12 @@ public static Locale getLocale(DataFetchingEnvironment environment) {
   }
 
   /**
-   * Null-safe handling of a collection of type T. Returns an empty list if the collection is null.
+   * Null-safe handling of a collection of type T. Returns null if the incoming collection is null.
    * Null elements are filtered out.
    */
-  public static <T> List<T> toListNullSafe(@Nullable Collection<T> args) {
+  public static <T> List<T> toList(@Nullable Collection<T> args) {
     if (args == null) {
-      return List.of();
+      return null;
     }
     return args.stream().filter(Objects::nonNull).toList();
   }

application/src/main/java/org/opentripplanner/transit/api/model/FilterValues.java

@@ -42,6 +42,21 @@ public static <E> FilterValues<E> ofEmptyIsEverything(
     return new FilterValuesEmptyIsEverything<>(name, values);
   }
 
+  /**
+   * Returns a {@link FilterValues} that matches everything if the filter values are null.
+   * </p>
+   * @param name   - The name of the filter.
+   * @param <E>    - The type of the filter values. Typically, String or {@link FeedScopedId}.
+   * @param values - The {@link Collection} of filter values.
+   * @return FilterValues
+   */
+  public static <E> FilterValues<E> ofNullIsEverything(
+    String name,
+    @Nullable Collection<E> values
+  ) {
+    return new FilterValuesNullIsEverything<>(name, values);
+  }
+
   /**
    * Returns a {@link RequiredFilterValues} that throws an exception at creation time if the filter
    * values is null or empty.

application/src/main/java/org/opentripplanner/transit/api/model/FilterValuesNullIsEverything.java

@@ -0,0 +1,19 @@
+package org.opentripplanner.transit.api.model;
+
+import java.util.Collection;
+
+/**
+ * {@link FilterValuesNullIsEverything} is a subclass of {@link FilterValues} that
+ * includes everything only if the values collection is null.
+ */
+public class FilterValuesNullIsEverything<E> extends FilterValues<E> {
+
+  FilterValuesNullIsEverything(String name, Collection<E> values) {
+    super(name, values);
+  }
+
+  @Override
+  public boolean includeEverything() {
+    return values == null;
+  }
+}

application/src/main/java/org/opentripplanner/transit/api/request/TripRequest.java

@@ -12,40 +12,40 @@
  */
 public class TripRequest {
 
-  private final FilterValues<FeedScopedId> agencies;
-  private final FilterValues<FeedScopedId> routes;
-  private final FilterValues<String> netexInternalPlanningCodes;
-  private final FilterValues<LocalDate> serviceDates;
+  private final FilterValues<FeedScopedId> includeAgencies;
+  private final FilterValues<FeedScopedId> includeRoutes;
+  private final FilterValues<String> includeNetexInternalPlanningCodes;
+  private final FilterValues<LocalDate> includeServiceDates;
 
   TripRequest(
-    FilterValues<FeedScopedId> agencies,
-    FilterValues<FeedScopedId> routes,
-    FilterValues<String> netexInternalPlanningCodes,
-    FilterValues<LocalDate> serviceDates
+    FilterValues<FeedScopedId> includeAgencies,
+    FilterValues<FeedScopedId> includeRoutes,
+    FilterValues<String> includeNetexInternalPlanningCodes,
+    FilterValues<LocalDate> includeServiceDates
   ) {
-    this.agencies = agencies;
-    this.routes = routes;
-    this.netexInternalPlanningCodes = netexInternalPlanningCodes;
-    this.serviceDates = serviceDates;
+    this.includeAgencies = includeAgencies;
+    this.includeRoutes = includeRoutes;
+    this.includeNetexInternalPlanningCodes = includeNetexInternalPlanningCodes;
+    this.includeServiceDates = includeServiceDates;
   }
 
   public static TripRequestBuilder of() {
     return new TripRequestBuilder();
   }
 
-  public FilterValues<FeedScopedId> agencies() {
-    return agencies;
+  public FilterValues<FeedScopedId> includeAgencies() {
+    return includeAgencies;
   }
 
-  public FilterValues<FeedScopedId> routes() {
-    return routes;
+  public FilterValues<FeedScopedId> includeRoutes() {
+    return includeRoutes;
   }
 
-  public FilterValues<String> netexInternalPlanningCodes() {
-    return netexInternalPlanningCodes;
+  public FilterValues<String> includeNetexInternalPlanningCodes() {
+    return includeNetexInternalPlanningCodes;
   }
 
-  public FilterValues<LocalDate> serviceDates() {
-    return serviceDates;
+  public FilterValues<LocalDate> includeServiceDates() {
+    return includeServiceDates;
   }
 }

application/src/main/java/org/opentripplanner/transit/api/request/TripRequestBuilder.java

@@ -2,50 +2,65 @@
 
 import java.time.LocalDate;
 import java.util.List;
+import javax.annotation.Nullable;
 import org.opentripplanner.transit.api.model.FilterValues;
 import org.opentripplanner.transit.model.framework.FeedScopedId;
 
 public class TripRequestBuilder {
 
-  private FilterValues<FeedScopedId> agencies = FilterValues.ofEmptyIsEverything(
-    "agencies",
-    List.of()
+  private FilterValues<FeedScopedId> includeAgencies = FilterValues.ofNullIsEverything(
+    "includeAgencies",
+    null
   );
-  private FilterValues<FeedScopedId> routes = FilterValues.ofEmptyIsEverything("routes", List.of());
-  private FilterValues<String> netexInternalPlanningCodes = FilterValues.ofEmptyIsEverything(
-    "netexInternalPlanningCodes",
-    List.of()
+  private FilterValues<FeedScopedId> includeRoutes = FilterValues.ofNullIsEverything(
+    "includeRoutes",
+    null
   );
-  private FilterValues<LocalDate> serviceDates = FilterValues.ofEmptyIsEverything(
-    "serviceDates",
-    List.of()
+  private FilterValues<String> includeNetexInternalPlanningCodes = FilterValues.ofNullIsEverything(
+    "includeNetexInternalPlanningCodes",
+    null
+  );
+  private FilterValues<LocalDate> includeServiceDates = FilterValues.ofNullIsEverything(
+    "includeServiceDates",
+    null
   );
 
   TripRequestBuilder() {}
 
-  public TripRequestBuilder withAgencies(FilterValues<FeedScopedId> agencies) {
-    this.agencies = agencies;
+  public TripRequestBuilder withIncludeAgencies(@Nullable List<FeedScopedId> includeAgencies) {
+    this.includeAgencies = FilterValues.ofNullIsEverything("includeAgencies", includeAgencies);
     return this;
   }
 
-  public TripRequestBuilder withRoutes(FilterValues<FeedScopedId> routes) {
-    this.routes = routes;
+  public TripRequestBuilder withIncludeRoutes(@Nullable List<FeedScopedId> includeRoutes) {
+    this.includeRoutes = FilterValues.ofNullIsEverything("includeRoutes", includeRoutes);
     return this;
   }
 
-  public TripRequestBuilder withNetexInternalPlanningCodes(
-    FilterValues<String> netexInternalPlanningCodes
+  public TripRequestBuilder withIncludeNetexInternalPlanningCodes(
+    @Nullable List<String> includeNetexInternalPlanningCodes
   ) {
-    this.netexInternalPlanningCodes = netexInternalPlanningCodes;
+    this.includeNetexInternalPlanningCodes = FilterValues.ofNullIsEverything(
+      "includeNetexInternalPlanningCodes",
+      includeNetexInternalPlanningCodes
+    );
     return this;
   }
 
-  public TripRequestBuilder withServiceDates(FilterValues<LocalDate> serviceDates) {
-    this.serviceDates = serviceDates;
+  public TripRequestBuilder withIncludeServiceDates(@Nullable List<LocalDate> includeServiceDates) {
+    this.includeServiceDates = FilterValues.ofNullIsEverything(
+      "includeServiceDates",
+      includeServiceDates
+    );
     return this;
   }
 
   public TripRequest build() {
-    return new TripRequest(agencies, routes, netexInternalPlanningCodes, serviceDates);
+    return new TripRequest(
+      includeAgencies,
+      includeRoutes,
+      includeNetexInternalPlanningCodes,
+      includeServiceDates
+    );
   }
 }

application/src/main/java/org/opentripplanner/transit/model/filter/transit/TripMatcherFactory.java

@@ -36,14 +36,14 @@ public static Matcher<Trip> of(
   ) {
     ExpressionBuilder<Trip> expr = ExpressionBuilder.of();
 
-    expr.atLeastOneMatch(request.agencies(), TripMatcherFactory::agencyId);
-    expr.atLeastOneMatch(request.routes(), TripMatcherFactory::routeId);
+    expr.atLeastOneMatch(request.includeAgencies(), TripMatcherFactory::agencyId);
+    expr.atLeastOneMatch(request.includeRoutes(), TripMatcherFactory::routeId);
     expr.atLeastOneMatch(
-      request.netexInternalPlanningCodes(),
+      request.includeNetexInternalPlanningCodes(),
       TripMatcherFactory::netexInternalPlanningCode
     );
     expr.atLeastOneMatch(
-      request.serviceDates(),
+      request.includeServiceDates(),
       TripMatcherFactory.serviceDate(serviceDateProvider)
     );
 

application/src/test/java/org/opentripplanner/transit/model/filter/transit/TripMatcherFactoryTest.java

@@ -1,14 +1,13 @@
 package org.opentripplanner.transit.model.filter.transit;
 
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.time.LocalDate;
 import java.util.List;
 import java.util.Set;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
-import org.opentripplanner.transit.api.model.FilterValues;
 import org.opentripplanner.transit.api.request.TripRequest;
 import org.opentripplanner.transit.model.basic.TransitMode;
 import org.opentripplanner.transit.model.filter.expr.Matcher;
@@ -75,9 +74,7 @@ void setup() {
   @Test
   void testMatchRouteId() {
     TripRequest request = TripRequest.of()
-      .withRoutes(
-        FilterValues.ofEmptyIsEverything("routes", List.of(new FeedScopedId("F", "RUT:route:1")))
-      )
+      .withIncludeRoutes(List.of(new FeedScopedId("F", "RUT:route:1")))
       .build();
 
     Matcher<Trip> matcher = TripMatcherFactory.of(request, feedScopedId -> Set.of());
@@ -98,12 +95,32 @@ void testMatchDefaultAll() {
     assertTrue(matcher.match(tripAkt));
   }
 
+  @Test
+  void testNullListMatchesAll() {
+    TripRequest request = TripRequest.of().withIncludeAgencies(null).build();
+
+    Matcher<Trip> matcher = TripMatcherFactory.of(request, feedScopedId -> Set.of());
+
+    assertTrue(matcher.match(tripRut));
+    assertTrue(matcher.match(tripRut2));
+    assertTrue(matcher.match(tripAkt));
+  }
+
+  @Test
+  void testEmptyListMatchesNone() {
+    TripRequest request = TripRequest.of().withIncludeAgencies(List.of()).build();
+
+    Matcher<Trip> matcher = TripMatcherFactory.of(request, feedScopedId -> Set.of());
+
+    assertFalse(matcher.match(tripRut));
+    assertFalse(matcher.match(tripRut2));
+    assertFalse(matcher.match(tripAkt));
+  }
+
   @Test
   void testMatchAgencyId() {
     TripRequest request = TripRequest.of()
-      .withAgencies(
-        FilterValues.ofEmptyIsEverything("agencies", List.of(new FeedScopedId("F", "RUT:1")))
-      )
+      .withIncludeAgencies(List.of(new FeedScopedId("F", "RUT:1")))
       .build();
 
     Matcher<Trip> matcher = TripMatcherFactory.of(request, feedScopedId -> Set.of());
@@ -116,12 +133,7 @@ void testMatchAgencyId() {
   @Test
   void testMatchServiceDates() {
     TripRequest request = TripRequest.of()
-      .withServiceDates(
-        FilterValues.ofEmptyIsEverything(
-          "operatingDays",
-          List.of(LocalDate.of(2024, 2, 22), LocalDate.of(2024, 2, 23))
-        )
-      )
+      .withIncludeServiceDates(List.of(LocalDate.of(2024, 2, 22), LocalDate.of(2024, 2, 23)))
       .build();
 
     Matcher<Trip> matcher = TripMatcherFactory.of(request, this::dummyServiceDateProvider);