Skip to content
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.

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

feat: GTFS related endpoints #90

Merged
merged 13 commits into from
Sep 20, 2023
Merged

feat: GTFS related endpoints #90

merged 13 commits into from
Sep 20, 2023

Conversation

jujiang526
Copy link
Contributor

Summary:
Linked Issue

Implemented the following endpoints without supporting filtering or sorting

Get GTFS feeds
Get GTFS RT feeds
Get GTFS feed by ID
Get GTFS RT feed by ID
Get GTFS feed datasets for a given feed ID
Get GTFS datasets by dataset ID

Expected behavior:

Tested all those end points. The performance is good. Added a few unit tests regarding bounding filtering and database join query.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@welcome
Copy link

welcome bot commented Aug 27, 2023

Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:

  • fix: Bug with ssl network connections + Java module permissions.
  • feat: Initial support for multiple @PrimaryKey annotations.
  • docs: update RELEASE.md with new process
    To get this PR to the finish line, please do the following:
  • Include tests when adding/changing behavior
  • Include screenshots

@jujiang526 jujiang526 changed the title GTFS feed/dataset API implementation FETA/33 Aug 27, 2023
@jujiang526 jujiang526 changed the title FETA/33 FETA/33: GTFS related endpoints Aug 27, 2023
@aronza
Copy link
Contributor

aronza commented Aug 30, 2023

Hi @jujiang526 ,

Thank you for all this! I've refactored it further to reduce code duplication.

Also I saw that you were mapping database feed or dataset IDs directly to API IDs. I believe @jcpitre clarified in an earlier meeting that they want us to use stable_id when accepting or returning IDs in API. I've made the changes in the code and verified in my local Postman it works as expected.

Though the tests you've added were using these database IDs that are auto-generated in https://github.com/MobilityData/mobility-feed-api/blob/02f2b6f0b2a4dddac51f7e02de220c965e9004f4/api/src/scripts/populate_db.py#L123C1-L123C1 in our locals, so these tests won't pass anywhere else. Do you know which dataset stable ID did you use for these values?

@aronza aronza mentioned this pull request Sep 5, 2023
@aronza
Copy link
Contributor

aronza commented Sep 5, 2023

Found more missing functionality in this PR. The fixes are in https://github.com/MobilityData/mobility-feed-api/tree/arda/add-api-filter with the filtering functionality

@indraneel
Copy link
Contributor

🚨 Do not merge this PR into main, until #94 is reviewed/merged into this branch

@indraneel indraneel linked an issue Sep 5, 2023 that may be closed by this pull request
indraneel and others added 2 commits September 5, 2023 18:39
authored-by: arda <arda.turkmen@twosigma.com>
# Conflicts:
#	api/src/database/database.py
@aronza
Copy link
Contributor

aronza commented Sep 5, 2023

🚨 Do not merge this PR into main, until #94 is reviewed/merged into this branch

merged and resolved the conflicts from main

@jcpitre
Copy link
Contributor

jcpitre commented Sep 7, 2023

Added #96 to track filtering on dates for feeds.

docs/DatabaseCatalogAPI.yaml Outdated Show resolved Hide resolved
docs/DatabaseCatalogAPI.yaml Show resolved Hide resolved
@jcpitre jcpitre changed the title FETA/33: GTFS related endpoints FEAT/33: GTFS related endpoints Sep 12, 2023
@jujiang526
Copy link
Contributor Author

@aronza - Can you please try to run the unit test in your environment? I believe I have fixed it. I also fixed a small issue with returning duplicated records from the joined table.

@cka-y cka-y changed the title FEAT/33: GTFS related endpoints feat: GTFS related endpoints Sep 15, 2023
@aronza
Copy link
Contributor

aronza commented Sep 15, 2023

@aronza - Can you please try to run the unit test in your environment? I believe I have fixed it. I also fixed a small issue with returning duplicated records from the joined table.

They pass on my local too @jujiang526 . Though our changes to database class broke @goorui 's unit tests, so I refactored these tests to work with the latest changes

instance = None

def __new__(cls, *args, **kwargs):
if not isinstance(cls.instance, cls):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this thread safe?
If not, maybe it's irrelevant to our case. If so, can you add a comment explaining why it's not relevant?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jujiang526 , I believe you've added this. Any thoughts?

@jcpitre jcpitre merged commit 984cb96 into main Sep 20, 2023
1 check passed
@jcpitre jcpitre deleted the jujiang/gtfs_feed branch September 20, 2023 20:10
@welcome
Copy link

welcome bot commented Sep 20, 2023

🥳 Congrats on getting your first pull request merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Develop feeds/gtfs endpoint
4 participants