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

Respect initialisms for schema generation #454

Closed
jaredpetersen opened this issue Oct 6, 2021 · 4 comments · Fixed by #1007
Closed

Respect initialisms for schema generation #454

jaredpetersen opened this issue Oct 6, 2021 · 4 comments · Fixed by #1007

Comments

@jaredpetersen
Copy link

jaredpetersen commented Oct 6, 2021

Per the Go code review guide, initialisms like ID, DB, etc. should have consistent casing like id or ID. However, the schema generation functionality is just converting the properties to UpperCamelCase and disregarding this rule, which produces structs with fields like Id and Db.

The culprit seems to be that schema.go calls ToCamelCase, a homegrown case converter. Packages like iancoleman/strcase support custom acronyms so that users can specify these initialism overrides. We don't necessarily need to import this new package but it's a good illustration of how this might be implemented.

One option is to maintain the current behavior but add a new configuration flag -initialisms with values like ID,HTTP,DB that would override those parts of the property name.

Another option is to create a new property extension that would allow users to override the struct field name in the Open API definition, e.g. x-oapi-codegen-name. Maybe something similar for overriding the json tag as well if necessary. The risk with this approach is that we will alienate people that are using an OpenAPI doc that they do not own and therefore cannot modify to add these extended properties.

Some test cases:

  • dbName
    • Expected: DBName string `json:"dbName"`
    • Actual: DbName string `json:"dbName"`
  • id
    • Expected: ID string `json:"id"`
    • Actual: Id string `json:"id"`
  • someHttpThing
    • Expected: SomeHTTPThing string `json:"someHttpThing"` (json tag is not changed)
    • Actual: SomeHttpThing string `json:"someHttpThing"`
  • some_http_thing
    • Expected: SomeHTTPThing string `json:"some_http_thing"`
    • Actual: SomeHttpThing string `json:"some_http_thing"`
@jaredpetersen
Copy link
Author

Since golint is deprecated, we can also rely on a list of initialisms provided by staticcheck.

https://staticcheck.io/docs/options#initialisms

["ACL", "API", "ASCII", "CPU", "CSS", "DNS", "EOF", "GUID", "HTML", "HTTP", "HTTPS", "ID", "IP", "JSON", "QPS", "RAM", "RPC", "SLA", "SMTP", "SQL", "SSH", "TCP", "TLS", "TTL", "UDP", "UI", "GID", "UID", "UUID", "URI", "URL", "UTF8", "VM", "XML", "XMPP", "XSRF", "XSS", "SIP", "RTP", "AMQP", "DB", "TS"]

We'd just have to be careful about breaking backwards compatibility. So maybe that means a cli flag -initialisms to start that is just a boolean flag. And if it turns out that the community wants to add custom ones later, we can cross that bridge.

@paulmach
Copy link
Contributor

my sed/regex skill is novice at best but I converted Id -> ID with this command that is run after generating the code

sed -E -i.bak 's/([^_])Id([^a-zA-Z])/\1ID\2/g' openapi.gen.go

The only problem is it converts inside strings too. So if your path parameters are snake_case it's okay, but camelCase is not :(

I'm hoping there is a better way, if the linter can warn on it, they can autofix too right?

@jamietanna
Copy link
Member

As raised on Gopher Slack, it appears that this doesn't get wired in for generated types.

We should pass around the flag's value and make sure that i.e. SchemaNameToTypeName gets the flag, and responds accordingly.

While doing this, I also noticed that we get the following diff:

        handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-               siw.Handler.FindPetByID(w, r, id)
+               siw.Handler.FindPetByID(w, r, iD)
        }))

Which doesn't seem quite right.

@jamietanna
Copy link
Member

I'd say this will be covered better with #1041 which provides a configurable means to specify the way that casing + initialisms get applied

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

Successfully merging a pull request may close this issue.

3 participants