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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to preserve RPC paths order in swagger output #3051

Closed
eccles opened this issue Dec 7, 2022 · 8 comments 路 Fixed by #3500
Closed

Add option to preserve RPC paths order in swagger output #3051

eccles opened this issue Dec 7, 2022 · 8 comments 路 Fixed by #3500

Comments

@eccles
Copy link

eccles commented Dec 7, 2022

馃殌 Feature

The generated swagger files from the proto files always emits the "paths" list in alphabetical order apparently.
We need these in the same order as in the proto file.

An option to the openapiv2 protoc plugin such as "paths_in_emit_order=true" with a default value of false would be exceedingly useful.

@johanbrandhorst
Copy link
Collaborator

Hi, thanks for your issue! Funnily enough someone else asked for this exact feature on the Slack channel just a few days ago. I usually say that we wait until there are two users who want the same thing until implementing it, so I think we should probably add this. If I may bikeshed for a moment, I'd prefer something like preserve_rpc_order=true, what do you think? I'd also add the following requirements to this feature:

  1. When using additional bindings, paths should preserve this ordering within an RPC
  2. When using multiple services, paths should preserve the ordering between RPCs in the whole protobuf file
  3. When merging protobuf files, paths should preserve the ordering specified on the command line.

Given those requirements, I think this should be ready for someone to look at implementing. Would you be interested in contributing this?

@johanbrandhorst johanbrandhorst changed the title Generated openapi emit paths in specified order Add option to preserve RPC paths order in swagger output Dec 8, 2022
@eccles
Copy link
Author

eccles commented Dec 12, 2022

I'm not bothered about what to call it - preserve-rpc-order=true seems entirely acceptable.
I might have time in the near future - I will let you know...

@CemGurhan
Copy link
Contributor

I wouldn't mind giving this a go, if no one has picked it up yet?

@johanbrandhorst
Copy link
Collaborator

Go for it :)

@CemGurhan
Copy link
Contributor

Will do! Also, I've noticed that the the paths are keys to the openapiPathsObject map, so I think encoding/json is ordering them alphabetically by default once the openapiSwaggerObject is encoded (this is expected behaviour I believe). I've seen some workarounds to this on StackOverflow and here: golang/go#27179.

@CemGurhan
Copy link
Contributor

CemGurhan commented Aug 14, 2023

@johanbrandhorst I believe I have a PR ready for this, but I've avoided altering the type of the openapiPathsObject from a map to an ordered data structure as it may cause a lot of other changes to be made in the existing codebase.

That being said, if I can alter this object's type to something else it'll help reduce the complexity of this feature, so I was wondering if this is okay to do in this PR? I'd have to change a lot of the tests in template_test.go as a consequence.

This is the current defintion for the openapiPathsObject:

type openapiPathsObject map[string]openapiPathItemObject

And to preserve ordering upon encoding, I was thinking something like this could be better:

type openapiPathsObject []pathData

type pathData struct {
        Path           string
        PathItemObject openapiPathItemObject
}

@johanbrandhorst
Copy link
Collaborator

It sounds fine to me to change it to an ordered data structure, thank you for asking :).

@CemGurhan
Copy link
Contributor

I've got a pull request for this issue opened now: #3500

Let me know of any flaws in my logic/code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants