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

Tidying up the OpenAPI description #6

Closed
judgej opened this issue Dec 3, 2020 · 3 comments
Closed

Tidying up the OpenAPI description #6

judgej opened this issue Dec 3, 2020 · 3 comments

Comments

@judgej
Copy link

judgej commented Dec 3, 2020

I probably need to be corrected on soem of my assumptions, but from what I can tell, the KeyCloak project does not publish OpenAPI descriptions, so this project was set up to generate them by parsing the KeyCloak code. This is the only OpenAPI description for KeyCloak, so it's this or nothing. Like I say, I may be wrong on that, but that's where I'm coming from.

The generated OpenAPI description is probably the best that can be generated, given its source. It lacks operation IDs, many descriptions, and most data types and validation rules. It is understandable that those cannot always be parsed from the code, and what has been achieved is remarkable.

Now, I would like to generate code from the OpenAPI description, which means a lot of the missing bits need to be filled in to create consistent and clean code. There are over 1300 errors that show as OpenAPI violations. The code generators do what they can to work around that, but it's still not quite enough.

For example, endpoint /{realm}/client-scopes/{id}/protocol-mappers/protocol/{protocol} does not have an operationId defined, so the generator constructs one from the path, coming up with realmClientScopesId1ProtocolMappersModelsId2Get. That's a bit of a mouthful, so setting an operatinoId, which must be unique, would help set that function to what the operation is really doing - getRealmClientScopesProtocolMappersModels. It could probably be shortened more - the "Models" suffix is likely redundant.

Then the function takes three parameters: a realm and two IDs. All are strings, with no defined format, presumably mandatory, no validation rules, no descriptions. All that should be in the OpenAPI description, and all that results in generated code that applies those validation rules and prevents requests from being sent if the data is not at least in the right format.

Then the results all map onto success models if the server returns a "2XX" response code. That's it a "2 followed by two more digits" - it is literally a 2XX response code. That generates code that simply will not work, because that response will never be returned by a HTTP response. The code generation templates could possibly be modified to look specifically for a "2XX" in the API description and map it to some code that looks for "200" to "299", but that's not ideal.

So, sounds like a bunch of criticisms, and I hope it does not come over negatively. However, I'm just trying to set the scene, to describe how we generally use OpenAPI descriptions to generate code. They also feed into test tools such as Prism and generated documentation. The golden rule that works for us, is that the OpenAPI description is the source of truth - it generates code that we then do not touch. If the code needs to change, then we change the generation templates, grab a new OpenAPI desription, and generate again.

So where am I going with this. It's a question: how can we fix this? There are many thousands of fixes that could or should be done to the OpenAPI description, but is there a place to do that within this project? Most will be done manually, with a decision being made by someone choosing a name, or looking up a validation rule or data format. They probably could not be done by one person, since it will involve much knowledge, and it would take time. If the descriptino file is edited directly (using a tool like Stoplight Studio perhaps) then what happens when the source file needs to be generated again?

Or perhaps for my own projects I need to take the OpenAPI file, strip out or ignore everything I am not going to use, and manually fix up what's left? That feels like a missed opportunity, and a frozen API that will drift away from reality.

Sorry for the long read - I hope it's clear what I'm looking for: a way to fix up the files to make them complete, while being able to carry those fixes forward for all to share. Do you have any thoughts or suggestions on this?

@ccouzens
Copy link
Owner

ccouzens commented Dec 3, 2020

Hi Jason,

First of all, thank you for contacting me. It make me happy to see people finding my work useful (or in this case nearly useful).

I believe the Keycloak project is working on their own OpenAPI descriptions, but for now I think this is all there is.

I don't directly parse Keycloak's Java source code. Instead I parse their published documentation. I believe their published documentation is generated from their source code.

I am indeed quite limited by what I can do. The documentation has it's problems, and there's only so much I can do about them. And I can't get information that isn't there to begin with.

The html documentation does have html id attributes, which I initially hoped to use for operationIds. But it turns out they're not unique which makes them non-viable.

Path parameters are always defined as string in the html. If I could give them a better type I would.

It may be possible to improve the HTTP response codes. The documentation doesn't say what any of the routes return, but it's reasonable to expect GET requests to return 200. But other HTTP verbs are less predictable.

2XX has been working for me. I'd been using OpenAPI generator to generate Ruby and TypeScript-Axios clients. Given you're generating mock servers, it's not surprising the ambiguity is more of a problem for you. It is part of the standard- but how things actually work is often more important than how they should work.

A further problem is that Keycloak has APIs that are missing from the HTML documentation, and therefore aren't here either. For example all the endpoints that start http://${host}:${port}/auth/realms/${realm_name}/authz/.

I agree Keycloak's API is far too large to reasonably work on by hand in its entirety - that realisation was my main motivation for coding up a generator for it.

I don't think there's much room for improvement in the generator. It's only as good as its input (the HTML documentation). And its input is limited.

I'm assuming you only need a fraction of the API. I think if I were you, I'd cut out everything you don't need from here, and manually improve what's left. It is a shame that it would require manual work every time you want to upgrade to a new Keycloak version. But the API stays reasonably static between versions so it shouldn't be too much work †.

I'm afraid I'm not actively using Keycloak's API at the moment, so I don't have any motivation to manually improve it myslef. If you decide to go ahead and improve some of it yourself, I would be happy to either link people to your repository or to host in this repository. Either way, having it open and discoverable should help lessen the burden.

Good luck,
Chris


† Only 228 lines changed between versions 10, and 11. There are about 13,000 lines total- so it is more than 98% the same.

$ diff keycloak/10.0.json keycloak/11.0.json | wc -l
228
$ wc -l < keycloak/10.0.json 
13226

@judgej
Copy link
Author

judgej commented Dec 4, 2020

Thanks Chris. You are right, it is a small fraction that we really need - just the user management (create, modify, delete, query, and presumably some roles, password resetting) and I think you are right that we just pull out the parts we need. It looks like the future keycloak API is going to be a very different beast, built more from the ground up, with OpenAPI at its core, so it's not worth the time refining the current clients too much. I'll keep what I do in the open, and hopefully it will help others too, much as you have.

Your approach is very similar as was done for Xero - no OpenAPI spec for years, but documentation that was used to generate first code, then and API description. Xero then used that project as a starting point to building an official set of OpenAPI descriptions. And what a monster that has turned out to be, but it got things moving, gets more stable over time, and the number of tools and apps that now hang off it has exploded. The way things are these days, if there is not a easy to use API, then your product is going to flounder and be overtaken quickly.

Anyway, I think we are in agreement here, and I have a way forward. I'll close this issue now. Thank you.

@judgej judgej closed this as completed Dec 4, 2020
@ccouzens
Copy link
Owner

ccouzens commented Dec 4, 2020

Heads up

https://issues.redhat.com/browse/KEYCLOAK-9655

has been marked as "Coding in progress" about 10 minutes ago.

Depending on your timeline it may be worth waiting and seeing what comes of it.

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

No branches or pull requests

2 participants