Skip to content

Commit

Permalink
refactor(graphql): update graphql a little bit
Browse files Browse the repository at this point in the history 
Take some reasonable changes that came out of #144 and add them here
  • Loading branch information
@evansiroky
evansiroky committed Nov 20, 2018
1 parent 1a3de7d commit 88b269e
Show file tree
Hide file tree
Showing 11 changed files with 262 additions and 58 deletions.
2 changes: 1 addition & 1 deletion pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@
<dependency>
<groupId>com.graphql-java</groupId>
<artifactId>graphql-java</artifactId>
<version>4.2</version>
<version>11.0</version>
</dependency>
</dependencies>
</project>
1 change: 1 addition & 0 deletions src/main/java/com/conveyal/gtfs/graphql/GraphQLGtfsSchema.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ public class GraphQLGtfsSchema {
.type(new GraphQLList(routeType))
.argument(stringArg("namespace"))
.argument(stringArg(SEARCH_ARG))
.argument(intArg(LIMIT_ARG))
.dataFetcher(new NestedJDBCFetcher(
new JDBCFetcher("pattern_stops", "stop_id", null, false),
new JDBCFetcher("patterns", "pattern_id", null, false),
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/com/conveyal/gtfs/graphql/GraphQLUtil.java
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package com.conveyal.gtfs.graphql;

import graphql.schema.FieldDataFetcher;
import graphql.schema.GraphQLArgument;
import graphql.schema.GraphQLFieldDefinition;
import graphql.schema.GraphQLList;
import graphql.schema.PropertyDataFetcher;

import static graphql.Scalars.GraphQLFloat;
import static graphql.Scalars.GraphQLInt;
Expand All @@ -17,15 +17,15 @@ public static GraphQLFieldDefinition string (String name) {
return newFieldDefinition()
.name(name)
.type(GraphQLString)
.dataFetcher(new FieldDataFetcher(name))
.dataFetcher(new PropertyDataFetcher(name))
.build();
}

public static GraphQLFieldDefinition intt (String name) {
return newFieldDefinition()
.name(name)
.type(GraphQLInt)
.dataFetcher(new FieldDataFetcher(name))
.dataFetcher(new PropertyDataFetcher(name))
.build();
}

Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/conveyal/gtfs/graphql/fetchers/FeedFetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import java.util.HashMap;
import java.util.Map;

import static com.conveyal.gtfs.graphql.fetchers.JDBCFetcher.validateNamespace;

/**
* Fetch the summary row for a particular loaded feed, based on its namespace.
* This essentially gets the row from the top-level summary table of all feeds that have been loaded into the database.
Expand All @@ -25,6 +27,7 @@ public class FeedFetcher implements DataFetcher {
@Override
public Map<String, Object> get (DataFetchingEnvironment environment) {
String namespace = environment.getArgument("namespace"); // This is the unique table prefix (the "schema").
validateNamespace(namespace);
StringBuilder sqlBuilder = new StringBuilder();
sqlBuilder.append(String.format("select * from feeds where namespace = '%s'", namespace));
Connection connection = null;
Expand Down
65 changes: 35 additions & 30 deletions src/main/java/com/conveyal/gtfs/graphql/fetchers/JDBCFetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,16 @@ public List<Map<String, Object>> get (DataFetchingEnvironment environment) {
* Handle fetching functionality for a given namespace, set of join values, and arguments. This is broken out from
* the standard get function so that it can be reused in other fetchers (i.e., NestedJdbcFetcher)
*/
List<Map<String, Object>> getResults (String namespace, List<String> parentJoinValues, Map<String, Object> arguments) {
List<Map<String, Object>> getResults (
String namespace,
List<String> parentJoinValues,
Map<String, Object> graphQLQueryArguments
) {
// Track the parameters for setting prepared statement parameters
List<String> parameters = new ArrayList<>();
List<String> preparedStatementParameters = new ArrayList<>();
// This will contain one Map<String, Object> for each row fetched from the database table.
List<Map<String, Object>> results = new ArrayList<>();
if (arguments == null) arguments = new HashMap<>();
if (graphQLQueryArguments == null) graphQLQueryArguments = new HashMap<>();
// Ensure namespace exists and is clean. Note: FeedFetcher will have executed before this and validated that an
// entry exists in the feeds table and the schema actually exists in the database.
validateNamespace(namespace);
Expand All @@ -188,31 +192,32 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
sqlBuilder.append("select *");

// We will build up additional sql clauses in this List (note: must be a List so that the order is preserved).
List<String> conditions = new ArrayList<>();
List<String> whereConditions = new ArrayList<>();
// The order by clause will go here.
String sortBy = "";
// If we are fetching an item nested within a GTFS entity in the GraphQL query, we want to add an SQL "where"
// clause. This could conceivably be done automatically, but it's clearer to just express the intent.
// Note, this is assuming the type of the field in the parent is a string.
if (parentJoinField != null && parentJoinValues != null && !parentJoinValues.isEmpty()) {
conditions.add(makeInClause(parentJoinField, parentJoinValues, parameters));
whereConditions.add(makeInClause(parentJoinField, parentJoinValues, preparedStatementParameters));
}
if (sortField != null) {
// Sort field is not provided by user input, so it's ok to add here (i.e., it's not prone to SQL injection).
sortBy = String.format(" order by %s", sortField);
}
Set<String> argumentKeys = arguments.keySet();
Set<String> argumentKeys = graphQLQueryArguments.keySet();
for (String key : argumentKeys) {
// The pagination, bounding box, and date/time args should all be skipped here because they are handled
// separately below from standard args (pagination becomes limit/offset clauses, bounding box applies to
// stops table, and date/time args filter stop times. All other args become "where X in A, B, C" clauses.
if (argsToSkip.contains(key)) continue;
if (ID_ARG.equals(key)) {
Integer value = (Integer) arguments.get(key);
conditions.add(String.join(" = ", "id", value.toString()));
Integer value = (Integer) graphQLQueryArguments.get(key);
whereConditions.add(String.join(" = ", "id", value.toString()));
} else {
List<String> values = (List<String>) arguments.get(key);
if (values != null && !values.isEmpty()) conditions.add(makeInClause(key, values, parameters));
List<String> values = (List<String>) graphQLQueryArguments.get(key);
if (values != null && !values.isEmpty())
whereConditions.add(makeInClause(key, values, preparedStatementParameters));
}
}
if (argumentKeys.containsAll(boundingBoxArgs)) {
Expand All @@ -222,7 +227,7 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
// operating on the patterns table, a SELECT DISTINCT patterns query will be constructed with a join to
// stops and pattern stops.
for (String bound : boundingBoxArgs) {
Double value = (Double) arguments.get(bound);
Double value = (Double) graphQLQueryArguments.get(bound);
// Determine delimiter/equality operator based on min/max
String delimiter = bound.startsWith("max") ? " <= " : " >= ";
// Determine field based on lat/lon
Expand All @@ -232,7 +237,7 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
boundsConditions.add(String.join(delimiter, fieldWithNamespace, value.toString()));
}
if ("stops".equals(tableName)) {
conditions.addAll(boundsConditions);
whereConditions.addAll(boundsConditions);
} else if ("patterns".equals(tableName)) {
// Add from table as unique_pattern_ids_in_bounds to match patterns table -> pattern stops -> stops
fromTables.add(
Expand All @@ -243,7 +248,7 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
String.join(" and ", boundsConditions),
namespace
));
conditions.add(String.format("%s.patterns.pattern_id = unique_pattern_ids_in_bounds.pattern_id", namespace));
whereConditions.add(String.format("%s.patterns.pattern_id = unique_pattern_ids_in_bounds.pattern_id", namespace));
}
}
if (argumentKeys.contains(DATE_ARG)) {
Expand All @@ -253,36 +258,36 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
// service_dates table. In other words, feeds generated by the editor cannot be queried with the date/time args.
String tripsTable = String.format("%s.trips", namespace);
fromTables.add(tripsTable);
String date = getDateArgument(arguments);
String date = getDateArgument(graphQLQueryArguments);
// Gather all service IDs that run on the provided date.
fromTables.add(String.format(
"(select distinct service_id from %s.service_dates where service_date = ?) as unique_service_ids_in_operation",
namespace)
);
// Add date to beginning of parameters list (it is used to pre-select a table in the from clause before any
// other conditions or parameters are appended).
parameters.add(0, date);
preparedStatementParameters.add(0, date);
if (argumentKeys.contains(FROM_ARG) && argumentKeys.contains(TO_ARG)) {
// Determine which trips start in the specified time window by joining to filtered stop times.
String timeFilteredTrips = "trips_beginning_in_time_period";
conditions.add(String.format("%s.trip_id = %s.trip_id", timeFilteredTrips, tripsTable));
whereConditions.add(String.format("%s.trip_id = %s.trip_id", timeFilteredTrips, tripsTable));
// Select all trip IDs that start during the specified time window. Note: the departure and arrival times
// are divided by 86399 to account for trips that begin after midnight. FIXME: Should this be 86400?
fromTables.add(String.format(
"(select trip_id " +
"from (select distinct on (trip_id) * from %s.stop_times order by trip_id, stop_sequence) as first_stop_times " +
"where departure_time %% 86399 >= %d and departure_time %% 86399 <= %d) as %s",
namespace,
(int) arguments.get(FROM_ARG),
(int) arguments.get(TO_ARG),
(int) graphQLQueryArguments.get(FROM_ARG),
(int) graphQLQueryArguments.get(TO_ARG),
timeFilteredTrips));
}
// Join trips to service_dates (unique_service_ids_in_operation).
conditions.add(String.format("%s.service_id = unique_service_ids_in_operation.service_id", tripsTable));
whereConditions.add(String.format("%s.service_id = unique_service_ids_in_operation.service_id", tripsTable));
}
if (argumentKeys.contains(SEARCH_ARG)) {
// Handle string search argument
String value = (String) arguments.get(SEARCH_ARG);
String value = (String) graphQLQueryArguments.get(SEARCH_ARG);
if (!value.isEmpty()) {
// Only apply string search if string is not empty.
Set<String> searchFields = getSearchFields(namespace);
Expand All @@ -292,23 +297,23 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
// FIXME: is ILIKE compatible with non-Postgres? LIKE doesn't work well enough (even when setting
// the strings to lower case).
searchClauses.add(String.format("%s ILIKE ?", field));
parameters.add(String.format("%%%s%%", value));
preparedStatementParameters.add(String.format("%%%s%%", value));
}
if (!searchClauses.isEmpty()) {
// Wrap string search in parentheses to isolate from other conditions.
conditions.add(String.format(("(%s)"), String.join(" OR ", searchClauses)));
whereConditions.add(String.format(("(%s)"), String.join(" OR ", searchClauses)));
}
}
}
sqlBuilder.append(String.format(" from %s", String.join(", ", fromTables)));
if (!conditions.isEmpty()) {
if (!whereConditions.isEmpty()) {
sqlBuilder.append(" where ");
sqlBuilder.append(String.join(" and ", conditions));
sqlBuilder.append(String.join(" and ", whereConditions));
}
// The default value for sortBy is an empty string, so it's safe to always append it here. Also, there is no
// threat of SQL injection because the sort field value is not user input.
sqlBuilder.append(sortBy);
Integer limit = (Integer) arguments.get(LIMIT_ARG);
Integer limit = (Integer) graphQLQueryArguments.get(LIMIT_ARG);
if (limit == null) {
limit = autoLimit ? DEFAULT_ROWS_TO_FETCH : -1;
}
Expand All @@ -322,7 +327,7 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
} else {
sqlBuilder.append(" limit ").append(limit);
}
Integer offset = (Integer) arguments.get(OFFSET_ARG);
Integer offset = (Integer) graphQLQueryArguments.get(OFFSET_ARG);
if (offset != null && offset >= 0) {
sqlBuilder.append(" offset ").append(offset);
}
Expand All @@ -331,7 +336,7 @@ List<Map<String, Object>> getResults (String namespace, List<String> parentJoinV
connection = GTFSGraphQL.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(sqlBuilder.toString());
int oneBasedIndex = 1;
for (String parameter : parameters) {
for (String parameter : preparedStatementParameters) {
preparedStatement.setString(oneBasedIndex++, parameter);
}
// This logging produces a lot of noise during testing due to large numbers of joined sub-queries
Expand Down Expand Up @@ -381,7 +386,7 @@ private static String getDateArgument(Map<String, Object> arguments) {
* @param namespace database schema namespace/table prefix
* @return
*/
private static void validateNamespace(String namespace) {
public static void validateNamespace(String namespace) {
if (namespace == null) {
// If namespace is null, do no attempt a query on a namespace that does not exist.
throw new IllegalArgumentException("Namespace prefix must be provided.");
Expand Down Expand Up @@ -437,7 +442,7 @@ private Set<String> getSearchFields(String namespace) {
/**
* Construct filter clause with '=' (single string) and add values to list of parameters.
* */
static String makeInClause(String filterField, String string, List<String> parameters) {
static String filterEquals(String filterField, String string, List<String> parameters) {
// Add string to list of parameters (to be later used to set parameters for prepared statement).
parameters.add(string);
return String.format("%s = ?", filterField);
Expand All @@ -448,7 +453,7 @@ static String makeInClause(String filterField, String string, List<String> param
* */
static String makeInClause(String filterField, List<String> strings, List<String> parameters) {
if (strings.size() == 1) {
return makeInClause(filterField, strings.get(0), parameters);
return filterEquals(filterField, strings.get(0), parameters);
} else {
// Add strings to list of parameters (to be later used to set parameters for prepared statement).
parameters.addAll(strings);
Expand Down
21 changes: 15 additions & 6 deletions src/main/java/com/conveyal/gtfs/graphql/fetchers/RowCountFetcher.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static com.conveyal.gtfs.graphql.GraphQLUtil.intt;
import static com.conveyal.gtfs.graphql.GraphQLUtil.string;
import static com.conveyal.gtfs.graphql.GraphQLUtil.stringArg;
import static com.conveyal.gtfs.graphql.fetchers.JDBCFetcher.makeInClause;
import static com.conveyal.gtfs.graphql.fetchers.JDBCFetcher.filterEquals;
import static graphql.Scalars.GraphQLInt;
import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition;
import static graphql.schema.GraphQLObjectType.newObject;
Expand Down Expand Up @@ -71,8 +70,13 @@ public Object get(DataFetchingEnvironment environment) {
// FIXME Does this handle null cases?
// Add where clause to filter out non-matching results.
String filterValue = (String) parentFeedMap.get(filterField);
String inClause = makeInClause(filterField, filterValue, parameters);
clauses.add(String.join(" ", "where", inClause));
clauses.add(
String.join(
" ",
"where",
filterEquals(filterField, filterValue, parameters)
)
);
} else if (groupByField != null) {
// Handle group by field and optionally handle any filter arguments passed in.
if (!argKeys.isEmpty()) {
Expand All @@ -86,8 +90,13 @@ public Object get(DataFetchingEnvironment environment) {
}
String filterValue = (String) arguments.get(groupedFilterField);
if (filterValue != null) {
String inClause = makeInClause(groupedFilterField, filterValue, parameters);
clauses.add(String.join(" ", "where", inClause));
clauses.add(
String.join(
" ",
"where",
filterEquals(groupedFilterField, filterValue, parameters)
)
);
}
}
// Finally, add group by clause.
Expand Down
Loading

0 comments on commit 88b269e

Please sign in to comment.