Fix graphql specification #68
Conversation
|
What can I do so it can be merged? |
|
Hi @l-vincent-l, generally we just prefer to have more explanations and recorded connections between commits, PRs and issues. For example in this case, there is an issue conveyal/gtfs-lib#94 which in turn references conveyal/gtfs-api#17 and records the history of this problem being reported and observations about it. In it's current state there's no information in this PR nor in its commits about how the specification was not being respected, nor the fact that this resolves an open issue. We would usually try to include the issue number reference in the commit message and/or the pull request, and even repeat some explanation of what problem was being addressed (even if it's a shortened copy and paste from an issue). It often also derails the review process when there's no statement of what the PR is meant to do or fix because there's no statement of a problem to check against. I will provide a review. |
| String query = node.get("query").asText(); | ||
| return doQuery(vars, query, response); | ||
| } | ||
|
|
||
| private static Object doQuery (String varsJson, String queryJson, Response response) { | ||
|
|
||
| private static Object doQuery (JsonNode varsJson, String queryJson, Response response) { |
There was a problem hiding this comment.
The type of result.toSpecification() is Map<String, Object> so the most specific return type of this function is Map<String, Object>.
| List<GraphQLError> errs = result.getErrors(); | ||
| if (!errs.isEmpty()) { | ||
| response.status(400); | ||
| return result.toSpecification(); |
There was a problem hiding this comment.
The if and else clauses both contain the same final return statement, so should be factored out. Other than setting the response status to 400 the rest of the statements in both branches of this conditional are always useful. So I'd prefer to just do if (!errs.isEmpty()) response.status(400); and then fall through to always run what is currently in the else block.
It wasn't respecting the graphql specification. So it couldn't be used with standard GraphQL clients.
l-vincent-l
force-pushed
the
fix_graphql_spec
branch
from
7f97324
to
57520e3
Compare
|
I change the PR according to @abyrd requests. |
|
Hi @l-vincent-l the only reason I haven't merged this is that it would require corresponding changes to our front end. I'll need to check with @landonreed to see how straightforward that will be. We're in the middle of testing the GTFS editor migration to RDBMS storage, so I'm not sure whether we'll have time to make that change for a few weeks. |
|
@l-vincent-l, we should be able to include this in the next release. However, I would recommend that the base branch be |
|
@Tristramg has rebased this against dev as #76. Closing. |
It wasn't respecting the graphql specification. So it couldn't be used
with standard GraphQL clients.