search

pulsar-edit / package-backend

Pulsar Server Backend for Packages
https://api.pulsar-edit.dev
MIT License
11 stars 11 forks source link

Automatically Update OpenAPI Documentation #235

Closed confused-Techie closed 4 months ago

confused-Techie commented 5 months ago

Requirements

Description of the Change

This PR finally takes advantage of all the work done when modularizing the endpoints and query parameters.

Now we have a system of code objects that define how the code works, define how the tests work, and define the documentation that's generated in a complex way.

This script uses all information available to generate updates to the OpenAPI yaml document on each commit, so that we can ensure our schema never becomes out of date

Superseedes #22

Spiker985 commented 5 months ago

Super excited to see this

Spiker985 commented 5 months ago

One thing that I noticed on the generated YAML, while not a requirement, is that the endpoints appear to not be alphabetized. If they were https://github.com/pulsar-edit/package-backend/blob/231de711db097dddda06d76095ec4976d5072e52/docs/swagger/openapi3_def.yaml#L71 should be where https://github.com/pulsar-edit/package-backend/blob/231de711db097dddda06d76095ec4976d5072e52/docs/swagger/openapi3_def.yaml#L17 currently is

But also there are quite a few endpoints that appear to not have any "response" information such as GET /api/login, GET /api/oauth, GET /api/pat POST /api/packages, GET /api/packages/{packageName}/versions/{versionName}

The Tarball endpoint response is a little screwy too, though I assume that's an indexing problem: https://github.com/pulsar-edit/package-backend/blob/231de711db097dddda06d76095ec4976d5072e52/docs/swagger/openapi3_def.yaml#L229-L231 similar problem here as well https://github.com/pulsar-edit/package-backend/blob/231de711db097dddda06d76095ec4976d5072e52/docs/swagger/openapi3_def.yaml#L279-L282

confused-Techie commented 5 months ago

@Spiker985 Thanks a ton for pointing this out!

Not sure what's gone wrong with the tarball endpoint so I'll take a look. But I think the other issues is due to those endpoints not being fully written out, as in the object lacks details of it's response data. So I'll make sure to add that so we can start testing for it as well.

Otherwise you're right that alphabetizing this would be really cool, I almost wonder, I'm pretty sure the yaml.dumb() method supports a custom sort function, maybe adding it there would be enough to do that.

But thanks for reviewing the document, since wanna make sure it's valid and keeps up the standard of what your originally wrote

confused-Techie commented 5 months ago

@Spiker985 Would love to hear any feedback, I think I've addressed everything. From here there's just the case of a few missing responses and parameters

EDIT: I'm now aware of the issues with owner and direction query parameters. I'm on the fence about how to solve that, since those query parameters have the same limitations, but different schemas. Meaning in some cases it may come from a different location or may be under a different name, but the code handles them identically. So not sure what the best solution is just yet

Daeraxa commented 5 months ago

Sorry, should have done it as a single review, thought it was just one or two then I went a bit mad... Some are typos, many are just adding a full stop to the end of the sentence to match the style of the others.

confused-Techie commented 5 months ago

@Daeraxa You're feedback has been addressed, thanks a ton for the review and taking a look at this

confused-Techie commented 5 months ago

@Spiker985 @Daeraxa Alright, from here I've addressed the issues that had still existed (as well as discovering another) and I think we are 100% ready to merge if anyone wants to take a last look.

As for what I've addressed in the last couple PRs:

Spiker985 commented 5 months ago

Nitpicky, but I've never seen any OpenAPI schemas which list the components first

Does this have any effect on how the swagger page displays them?

confused-Techie commented 5 months ago

Nitpicky, but I've never seen any OpenAPI schemas which list the components first

Does this have any effect on how the swagger page displays them?

Valid question, threw me off at first too.

But doesn't seem to have any negative effect:

image

But this likely happens because we now sort the results to list the endpoints in a more satisfying way. If we really wanted to I could try my hand at a custom sorter, but as it doesn't seem to affect anything I don't think we have anything to worry about.

Spiker985 commented 5 months ago

I'll likely reread this PR later today just to double check and make sure I haven't missed anything with the other changes

But if it gets a green light before then, feel free to merge

Arguably, the "current" version is no more accurate than anything that would be missed in this PR anyway

confused-Techie commented 5 months ago

Sounds like a fine plan to me @Spiker985

Daeraxa commented 5 months ago

Nitpicky, but I've never seen any OpenAPI schemas which list the components first

@Spiker985 Do you have an example of what you normally expect? It looks the same as the existing UI from what I can see or am I missing the issue here?

Spiker985 commented 5 months ago

Nitpicky, but I've never seen any OpenAPI schemas which list the components first

@Spiker985 Do you have an example of what you normally expect? It looks the same as the existing UI from what I can see or am I missing the issue here?

Nope, that was exactly what I wanted to be sure of.

I was unsure of how the OpenAPI UI implementation was affected by the schema

From confused's response, it appears that the schema ordering is irrelevant and the UI will always display in a "static" order of API information -> endpoints -> schema

Basically, as long as the data exists in your yaml file, regardless of order, it'll always be implemented the same in the UI

That was all I wanted to validate with that question

(This doesn't count as reading through the updates, because I haven't done that yet)

confused-Techie commented 5 months ago

Alright this PR now has OPTIONS methods added, as well as everything else should be fully fleshed out

confused-Techie commented 4 months ago

Alright, since it's been a week since the last comment here, and this PR touches far too much of the codebase to avoid future conflicts, I'm going to go ahead and merge.

Thanks a ton to those that helped get this one along!