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: add openapi specification (to discuss !136) #138

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

dersvenhesse
Copy link
Contributor

@dersvenhesse dersvenhesse commented Aug 5, 2024

Hello,
as suggested in !136 I created a specification for v2.3 to discuss the idea with a suggestion.

For now, the file is generated manually and contains mostly (conditionally) required feeds. All feeds are combined in a single specification separated by tags.
How do you feel about this?

@Alessandro100
Copy link
Contributor

Thank you for taking the initiative to work on this!

Personally I think splitting up the spec into smaller files would be best, but depending on how the generator works, I wouldn’t be totally against having a large file with tags

The yaml file should be moved under the models folder. It seems to be missing as few files: system_hours, system_alerts, system_calendar, system_regions, system_pricing_plans, geofencing_zone

We should include a Readme file that explains that the files were manually generated (link to the generator) as well as some basic instructions on how to use it. Also add it to the main Readme!

Hyped to keep expanding the gbfs dev tools

@dersvenhesse
Copy link
Contributor Author

Thanks for your response and sorry for my delayed activity. I applied your notes and finished the file for v2.3. Does that look good to you?

Unfortunately, most generators from JSON schema to OpenAPI don't deliver satisfying results. I will look into automation and more versions (presumably v3.0) in an upcoming PR.

@dersvenhesse
Copy link
Contributor Author

Do you have any feedback, @richfab or @Alessandro100?

@Alessandro100
Copy link
Contributor

I’m interested in the use case of the openapi.yml file(s). Would the main use case be to generate language bindings for other languages?

For language binding generation it would be advisable to generate directly off the official gbfs .json schemas found in the repo. Example for Typescript and Golang could be found in

  • scripts/generate_go_models.sh

  • scripts/generate_typescript_models.sh

I’m also curious on the scalability of creating openapi.yml files for future gbfs versions. For version 2.3 was there a lot of manual intervention?

@dersvenhesse
Copy link
Contributor Author

The are various use cases. A major one is the large amount of generators, which not only generate models but also nearly ready to use servers and clients. Take a look: https://openapi-generator.tech/docs/generators.

I manually created the file by copying attributes and descriptions from the json-schema, which is not future-proof. If the existing generators from JSON schema to OpenAPI won't deliver satisfying results, a custom converter script should do the job.

@Alessandro100
Copy link
Contributor

I understand the use case and utility. If the main utility would be to generate code, I would remove the ## Usage section in the readme as we don’t want to promote a generator and be accountable for it

We should break up the openapi.yml file so that each component has its own .yaml file which will link to the main one. This will set a good precedent going forward for future openapi version additions. We are on a good path!

@dersvenhesse
Copy link
Contributor Author

Thanks for the feedback. I originally added the ## Usage based on your comment on how to use the generator. Should I just link to their README or HOWTO instead?

Personally, I'd argue in favor of one openapi file instead of multiple. It is much easier to use with one file and many openapi meta information (server, description,...) and gbfs meta information (last_updated, ttl,...) will be redundant when using multiple files. Here is an real world example from ridedott.com with also one file, in that case even with multiple visibilities and versions.

@Alessandro100
Copy link
Contributor

Thank you for linking the ridedott example, it was insightful. It gave us the idea that we can link JSON schema’s directly in the openapi.yml like it’s done in ridedott



from $ref: '#/components/schemas/SystemPricingPlansJson’
to $ref: https://raw.githubusercontent.com/MobilityData/gbfs-json-schema/refs/heads/master/v2.3/system_pricing_plans.json

This would allow us to cut the entire component section of the current openapi.yml, reference the original schema, and make it easier to upgrade to future version of gbfs. With this, we can keep a single openapi.yml file per version of gbfs

In the usage section of the readme, we should tell users to include in their base url in the openapi.yml. This is more advisable vs adding it as a property to a cli generator



servers:
  - url: https://api.example.com/v1
    description: Main API server

we could also mention that since it’s possible that the base url could possibly not be the same for all links, that they could override the main server endpoint for a specific path

ex:


paths:
    /gbfs:
        get:
            tags:
                - gbfs
            operationId: getGbfs
            servers:
                - url: https://modified-url.example.com/v1
                  description: Production server (uses live data)


Lastly, for the promotion of the generator, I would give 3 generator tool options and phrase it more like “here are some tools you can use with the openapi.yml file” and link to their page without going into detail of each one

@dersvenhesse
Copy link
Contributor Author

dersvenhesse commented Oct 8, 2024

Ah, nice catch, that seems pretty neat. I will look into this and update this PR in within the next days.

Copy link
Contributor

@Alessandro100 Alessandro100 left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for taking the time to improve gbfs tools ✅

@Alessandro100 Alessandro100 merged commit 4610d04 into MobilityData:master Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants