search

esh-b / RAML-to-Swagger-Converter

A java project to convert RAML 0.8 definition to Swagger 2.0 definition
BSD 3-Clause "New" or "Revised" License
17 stars 9 forks source link

Support converter usage as library as well as a standalone application #4

Closed sslb closed 7 years ago

sslb commented 7 years ago

Hi,

This is a bigger set of changes, let me collate the key changes here for you to review:

The unit tests are covering 96% of the code and validate key behaviours like parameter type resolution, the recursive resolver's correct behaviour, JSON validity and both entry points to the converter.

We might want to further extend them with RAMLs from the Example directory, but I'll write those later.

Have a look at it and if you agree with the changes, then we're one step ahead. Don't merge the other PR if you decide to merge this.Those changes are included in this PR as well.

Szabolcs

sslb commented 7 years ago

I have a question though.

This last commit fixes a bug(?) in the original code, when "path" parameters are put in the top level of an endpoint and not under the parameters block within each method.

It seems like the swagger standard requires those under the method's ("get", "post", etc.) parameters block. I have discovered this working with WSO2 API gateway, while I tried to import the swagger output. It discarded all these path parameters. I've looked into the swagger standard and fixed it.

My question is: did you have any particular reason why these were handled separately (because the code seems to be handling path parameters different)? Want to understand if you had a reason doing this way or is it just a smaller bug?

The generated swagger now properly processed by the WSO2 API gateway.

esh-b commented 7 years ago

@sslb...Actually, I don't remember the reason for having the path parameters below the endpoint (and not below the methods particularly). But I did that for some purpose. Maybe that time, Swagger had some issues with it (they might have updated it now). For some of the test cases (around 10 types of RAML files) that I used, the resulting Swagger had some syntax error. That's why I had put them under the endpoint directly. But is Swagger showing that as a validation error (having path params below the endpoint)? I dont think I got any error when I wrote the code. I am not sure whether Swagger updated any of it's definitions now.

Now, I am concerned about it. Thanks for bringing it to the table btw. So, first can you check the output (Swagger file) for any validation error in the swagger website for your input RAML file? I mean run it on the code where you modified the path param part. Can you please take around 5 or 6 types of RAML files and check the resulting swagger output in the swagger website for any validation errors on your code? That would be really helpful.

Thanks in advance. :+1:

sslb commented 7 years ago

Will do a comprehensive swagger validation now. I did a massive RAML conversion and noticed that some include files are not properly read if they are in directories. That's due to the RAML builder used not our code. They have this in their code:

new CompositeResourceLoader(new ResourceLoader[]{new UrlResourceLoader(), new ClassPathResourceLoader()})

This results that anything other then being in the current directory (say your schemas in a dir 'schemas', etc.) will fail because the UrlResourceLoader requires the Java file: designator in front. That's obviously not good, so I extended the code to support passing a resource loader to RamlDocumentBuilder(). This way the caller decides.

In App.java now properly passing in a UrlResourceLoader and a FileResourceLoader which results in correct behaviour.

sslb commented 7 years ago

I tried 9 very different RAML files now and validated the swagger output and it checks out fine. I have tried inclusions from web resources (!include http://....) and from local resources too. They all check out.

As for the path parameters the validator fails if they are at top level and passes the file if they are in the parameters block. I guess I see the point why it is in the parameters block under the method. Imagine an endpoint having different data type (integer for get and string for post) for the same path parameter. Arguably it's poor design, but not unimaginable.

esh-b commented 7 years ago

/pet/{petId}: parameters:

This code passes the validation test (sorry for the bad indentation). Incase you are confused about it, the code has path params defined above all the methods. However, my bad I dont remember the reason for me doing that. Also, looks like OpenAPI 3.0 has come out recently which maybe the reason for these issues. But since you say it's working fine for different kind of RAML files (about having path params inside the methods block), let's go with that now. I will merge your request to the update branch. So, can you please send me a pull request for that branch?

sslb commented 7 years ago

Ok, so you want me to merge this last commit about the path params to PR#3 and we leave this one open?

sslb commented 7 years ago

It seems like their documentation for 2.0 also puts the parameters block within the method:

https://swagger.io/docs/specification/2-0/describing-parameters/

There's no indication that it's supported outside, but I might be missing something. As for the fore-mentioned WSO2 API gateway, it just simply ignore all path parameters if they weren't in within the method.

esh-b commented 7 years ago

okay..cool! 1) I want you to send me a new pull request (same as #4 ) to the update branch instead of master branch. That way, we could have some version control.

esh-b commented 7 years ago

2) I am still not sure about that path param part. But the above code passes the validation test..right? Anyway, let's get on with putting the path params inside the method block since you are sure(?) it's working. Send me a pull request to the update branch. I will close this and merge that.

sslb commented 7 years ago

Because we don't have a firm consensus on where the path param should go, shall we make it a command line option? Say --top-level-path-params, and to render it to both places an added --method-level-path-params and they would not be mutually exclusive and we could default to whichever but retain the logic for both...

esh-b commented 7 years ago

Nope..Not necessary. Let's go with having the params inside the methods block atleast for now.

esh-b commented 7 years ago

I am closing this and will merge #5