Skip to content

Add validation to the serviceJourney Transmodel API endpoint #6559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: dev-2.x
Choose a base branch
from
+29 −4
Open

Add validation to the serviceJourney Transmodel API endpoint #6559

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a60232
Requires at least one of the arguments to the serviceJourneys API end…
eibakke Mar 19, 2025
00e4dc4
Filters out empty strings for the privateCodes field in the serviceJo…
eibakke Mar 19, 2025
36fec83
Adds details to error message and documentation of serviceJourneys API.
eibakke Mar 20, 2025
45193a1
Update application/src/main/java/org/opentripplanner/apis/transmodel/…
eibakke Mar 26, 2025
3a04547
Adds nullable to return value of toStringList.
eibakke Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions application/src/main/java/org/opentripplanner/apis/transmodel/TransmodelGraphQLSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
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;
Expand Down Expand Up @@ -1215,11 +1216,20 @@ private GraphQLSchema create() {
.build()
)
.dataFetcher(environment -> {
if (
Stream.of("lines", "privateCodes", "authorities", "activeDates").noneMatch(
key -> environment.getArgument(key) != null
)
) {
throw new IllegalArgumentException("At least one argument must be non-null");
}
var tripRequest = TripRequest.of()
.withIncludeAgencies(mapIDsToDomain(environment.getArgument("authorities")))
.withIncludeRoutes(mapIDsToDomain(environment.getArgument("lines")))
.withIncludeNetexInternalPlanningCodes(environment.getArgument("privateCodes"))
.withIncludeServiceDates(environment.getArgument("activeDates"))
.withIncludeNetexInternalPlanningCodes(
GqlUtil.toStringList(environment.getArgument("privateCodes"))
)
.withIncludeServiceDates(GqlUtil.toList(environment.getArgument("activeDates")))
.build();

return GqlUtil.getTransitService(environment).getTrips(tripRequest);
Expand Down
11 changes: 11 additions & 0 deletions application/src/main/java/org/opentripplanner/apis/transmodel/support/GqlUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,15 @@ public static <T> List<T> toList(@Nullable Collection<T> args) {
}
return args.stream().filter(Objects::nonNull).toList();
}

/**
* Null-safe handling of a collection of strings. Returns null if the incoming collection is null.
* Null and empty elements are filtered out.
*/
public static List<String> toStringList(@Nullable Collection<String> args) {
if (args == null) {
return null;
}
return args.stream().filter(Objects::nonNull).filter(s -> !s.isEmpty()).toList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general it's best to be strict and not have special handling for empty strings and other edge cases. Cleaning user data hides bugs in the clients where they should be fixed.

If a user says they want the service journeys with private codes [ "" ] then they should get the service journeys that has that private code, which will most probably be an empty set.

What do you think about that?

The null filtering is fine though, we probably need this since we for some reason allowed the elements to be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the opposite of what @vpaturet expressed in #6545 (comment)

I tend to agree with Vincent here, that empty strings should be ignored ie. treated the same as nulls. This also matches how the mapIDsToDomain method handles empty (or even blank strings), using

OpenTripPlanner/utils/src/main/java/org/opentripplanner/utils/lang/StringUtils.java

Line 25 in e8e9a77

public static boolean hasValue(String text) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow response here!

I must say that I disagree with treating empty strings as nulls. If we need to do it for backwards compatability I can see the point, but in this case it looks like we are making the api less strict than previously. The stricter we make the api and the less special cases we add the easier it will be to maintain.

We had a short discussion around this on the developer meeting two weeks ago, and I think the consensus was to avoid special handling for empty strings unless it's needed for compatability reasons.

}
}
Loading