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

Export OpenAPI specification for our own VTN #17

Open
pohlm01 opened this issue Oct 7, 2024 · 16 comments
Open

Export OpenAPI specification for our own VTN #17

pohlm01 opened this issue Oct 7, 2024 · 16 comments
Assignees

Comments

@pohlm01
Copy link
Member

pohlm01 commented Oct 7, 2024

No description provided.

@pohlm01
Copy link
Member Author

pohlm01 commented Oct 14, 2024

https://crates.io/crates/utoipa/4.2.3 might be helpful

@pohlm01 pohlm01 moved this to Todo in OpenADR 3.0 Plan Oct 14, 2024
benjaminedwardwebb pushed a commit to benjaminedwardwebb/openleadr-rs that referenced this issue Oct 26, 2024
…ions/codecov/codecov-action-4.5.0

Bump codecov/codecov-action from 4.4.1 to 4.5.0
@benjaminedwardwebb
Copy link
Contributor

Hey I've been looking into this issue and I wanted to clarify what the expectations are.

Some ways to "export" an openapi spec that come to mind are:

  • export an openapi.json file as a build artifact (probably using the approach mentioned here -- via a custom binary that depends on openadr-vtn just to write the definitions to a file)
  • serve an interactive swagger ui endpoint at /docs/ui
  • serve the raw openapi JSON file at /docs/openapi.json

I think all three ways are useful and relatively easy to do (that is what I would default to without any team feedback). I also listed them in order of usefulness/importance, according to my own personal opinion (if for any reason we wanted to only implement a subset).

But what does the team think?

Also, if someone is already working on this or there are plans by someone to pick it up, please let me know and I'll see if I can be helpful in some other way.

@pohlm01
Copy link
Member Author

pohlm01 commented Oct 28, 2024

Hi Ben,

thanks a lot for your initiative!
Our intention was to export the OpenAPI specification during compilation as a build artifact.
The third point you are mentioning, serving an openapi.json during runtime, seems helpful as well.

We are skeptical about serving the UI though, as we think this would introduce a lot of additional dependencies, and serving a web page feels wrong in an API. The additional value seems limited to us, as web pages like https://editor.swagger.io/ exist and most IDEs can handle the OpenAPI specification as well.

The link to utoipa was just a suggestion, and we have not investigated ourselves how to tackle this issue yet, so feel free to suggest other solutions as well, if you know anything better.

I don't know of anyone working on that already.

I'm happy to help with any further questions or suggestions, and looking forward to a PR from you!

@benjaminedwardwebb
Copy link
Contributor

Awesome, thanks for the feedback! I'll work on exposing the openapi JSON via a build artifact and endpoint, then.

I looked around only briefly and utoipa was the best I could find. However, I'll do a slightly more thorough analysis of the options available here before continuing down the path with utoipa and submitting a PR.

If you're interested to see what a very initial application of the utoipa crate to this task looks like you can check out the branch where I'm currently working (obviously bearing in mind that it's a work in progress).

@pohlm01
Copy link
Member Author

pohlm01 commented Oct 29, 2024

Feel free to also open a draft pull request even in an early stage of development, if you like.

@benjaminedwardwebb
Copy link
Contributor

benjaminedwardwebb commented Oct 30, 2024

So I think there's three reasonable options for maintaining openadr-vtn's openapi spec:

  • manually maintain the openapi spec in yaml
  • generate the openapi spec with utoipa
  • generate the openapi spec with aide (and schemars)

Here are my personal, informal thoughts on each option after reading a lot of GitHub issues (more journalism than science :)).

Normally I'd avoid manually maintaining a spec in yaml if the option to generate it from code exists. However, in this case it may be appropriate. The api is not a moving target -- it will be similar to the open standard we're implementing, and any changes to the standard will be small, slow, and well-communicated. Of course, any time you have two separate definitions of the same interface (openapi spec and service code), it's an opportunity for bugs -- but perhaps we could mitigate this with a test suite that generates a client from the spec and uses this client to interact with the service, ensuring the spec and code agree. Also, it's always nice to avoid a dependency.

