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

Add OpenAPI Specification support (Version 2.0) #347

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Add OpenAPI Specification support (Version 2.0) #347

merged 1 commit into from
Jul 9, 2018

Conversation

djezzzl
Copy link
Contributor

@djezzzl djezzzl commented Jun 6, 2017

The PR adds a support for OpenAPI Specification. This is already used by some projects and it works. There are some things like oauth2 security not implemented yet. But most of the required functionality to able to work with famous Swagger UI are done.

@djezzzl djezzzl mentioned this pull request Jun 7, 2017
@mroach
Copy link

mroach commented Dec 1, 2017

Are you still working on this, or do you want a helping hand? I'd love to replace our manual generation of Swagger docs with this.

@djezzzl
Copy link
Contributor Author

djezzzl commented Dec 1, 2017

@mroach Hi, right now I stop work on improving it because have no free time. I'm using it in few projects already but it still messy and little bit tricky. So if you can finish it and totally merge to main repo it will be very nice!

@stephanebruckert
Copy link

stephanebruckert commented Jan 10, 2018

Thanks for that it works well, using your branch in my Gemfile for now! Would be very happy if that could be merged to master one day

@djezzzl
Copy link
Contributor Author

djezzzl commented Jan 10, 2018

@stephanebruckert thank you. Nice to hear that. I also still using that. I'll write docs in README later because it even includes few more helpers and features.

@jwg2s
Copy link

jwg2s commented Mar 13, 2018

Hey @djezzzl and @stephanebruckert stumbled upon this branch... Down to help out with this as well. We've been using this gem to generate apiblueprint, but swagger has some obvious advantages. Are you guys still working on anything here?

@djezzzl
Copy link
Contributor Author

djezzzl commented Mar 13, 2018

@jwg2s Hello, I'm just using it. Because for my tasks it's complete and ready to use. I'm not sure I did kind of demo in example project or document new created helpers so if you need any help just ask.

@jwg2s
Copy link

jwg2s commented Mar 19, 2018

@djezzzl been using it now with success. Have any pointers for how to include example request bodies? That's the biggest thing I see missing that would definitely be useful. Thinking this: https://swagger.io/docs/specification/2-0/describing-request-body/

EDIT: Was actually reading the v3 swagger format, seems like v2 doesn't support example request bodies?

@djezzzl
Copy link
Contributor Author

djezzzl commented Mar 20, 2018

@jwg2s @stephanebruckert . It's already possible. I make it like following:

post '/accounts/:account_id/projects' do
    with_options with_example: true, in: :body do
      parameter :name, 'The name of the project.'
      parameter :site_primary, 'The main domain of the project, including the protocol.'
      parameter :alert_emails, 'An array of email addresses to send alerts to.'
      parameter :alert_setting, "Choices: always never\nWhen to send alerts.", default: 'always'
    end
    
    let(:name) { 'example.com' }
    let(:site_primary) { 'https://www.example.com' }
    let(:alert_emails) { ['[email protected]', '[email protected]'] }
    let(:alert_setting) { 'never' }

    header 'Content-Type', 'application/json'
    let(:raw_post) { params.to_json }

    context '201' do
      example_request 'Create project' do
        expect(status).to eq 201
        expect(response_body).to eq <expected response here>
      end
    end

    context '422' do
      let(:name) { nil }
      let(:site_primary) { nil }

      example_request 'Invalid data provided' do
        expect(status).to eq 422
        expect(response_body).to eq <expected response here>
      end
    end
  end

@jwg2s
Copy link

jwg2s commented Mar 20, 2018

@djezzzl thanks for getting back to me so quick. I've been making some changes to handle edge-cases that we've run into which I'm happy to share upstream if you'd like.

EDIT: Nevermind, I see that it's generated from a combination of :with_example => true and having :value => set to something, right?

@djezzzl
Copy link
Contributor Author

djezzzl commented Mar 21, 2018

@jwg2s :with_example => true Will just add to the end of description of parameter its example value.
E.g.

parameter :name, 'The name of the project.', with_example: true
let(:name) { 'example.com' }

Description of that parameter in docs will become: The name of the project. Eg 'example.com'

