-
Notifications
You must be signed in to change notification settings - Fork 191
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
[PROPOSAL] Experiment with using Smithy API spec to generate missing APIs #284
Comments
Options ExploredPre-existing OpenAPI generatorA quick inspection of the existing Java generator for openapi-generator shows it is generally only workable when generating completely from scratch, as it expects to be generating a whole client, project files and all. There is some flexibility within the CLI tool to reduce surface area of what you desire to generate, however not to level necessary. All the generated code is also specific to whatever HTTP & JSON libraries have had templates written for. Verdict: 👎 Any generated code would be completely independent of the existing client which is not desirable. Custom OpenAPI templates/generatorThe OpenAPI generator tool does allow for customising templates of an existing generator, or writing a whole new one. This would allow for being able to generate code that correctly integrates with the custom JSON and transport code used by the existing client. However an architectural limitation of the custom generators means it's only possible to generate files per service (ie. client class) or per model, not per API operation. Meaning it's unable to generate the "Request" classes like currently exist in the client that encapsulate body, query params & endpoint in one class. (This may be less of an issue in other languages where a file maps to a module containing multiple types rather than a single type/class) Verdict: 👎 Generated code could be inter-operable with existing client, however would have an entirely different calling convention, making it unsuitable as we want to work towards regenerating the entire client with minimal breaking changes. Custom Smithy Build plugin/generatorSmithy Build allows building plugins (written in Java/Kotlin) that are passed the Smithy model and allow processing it in whatever manner you desire, in a way that is orders of magnitude more flexible than the OpenAPI generator approach. Due to the ability to process the model from the start with custom code it is relatively easy to generate code in the same structure as the existing client. As we'd be dealing with the Smithy model directly it'd be possible to implement and handle custom Smithy traits if desired. Unfortunately the means by which to create these plugins is not documented, and so required studying the smithy-kotlin & smithy-rs plugins to understand how it works. The requirement of using Java/Kotlin will be fine here, but may be problematic for other language clients. Verdict: 👍 (mostly) Will allow us to generate (near-)identical code to the rest of the client, without having to deal with parsing and manipulating the model from scratch. Comes at cost in onboarding new contributors due to lack of documentation. Unsolved Issues
|
I created a draft PR with my current code for the Smithy build plugin generator #318 |
Good start!
|
No it doesn't have to be Kotlin, it could be written in Java. Kotlin just has the advantage of being less verbose, and the other plugins I was researching off of were in Kotlin so made it easier.
None that I've found, Smithy has no Java generator, and all the OpenAPI generators expect to be generator from scratch and naturally don't know to create the specialised output desired for the client.
Yeah the actual getting it to run inside this repo hasn't yet been done yet, as it's a bit tricky with the spec living in a different repo. And I've been experimenting on the remote store api which doesn't yet exist in the client, so not replacing anything existing. This is the spec & config I've been experimenting with so far opensearch-project/opensearch-api-specification@main...Xtansia:opensearch-api-specification:experiment/codegen and this is the output for that config https://gist.github.com/Xtansia/f3eaaf8aeb1121299484e9e8a3513700. Once I get some proper semblance of it running contained within the repo, then I'll update the PR so that it includes the generated output. |
Thanks @Xtansia! Looks like great progress. Add license headers to those generated files, and a header that says that the file was generated ;) |
Thanks @Xtansia, few clarifications from my side:
It seems like one of the advertised Smithy features is the ability to generate the OpenAPI spec which could be fed to openapi-generator to generate a client for a wide variety of the programming languages and frameworks
This is not true: the generator is highly configurable in what and how should be generated. It could generate only models, models etc (used it in the past and 100% sure of that).
@dblock @wbeckler This is personal perspective: there are many specs like Smithy, backed by different vendors. The OpenAPI is vendor neutral, widely supported / accepted / understood specification, which seems to be a reasonably good way to share contracts for web APIs. The OpenAPI v3.1 brought a full alignment with JSON Schema draft 2020-12, this is big deal in a way how large pieces (models) could be described and referred to. I think it would be very beneficial to use OpenAPI spec as a basis for client code generation, just my 5 cents |
You are correct and that's the foundation of how I tested with openapi-generator, all of these experimentations use the Smithy spec as its starting point.
That's correct, and is what I was hinting at by "There is some flexibility within the CLI tool to reduce surface area of what you desire to generate, however not to level necessary.", I was unable to find quite the level of filtering I desired, as I wanted more advanced filtering than "only models" but potentially I missed something and did it a disservice.
I believe there was an issue somewhere to discuss the benefits of each at some point, I'm unsure where though. It would be possible to generate from OpenAPI spec, but as far as I can tell to get code that matches the existing Java client would mean doing it from near-scratch. |
I think you mean this one, opensearch-project/opensearch-api-specification#30 ? Just recently we have had the discussion with @wbeckler regarding Smithy w/o OpenAPI, and one of the ideas we where thinking about is to have POCs to compare both approaches. I believe yours is the great one to showcase Smithy case. |
For reference this is as far as I got with the custom openapi-generator templates/generator before hitting the fact I could not generate code in the same structure as the existing client: main...Xtansia:opensearch-java:experiment/codegen-openapi |
@VachaShah @reta Here's the example output for that openapi-generator, https://gist.github.com/Xtansia/b88ad73bde610a5e677b0b293c6155ea, while it's obviously not complete functional code, the main thing to notice is how the models only represent the body of the request/response, and the query params are shifted to the operation method and "flattened" (ie. if there's n query params then the method will have n+ params) |
Mustache templates seems way easier than kotlin code, isn't it? |
That's entirely subjective, and depends on the complexity of the template. I personally find code generally easier to reason about. But it's not really templates vs code, you could go to the effort of bringing mustache into the smithy plugin if you wanted to. The problem is entirely the generated code compatibility with the openapi-generator. |
@Xtansia thanks for sharing, I picked up your work here #336, there are no generator changes at the moment (no need, templates are just enough) but I hope to finalize the PoC by getting as close to exiting generated model as possible. |
Thanks for putting together that more complete PoC for OpenAPI @reta! It has been really helpful. I wanted to distil a summary of where we're currently at with regard to the pros & cons etc. I've tried to be even-handed, but it's possible I've missed some points, so do let me know if you've got anything to add or correct and I'll make sure to update it. Explored Approaches / Proof of ConceptsSmithy Build PluginPoC PR: #318 Pros
Cons
Off-the-shelf OpenAPI Generator With Customised TemplatesPoC PR: #336 Pros
Cons
Other Potential ApproachesThe following approaches may be worth exploring to bridge the gap between the two current PoCs: Modify Smithy Build Plugin To Utilise TemplatesProvides only slight differentiation from current Smithy Build Plugin approach, in that it would allow for making some of the code generation more human friendly, but at the cost of having to integrate templating and mapping a cleaner model to pass into Mustache. From Scratch OpenAPI GeneratorCreate a minimal CLI tool that uses existing OpenAPI model libraries to load and parse the spec, manipulating it and then passing into Mustache templates. Desirable AttributesThere's likely some common desirable attributes we have for whatever the solution ends up being, and I think it'd be good to solidify those.
|
@Xtansia thanks a lot for putting everything together, I think the summary is quite fair. The only things I would suggest to add to OpenAPI pros: |
Is there some consensus on what to choose? I think the way we decide is that the person doing the work makes the call. |
Not only is a diversity of approaches possible, it may be necessary, because smithy can't directly generate most languages. For example, there's a first attempt at Ruby based on OpenAPI: opensearch-project/opensearch-ruby#139 The question here may be just whether the smithy + gradle plugin approach is the right approach for incremental java client API generation. As a byproduct of this approach, the clients for non-smithy languages would rely on an OpenAPI spec generated from the Smithy models. |
I don't think we have a clear winner :D (as in most cases in tech), one of the most convincing benefits (for me at least) of using OpenAPI - you just need to know one thing (which is an open specification), and this is it (whereas with Smithy, you have to know both).
That is very possible, the thing I am personally concerned is that we would definitely run into Smithy -> OpenAPI generation issues and essentially would have to deal with that. |
What main issues do you foresee with using Smithy as the definition language and then converting to OpenAPI, other than the adoption/learning hurdle for Smithy? Do you know of any commonly used definition language for OpenAPI that don't involve writing thousands of lines of YAML/JSON directly? 😅 |
Thanks @Xtansia
This is yet another generator (== code), right? And I don't know at least 2 things: a) how good it is b) does it support OpenApi 3.1 (this would be very handy for Json Schemas)
:D the good part - it is rarely needed to modify anything at all (I do have real world examples of that from the past), but we want things "our way" (for a reason), so here we are 😄 |
Here's my take on Smithy vs OpenAPI. Smithy is superior in terms of features and maintainability. However, it has not been widely adopted like the well-established OpenAPI. It lacks parsers for many languages, and most contributors are not familiar with it. I used to think that Smithy was a lot more readable (well it is, from the API Design standpoint). But weirdly enough, when writing the PoC generator for Ruby, I much preferred looking at the JSON object representing an endpoint in OpenAPI over its Smithy equivalence. I had the overview of the entire endpoint in one place in OpenAPI VS referencing several files in Smithy. Where do we go from here?1. Smithy all the way (for both spec and code generation)This requires contributors to learn Smithy and its limited toolset to be able to contribute to the project. If I were a programmer looking to contribute to an Open Source project and learn something new along the way, I'd more likely pick an OpenAPI project over a Smithy one:
More importantly, we might risk alienating developers who have experience in maintaining OpenAPI specs, and in writing code generators for OpenAPI. The biggest advantage of this approach is utilizing all of Smithy unique features without worrying about whether they are translatable to OpenAPI. 2. Only use Smithy as a tool to write OpenAPI Spec more efficientlyIn other words, we keep maintaining the opensearch-api-specification repo as a Smithy project with Gradle plugin to generate OpenAPI Spec. We will avoid Smithy features that are not translatable to OpenAPI.
As @Xtansia has pointed out, writing JSON and YAML for OpenAPI from scratch is a pain. Smithy's Mixin and Traits make it a lot easier and faster to maintain than editing OpenAPI Json/Yaml files. Though there are 3 counter points that weaken the advantages of this approach:
3. OpenAPI all the way (scratch off what we have so far with Smithy and start over)Even though we spent quite a bit of time setting up the Smithy spec repo (You can see a lot of care for this project in the Developer Guide <3), there are barely any endpoints in the spec, and most are incomplete. So, starting over with OpenAPI is not too wild of an idea. In fact, this might be the best approach, in my opinion:
|
Thanks @nhtruong
It is funny you mentioned that, I just posted the same kind of question / comment here opensearch-project/opensearch-api-specification#74 (comment) this morning. Yes, @Xtansia is very right, the
|
@reta it's quite a funny coincidence. I discovered that ES spec while studying this commit to review this PR. Told @wbeckler about it yesterday and he told me this morning that you just brought up the same thing 😆 Anyway, it's cool that OpenAPI 3.1 can do that. I'll have to look into it. But I have a hunch that we should still convert it to native OpenAPI spec to make it easier to maintain, more consistent with the spec for future endpoints, and more compatible with off-the-shell parsers. (Again just a hunch. Still need to do research on it) |
Just wanted to share I'd started looking at writing a PoC "from-scratch" OpenAPI based generator, as I'd mentioned in a previous comment. It's still not quite at the point I want to create a draft PR, but if you want to have a look, it's pushed up here: https://github.com/Xtansia/opensearch-java/tree/experiment/codegen-openapi-scratch/java-codegen |
@reta I'm working on converting the old OpenSchema API Spec to OpenAPI Spec. I misunderstood what you said by using the JSON schema. JSON schema is only meant for the request/response bodies, not the entire API spec right? |
thanks @nhtruong
Correct for request/response bodies, but the spec has own schema here [1] |
I have raised a draft PR with my working PoC for the from scratch OpenAPI based generator: #366 |
Hi, Out of the options listed in this response(#284 (comment)), I think the 2nd option might be a good way forward, i.e we can define the API models in Smithy and for use-cases which require OpenAPI we can consume the OpenAPI build output from Smithy. We (myself and pgtgrly) just wanted to add some more context on why we had proposed to use Smithy instead of OpenAPI for the opensearch api specification repo (opensearch-project/OpenSearch#1590). Some of the key reasons were:
In order to speed up API model generation in Smithy, it should also be possible to programmatically generate the Smithy initial API model files using the OpenSearch rest-api-spec JSONs (https://github.com/opensearch-project/OpenSearch/tree/main/rest-api-spec/src/main/resources/rest-api-spec/api). Once the inital model files are created, these can be enhanced to include the request and response bodies. We would also like to add the discussion this thread by Disney Streaming about the reason they went for Smithy over OpenAPI for it’s highly customizable codegen capabilities (https://www.reddit.com/r/scala/comments/zftfwf/comment/izemlqq/). |
I've generated the OpenAPI Spec from the legacy spec (with come caveats). The specs and some of my findings when working on this can be found here. @sachetalva Thanks for the input. Some of the points you mentioned above are not unique to Smithy:
Regarding "[Disney Streaming] went for Smithy over OpenAPI for it’s highly customizable codegen capabilities", I think you mislinked the thread? I don't see it in the thread you quoted. Regardless, we will have to write our own client generators for OpenSearch due to the level complexity of its API and the current clients:
Sure we can customize an existing codegen to handle those, in theory, but at that point, I think it's a lot simpler to write our own generator to retrofit new endpoints into the current clients' structures. |
One thing I just discovered when translating the legacy spec to OpenAPI spec is that, Smithy has another advantage over OpenAPI: The ability to group related endpoints into one location (Similar to that of the legacy spec). This can't be done with OpenAPI because of the restrictions on Path and Operation references. |
@Xtansia what do we want to do with this issue? |
Closing this issue as the spec is now solely authored in OpenAPI and #366 has been merged |
What/Why
What are you proposing?
Performing an experiment using the WIP Smithy API specification for OpenSearch to generate missing functionality in the client for a small API without re-creating the entire client.
What users have asked for this feature?
What problems are you trying to solve?
What is the developer experience going to be?
A script/tool that can be run to automatically fetch the API spec and generate the chosen code.
Are there any security considerations?
n/a
Are there any breaking changes to the API
This experiment is only successful if it does not break compatibility.
What is the user experience going to be?
Same as developer experience.
Are there breaking changes to the User Experience?
See above.
Why should it be built? Any reason not to?
The learnings from this experiment, if successful:
If the experiment is unsuccessful, it will not be merged, negating almost any potential downsides.
It may not be feasible to integrate generated code from an off-the-shelf generator with the pre-existing client code.
What will it take to execute?
/_remotestore/_restore
API as a low-risk low-touch use-caseThe text was updated successfully, but these errors were encountered: