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

feat: removeDuplicateTags() validates tags and panic with meaningful … #2597

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

gitxiongpan
Copy link
Contributor

@gitxiongpan gitxiongpan commented Mar 28, 2023

if schema defines a bad goTag, example:

type AObject {
    id: ObjectID! @goTag(key: "bson", value: "_id, omitempty")
}

The value contains an empty space. This will break removeDuplicateTags function and panic with vague error message which makes it hard to debug.

My PR checks goTag format and panic with meaningful message which helps debugging a lot.

Before:
image
After:
image

Test result:
image

I have:

  • [✅] Added tests covering the bug / feature (see testing)
  • [❌ ] Updated any relevant documentation (see docs)

@coveralls
Copy link

Coverage Status

Coverage: 75.417% (+0.009%) from 75.408% when pulling 4bf2616 on gitxiongpan:master into 677d854 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

In general, I would prefer if we did not panic, but since we already did, this is a big improvement to at least give a good reason! Thanks very much!

@StevenACoffman StevenACoffman merged commit 6da735c into 99designs:master Mar 28, 2023
@emanuel-gjini
Copy link

emanuel-gjini commented Apr 28, 2023

@gitxiongpan It looks like this update actually broke one of my running servers due to the usage of some tags that are used to generate GORM model tags. And some of those actually have spaces, like this one for example (from the gql schema):

type Proxy @private {
  id: Uint! @goTag(key: "gorm", value: "primaryKey")
  ip: String! @goTag(key: "gorm", value: "unique;not null")
}

Notice the not null

Is there any chance we can allow spaces in tag values?

@gitxiongpan
Copy link
Contributor Author

@emanuel-gjini I am sorry for what happened to you. In fact, it was broken before my PR, by other contributor.
have a look of that removeDuplicateTags()
func removeDuplicateTags(t string) string { processed := make(map[string]bool) tt := strings.Split(t, " ")

I totally understand your frustration. I have just submitted a PR to fix this issue. #2628

@emanuel-gjini
Copy link

@gitxiongpan Many thanks for the fix ❤️

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 this pull request may close these issues.

4 participants