What you need to is to send as body of request. You just need to set parameter as in: :body More information can be found here: OpenAPI parameters.

@jwg2s
Copy link

jwg2s commented Mar 26, 2018

@djezzzl thanks for that explanation - definitely helped. Is there any reason we're not deriving example values / types from the request that's generated? For example, with the API blueprint formatter, we've got example request URLs/params/bodies that we can use to automate a lot of this at the gem layer as a default, and developers would only have to override.

If you were going to go about that, where would you start? If you point me in the right direction, I can knock it out and open a PR to your swagger branch

@djezzzl
Copy link
Contributor Author

djezzzl commented Mar 26, 2018

@jwg2s Why do you think we are not? We actually do. You can skip writting type / value of parameter if you provide an example value using let. If I got you wrong or forgot something please provide your example and I'll give you detailed answer.

@karloscodes
Copy link

Any update on this?

@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 4, 2018

Unfortunately, not from me at least. I still didn't get any response from Owner/Contributors. So I didn't update the PR to the latest version of the gem but it's working 👍

@oestrich
Copy link
Contributor

oestrich commented Jul 4, 2018

Sorry for the delay on this.

I'm mostly concerned about this set of numbers +2,285 −32, I don't know when I'll have time to give this a look over. I also saw your checklist at the top and it hasn't been filled out yet so I thought you were still working on it.

That said, if this is working for people I am fine merging it after the conflicts are fixed.

@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 4, 2018

@oestrich, hard to call that "delay" but no problem :)

I'm not working on this right now. So, sorry, I left a checklist for myself but most of important and must have logic is working. We can try to pair/call when I update this PR that I can fast explain you all the changes. I also want to know your opinion on the idea at all.

So if you can confirm that you'd like to merge this I can prepare a PR. Can you?

@oestrich
Copy link
Contributor

oestrich commented Jul 4, 2018

@djezzzl Yeah I am behind getting a swagger writer in.

I just did a quick scan of this and it looks good. Two things though, there's some commented out specs in spec/writers/swagger_writer_spec.rb and then I think you commited a test generation of docs? There's a doc folder that looks empty.

The rest seems good 👍, fix those merge conflicts and once travis is passing I can merge in and I'll do a major bump since this is a pretty big new thing.

@jakehow
Copy link
Member

jakehow commented Jul 4, 2018

@djezzzl are you proposing making a different PR from this one?

At first glance it seems there may be change in this PR that seems unrelated to the functionality. I think either @oestrich or myself are happy to give feedback, but if you need help bringing this over the finish line its best to ask specifically for something.

Docs would help too, but I don't think either of us has projects using Swagger, so would be up to others interested to help you get this finished.

In general, I think think the idea of adding swagger/OpenAPI support as a feature should be merged.

@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 4, 2018

@oestrich @jakehow Wow, thank you, guys! Glad to see your interest.

I won't make different PR but I'll update this one and also will improve it. Because the first time I started on it I was needed it ASAP. Now I can make things more clearly.

So, I hope by the end of this week I'll prepare the PR. If not this, the next should be enough.

@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 5, 2018

Just to confirm:
even if CI will be green in someday, don't merge until I say it's completely ready.

@djezzzl djezzzl changed the title Add swagger 2.0 support Add OpenAPI Specification support (Version 2.0) Jul 7, 2018
@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 7, 2018

@oestrich @jakehow Hey guys, it's ready to be merged 👍

@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 9, 2018

@oestrich @jakehow Hey guys, do not disappear please, I tried hard to make it ready ASAP.

@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 9, 2018

@kurko @marnen @jonathanpa @samwgoldman Hi guys, somebody of you able to merge this PR?

@oestrich
Copy link
Contributor

oestrich commented Jul 9, 2018

@djezzzl Hey, sorry I didn't forget about this. Couldn't get to this until after the weekend.

@oestrich oestrich merged commit 229fe27 into zipmark:master Jul 9, 2018
@djezzzl
Copy link
Contributor Author

djezzzl commented Jul 9, 2018

@oestrich No problem. Great, it's merged 👍

@djezzzl djezzzl deleted the add-swagger-support branch July 9, 2018 18:00
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.

7 participants