The utoipa crate seems to be the most mature, popular, documented, and maintained crate in the rust ecosystem for generating openapi specs from web service code. It supports multiple frameworks, not just axum. But it has one big downside: it's basically like maintaining the spec manually, except instead of writing the spec in yaml you are writing it in rust macros. This is still an improvement because it brings the spec closer to the code. Also some parts of the json schemas can be generated for you, but ... there's information in the code's types and macros that utoipa doesn't always use. For example, utoipa's ToSchema derive macro won't use information defined in validator macros on a struct field, so you may need two definitions for something like minimum=1 and maximum=50 on a field -- once in a validator macro and again in a utoipa::path macro (and any time you have two separate definitions of the same interface ... 🐛🐛🐛). Ultimately it requires a lot of boilerplate. But on the other hand, since it's so popular maybe it will get better over time. It's also pretty simple and easy to use.

The aide crate is not as popular and recently it's maintainer transferred some responsibilities to a contributor because they felt they didn't have time to devote to the project. However, it has its fans -- multiple poeple offered to help out with maintenance on the GitHub issue about it. The crate's json schema generation relies on the schemars crate, which is very popular according to crates.io/crates/schemars. This is the main thing I think aide has going for it, because schemars seems to integrate well with serde and validator in the way utoipa doesn't. Overall I just really appreciate that schemars explicitly aims for compatibility with serde, and I suspect using aide will require a lot less boilerplate (but maybe at the cost of a few more slightly confusing compiler errors).

I don't know which of these three options is most appropriate for this project.

I don't have experience using the crates in other projects, so if anyone does please chime in. I'm working on two drafts, one using utoipa and one using aide, that generates the spec for a small subset of the api so we can compare their usage side-by-side.

Let me know if you have any thoughts or if there's anything in particular you think would be good to check is supported by these crates. I should have the draft PRs ready in a few days.

@pohlm01
Copy link
Member Author

pohlm01 commented Oct 30, 2024

Nice investigation! I don't have any strong opinion on how exactly to proceed.
I do agree with you that it's nice to not have a lot of boilerplate code, and we should reduce the chance of diverging code and documentation.
The main motivation for our own OpenAPI document is that we are not too happy with the amount of details provided in the official specification and would like to improve on that. It is quite possible that automated solutions will have a hard time inferring all those details, so I'm curious to see what utoipa and aide have to offer.
Just make sure please that you are not investing way too much time into both solutions, as we will obviously go with one (if at all).

@benjaminedwardwebb
Copy link
Contributor

I submitted a couple draft PRs

These drafts are just explorations of each crate, only implementing a couple operations to get a flavor of what it's like to use the libraries.

While it was fun to explore, I still don't see a clear winner among the three options mentioned above.

I think that maintaining the schema by hand remains a very reasonable choice.

The two crates are, in the end, very similar. They both have a macro layer that sits on top of traits, and the traits can be implemented if the macros are giving issues. Personally I prefer the experience of using aide/schemars because it honors serde and validator and because it requires less macro boilerplate -- lots of trait impls, but I think that boilerplate would be easier to maintain because it's vanilla rust code.

Interested in your thoughts.

Also

The main motivation for our own OpenAPI document is that we are not too happy with the amount of details provided in the official specification and would like to improve on that.

do you have a concrete example of what you weren't happy with in the official specification? Perhaps I can weigh in on whether that would affect which crate is a better choice.

@pohlm01
Copy link
Member Author

pohlm01 commented Nov 5, 2024

We checked your draft PRs. Thanks for that!
While both have quite some nice features, we were not really happy with the results in either case; see a list of problems we identified below.
In both cases, it requires changing the existing code quite a bit and making it less comprehensive.
Therefore, we think it's best to maintain a separate OpenAPI spec in parallel.
Likely, there won't be big changes to the API along the way, which makes manual maintenance possible.

One of the problems we identified in the original OpenAPI spec is that fields like id, createdDateTime, etc. are marked as optional.
We assume that is because they should not be sent for a POST/PUT request, but they are returned on a GET for example.
Another problem is that the TargetType is specified as a string instead of an enum.

Utopia

  • Double required: false and type string or null
    - name: resourceName
      in: query
      required: false
      schema:
        type:
          - string
          - 'null'
  • allOf is a bit confusing
  • Not sure if id, createdDateTime, etc. are marked as required in allOf construct.
  • TargetLabel is never specified

Aide

  • Does not honor the resourceName query parameter at all
  • Post vens completely misses the requestBody
  • TargetLabel enum is specified strangely

If you are interested to start a OpenAPI document, we are happy to see that. I can completely understand if you are not interested, though. If you are motivated to do something else instead, feel free to have a look at the issue board again or poke me if you are in doubt what to do.

@benjaminedwardwebb
Copy link
Contributor

