Skip to content

Add new path for GTFS GraphQL API, remove batch feature#5581

Merged
leonardehrenfried merged 1 commit into
opentripplanner:dev-2.xfrom
leonardehrenfried:graphql-batch
Jan 17, 2024
Merged

Add new path for GTFS GraphQL API, remove batch feature#5581
leonardehrenfried merged 1 commit into
opentripplanner:dev-2.xfrom
leonardehrenfried:graphql-batch

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Dec 18, 2023

Copy link
Copy Markdown
Member

Summary

Changes the official path of the GTFS GraphQL API to otp/gtfs/v1 and keeps the old one around for backwards-compatibility. We can remove the backwards-compat when all known users have migrated their systems off it, which I expect to take quite a long time.

It also removes the batch functionality which is confusing, unintuitive and in the age of HTTP2 doesn't have performance benefits.

Issue

#4052

Documentation

Updated.

cc @hbruch @vesameskanen @binh-dam-ibigroup @miles-grant-ibigroup

@t2gran: We agreed to remove tech from url, so I updated the summary - original suggestion was: otp/graphql/gtfs/v1.

@leonardehrenfried leonardehrenfried added the !Improvement A functional improvement or micro feature label Dec 18, 2023
@leonardehrenfried leonardehrenfried requested a review from a team as a code owner December 18, 2023 11:59
@leonardehrenfried leonardehrenfried force-pushed the graphql-batch branch 2 times, most recently from 8246ad7 to 27e619b Compare December 18, 2023 12:02
@codecov

codecov Bot commented Dec 18, 2023

Copy link
Copy Markdown

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c5dc289) 67.49% compared to head (216bea8) 67.49%.
Report is 45 commits behind head on dev-2.x.

Files Patch % Lines
.../org/opentripplanner/apis/gtfs/GtfsGraphQLAPI.java 0.00% 3 Missing ⚠️
...in/java/org/opentripplanner/apis/APIEndpoints.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #5581   +/-   ##
==========================================
  Coverage      67.49%   67.49%           
- Complexity     16226    16240   +14     
==========================================
  Files           1868     1874    +6     
  Lines          71297    71367   +70     
  Branches        7408     7398   -10     
==========================================
+ Hits           48121    48170   +49     
- Misses         20681    20700   +19     
- Partials        2495     2497    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t2gran

t2gran commented Dec 20, 2023

Copy link
Copy Markdown
Member

I think we should use otp/gtfs/v1. I think tech should not be part of the name. The alternative is otp/gtfs/graphql/v1, but I do not want to design for the future - and it is farely easy to come up with a new url for a new api. I think we should keep the otp/ prefix. It makes it easier to mount e.g. other things at root, and in general avoid naming clashes.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

I think we should use otp/gtfs/v1. I think tech should not be part of the name. The alternative is otp/gtfs/graphql/v1, but I do not want to design for the future - and it is farely easy to come up with a new url for a new api. I think we should keep the otp/ prefix. It makes it easier to mount e.g. other things at root, and in general avoid naming clashes.

I'm fine with all of this and have applied the feedback to this PR.

@optionsome optionsome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is still a reference in the magidoc.mjs to the old endpoint

optionsome
optionsome previously approved these changes Jan 12, 2024
@t2gran t2gran added this to the 2.5 (next release) milestone Jan 16, 2024
@t2gran t2gran added the !Technical Debt Improve code quality, no functional changes. label Jan 16, 2024

@Path("/routers/{ignoreRouterId}/index/graphql")
@Produces(MediaType.APPLICATION_JSON)
public class BackwardsCompatGraphQLAPI extends BaseGtfsGraphQLAPI {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider: I am not too happy about using inheritance for this, but I do get why you do it. A little less painful approach would be to let the old BackwardsCompatGraphQLAPI inherit the new GtfsGraphQLAPI - that allows us to use inheritance if we want in the new class (if there is a good reason to do it). Not sure if the @Path annotation clashes if you do so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This little trick worked - using a static inerclass to express the tight coupling ...
entur@3968a37

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By the way the url for the transmodel api should be /transmodel/v3. v1 and v2 exist in entur repo.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

Thanks for the tip, @t2gran. I updated the code to use an inner class.

However, I couldn't use the same one on the Transmodel API, since it has @PathParam annotations on both the class and the method level. This would mean that the new Transmodel API endpoint would be /otp/transmodel/v3/graphql which probably isn't what you want, isn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants