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

initialize gem #1

Merged
merged 1 commit into from
Nov 7, 2023
Merged

initialize gem #1

merged 1 commit into from
Nov 7, 2023

Conversation

paulsturgess
Copy link
Contributor

@paulsturgess paulsturgess commented Nov 1, 2023

This gem allows us to generate an OpenAPI schema of an Apia API.

It follows a similar convention to the apia-insomnia gem in that we can use Rack middleware to allow the schema to mounted on an endpoint.

For example, for a Ruby on Rails application:

module MyApp
  class Application < Rails::Application

    config.middleware.use Apia::OpenApi::Rack,
                          api_class: "CoreAPI::Base",
                          schema_path: "/core/v1/schema/openapi.json",
                          base_url: "http://katapult-api.localhost/core/v1"
  end
end

Where CoreAPI::Base is the name of the API class that inherits from Apia::API.

Why are we using v3.0.0 when the latest is v.3.1.0 ?

The OpenAPI generator does not support 3.1.0 (at least for Ruby yet).

So the specification is for version 3.0.0. Annoyingly in v3.0.0, having a request body against a DELETE is deemed to be an error. And this shows up in swagger-editor. However, after community pressure, this decision was reversed and in version 3.1.0 DELETE requests are now allowed to have a request body.

I have successfully used the Ruby client library to use a DELETE request with a v3.0.0 schema, so I don't think it's a big deal. We can bump to 3.1.0 when the tooling is ready.

What is implemented?

  • All endpoints are described by the spec.
  • ArgumentSet lookups with multiple methods of supplying params are handled
  • All the various "non-standard" Apia data types are mapped to OpenAPI ones (e.g. decimal, unix)
  • If include is declared on a field for partial object properties, then the endpoint response will accurately reflect that
  • Array params for get requests work in the "rails way". e.g. user_ids[]=1,user_ids[]=2
  • swagger-editor works, so we can use the "try it out" feature (including bearer auth)
  • Routes that exclude themselves from the Apia schema are excluded from the OpenAPI output
  • Endpoints are converted into "nice" names so that the generated client code is more pleasant to use
  • Apia types (enums, objects, argument sets, polymorphs) are implemented as re-usable component schemas
  • The spec is good enough to generate client libraries in various programming languages

What isn't implemented?

Any other known issues?

  • We can't have deeply nested objects in GET request query params. This just isn't defined by the OpenAPI spec. There's GitHub issue about it. I don't believe we can do much here and probably we don't need to.
  • File uploads are not implemented, but I don't think we have a need for that.
  • We do not try to be too 'clever' when the endpoint field uses include to customize the properties returned. e.g. include: '*,target[id,name]' in this case we could actually use a $ref for the object referenced by the *. But instead, if a field uses include we just declare an inline schema for that field for that endpoint.
  • The example API has been copied and expanded from the apia repo. Some of the additional arguments and ways the objects have been expanded is nonsense, but they're there just to ensure we execute all the code paths in the schema generation. Maybe we could come up with a better example API or perhaps we just don't worry about it.

@paulsturgess paulsturgess self-assigned this Nov 1, 2023
@paulsturgess paulsturgess force-pushed the init-gem branch 3 times, most recently from efaa479 to 300509c Compare November 2, 2023 11:57
.rubocop.yml Outdated
- https://dev.k.io/rubocop/rubocop.yml

AllCops:
TargetRubyVersion: 2.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this is different from the 3.2.2 in the .ruby-version but it matches the required ruby version in the gemspec.

Maybe we should bump up the version in the gemspec, but I thought it seemed a shame to restrict this gem from working with older apps.

Copy link

Choose a reason for hiding this comment

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

I would say we might want to target 2.7, as it would yield warnings for things that'll become issues in 3.x. While still running the test suite against 2.6 as well as more recent versions just to validate it's still usable with 2.6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, changed to 2.7 👍

@paulsturgess paulsturgess force-pushed the init-gem branch 2 times, most recently from 6d195cc to cc2d714 Compare November 2, 2023 13:19
Comment on lines +7 to +15
TODO: Replace `UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG` with your gem name right after releasing it to RubyGems.org. Please do not do it earlier due to security reasons. Alternatively, replace this section with instructions to install your gem from git if you don't plan to release to RubyGems.org.

Install the gem and add to the application's Gemfile by executing:

$ bundle add UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG

If bundler is not being used to manage dependencies, install the gem by executing:

$ gem install UPDATE_WITH_YOUR_GEM_NAME_PRIOR_TO_RELEASE_TO_RUBYGEMS_ORG
Copy link
Contributor Author

@paulsturgess paulsturgess Nov 2, 2023

Choose a reason for hiding this comment

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

Presumably we should do what is said here and not put the gem name here until after we publish? 🤔

Copy link

Choose a reason for hiding this comment

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

Probably yeah. I assume it's to avoid someone claiming the gem name based on what's in a public github repo before the gem name as been actually taken by us.

@paulsturgess
Copy link
Contributor Author

@jimeh I'm marking this as ready for review. This is obviously very much still "work in progress", lots of rough edges and room for improvement. But there's definitely enough here to give us a good feel for whether this is something we want to spend more time on.

I'm aware I don't want to spend ages polishing it before there's been time enough to evaluate and make a decision to continue to move forward with it.

I also don't want to be the "gatekeeper" of this entire implementation 😁

For anyone else following along, the client libraries can be found at:
https://github.com/krystal/katapult-openapi-clients

Copy link

@jimeh jimeh left a comment

Choose a reason for hiding this comment

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

I have not had a chance to look through all of this. But the Schema it generates, and the clients that can be generated from it looks promising.

The only thing I think that might be useful, but probably in a separate PR, is to tag endpoints with more details than just the API name, like "Core". Instead also include the group names that endpoints are bundled under. If this should all be one tag, or many tags though, I'm not sure.

@paulsturgess
Copy link
Contributor Author

paulsturgess commented Nov 7, 2023

The only thing I think that might be useful, but probably in a separate PR, is to tag endpoints with more details than just the API name, like "Core". Instead also include the group names that endpoints are bundled under. If this should all be one tag, or many tags though, I'm not sure.

Originally I didn't tag anything. And I only added the 'core' tag because in the Ruby Client it put everything under a class named DefaultApi, which felt odd to me. By tagging everything 'core', it put it under a class name called CoreApi instead.

I think the idea is that you could use multiple tags to split different parts of the api out into different classes.

So anyway, my main point is... we'd have to be careful what we do with tags. As it may introduce breaking changes in the clients.

@paulsturgess paulsturgess merged commit 4815a20 into main Nov 7, 2023
@paulsturgess paulsturgess deleted the init-gem branch November 7, 2023 10:54
@jimeh
Copy link

jimeh commented Nov 7, 2023

@paulsturgess Agreed about the need to be careful with how we use tags. Potentially then groups in Apia are not suitable for a tag source in OpenAPI, as you can move endpoints between groups without changing the request path. Essentially group changes are not by themselves breaking changes in Apia, but they would be in OpenAPI if we used the group as part of the tag.

Somewhat annoying, but it makes sense :)

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