Sounds good. I think I agree that manually maintaining the schema is the most reasonable choice in this case.

If you are interested to start a OpenAPI document, we are happy to see that. I can completely understand if you are not interested, though. If you are motivated to do something else instead, feel free to have a look at the issue board again or poke me if you are in doubt what to do.

I'll take a shot at putting together an OpenAPI document (I'd assume in yaml). It would be nice to see this issue through and it's a good way to get familiar with all the endpoints and data models. But, let me know if at any point the team feels it is best for me to step back and for someone else to address this (such as if it feels more trouble than its worth to communicate all the specific details desired in the final OpenAPI spec via PR comments).

@pohlm01
Copy link
Member Author

pohlm01 commented Nov 6, 2024

Sounds good to me. Please go ahead and don't hesitate to ask any questions.

@benjaminedwardwebb
Copy link
Contributor

benjaminedwardwebb commented Nov 6, 2024

Sounds good to me. Please go ahead and don't hesitate to ask any questions.

First question: any thoughts on how to verify that the spec conforms to the VTN implementation? My thought is to use something like progenitor (or any other generator for any other language, I suppose) to generate a client from the spec and write some integration tests (though perhaps the tests don't need to be committed to the repository, or maybe that can at least come later).

@KeeganMyers
Copy link
Contributor

Hi Ben,

thanks a lot for your initiative! Our intention was to export the OpenAPI specification during compilation as a build artifact. The third point you are mentioning, serving an openapi.json during runtime, seems helpful as well.

We are skeptical about serving the UI though, as we think this would introduce a lot of additional dependencies, and serving a web page feels wrong in an API. The additional value seems limited to us, as web pages like https://editor.swagger.io/ exist and most IDEs can handle the OpenAPI specification as well.

The link to utoipa was just a suggestion, and we have not investigated ourselves how to tackle this issue yet, so feel free to suggest other solutions as well, if you know anything better.

I don't know of anyone working on that already.

I'm happy to help with any further questions or suggestions, and looking forward to a PR from you!

Sorry I've been out of this repo. I'm doing a career accelerator trying to get into climate tech professionally. Anyway you can generate the swagger and save it as a json file with utopia. Tasks like this are part of the reason I was moving to a cli crate in the workspace. You often need build specific tasks that interact with the workspace. If you do this in clap the code looks like this

        Command::Swagger => {
            let filename = "swagger.json";
            let swagger = ApiDoc::openapi().to_pretty_json().unwrap();
            let mut file = File::create(filename)?;
            file.write_all(&swagger.bytes().collect::<Vec<u8>>())?;
        },

Yes there is an unwrap here because this is run as part of a CLI command so a sudden panic is fine in this context.
Anyway one thing to note about using either aide or utopia neither of them fully support serde. They both have some frustrating issues around things like flatten and some other attributes. If you end up moving in that direction you will need to work around the flatten limitation with meta-programming if you want to share common data structures (like pagination on request structs).

@pohlm01
Copy link
Member Author

pohlm01 commented Nov 7, 2024

@benjaminedwardwebb I'm not quite sure how to do that testing. Maybe not really worth it for now. I never had a great experience with code generators from OpenAPI specifications so far, and I hope the API is not changing too much anyway, so if we get it correct once, it should hopefully stay correct. But if you have the time and motivation to create some (basic) testing using things like progenitor, feel free to do that as well.

@KeeganMyers Yes, you are right, utopia would have been an option. Thanks for the analysis of @benjaminedwardwebb we decided against using it, though, and maintaining the OpenAPI document manually. This is mainly for the reasons that you give as well, like unsatisfying support for serde and non-optimal quality of the generated document.

@benjaminedwardwebb
Copy link
Contributor

Sorry I haven't been responsive on this issue recently. I only did a little exploratory work testing some endpoints to confirm the serialization they accepted, and then ended up becoming busy with other things in life.

You're of course welcome to remove me from this issue. I may have some time again later December to contribute and if I do I'll see how I can best help then.

@pohlm01
Copy link
Member Author

pohlm01 commented Nov 28, 2024

Thanks for letting us know. At the moment, this issue is not of the highest priority to us and we don't know anyone who would like to take over. Therefore, I suggest, we keep you assigned to this issue for now. You are always free to (explicitly) leave it, of course. We communicate in advance if someone else wants to take this issue.

@pohlm01 pohlm01 moved this from In Progress to Todo in OpenADR 3.0 Plan Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

4 participants