RemoveDuplicateTags func introduced to fix #2514

[valllabh]

As described in #2514, and without this PR, GoTagFieldHook just appends new tags, so @goTag will not override an existing tag.

Ideally it should override existing tag, since the sole purpose of the @goTag directive is to give control to user.

In this PR, the removeDuplicateTags helper func is introduced to fix #2514

I have:

• [✓] Added tests covering the bug / feature (see testing)
• [Not Required] Updated any relevant documentation (see docs)

[StevenACoffman]

Thanks for working on this!

The first run through only some of the matrix of tests failed (e.g. ubuntu, Go 1.18):

--- FAIL: TestModelGeneration (18.60s)
    --- FAIL: TestModelGeneration/field_hooks_are_applied (0.00s)
        models_test.go:92: 
            	Error Trace:	models_test.go:92
            	Error:      	Should be true
            	Test:       	TestModelGeneration/field_hooks_are_applied
FAIL
FAIL	github.com/99designs/gqlgen/plugin/modelgen	37.009s

I wonder if this means that either in that particular test, or perhaps in your code, it somehow related to ranging through a map in an order that is not deterministic. I'm on my phone right now, so I can't look at it in more detail, but just food for thought.

[StevenACoffman]

Also, your code appears to prefer the last value of duplicate tags, rather than the first. Is there a reason for this?

Update: Ah, I see. In GoTagFieldHook we apply the goTag directive by adding it as a suffix to existing tags, so you want to preserve the duplicate value that occurs last:

f.Tag = f.Tag + " " + strings.Join(args, " ")

Your original code preserved the first appearing tag key order, but gave the reverse precedence for duplicate tag values (first occurring duplicate would have last occurring value).

I felt like this was confusing. If someone was using a goTag directive to override a value, it would not be surprising to have an overridden key value moved to the end, and I thought it somewhat simplified the code. I adjusted your unit test to match this. WDYT?

https://go.dev/play/p/GxXnDGI9c2g

Another alternative would be to change GoTagFieldHook :

f.Tag = f.Tag + " " + strings.Join(args, " ")

to prepend instead of append:

f.Tag = strings.Join(args, " ") + " " + f.Tag 

This would allow the code to be further simplified (since we don't need to walk backwards through the struct tag key:value pairs.

[StevenACoffman]

Suggested change

	// removeDuplicateTags removes duplicate tags
	func removeDuplicateTags(t string) string {
	tt := strings.Split(t, " ")
	if len(tt) > 0 {
	tagMap := map[string]string{}
	returnTags := ""
	for _, ti := range tt {
	kv := strings.Split(ti, ":")
	if len(kv) > 0 {
	tagMap[kv[0]] = kv[1]
	}
	}
	for k, v := range tagMap {
	if len(returnTags) > 0 {
	returnTags += " "
	}
	returnTags += k + ":" + v
	}
	return returnTags
	}
	return t
	}
	// removeDuplicateTags removes duplicate tags
	func removeDuplicateTags(t string) string {
	processed := make(map[string]bool)
	tt := strings.Split(t, " ")
	returnTags := ""
	// iterate backwards through tags so appended goTag directives are prioritized
	for i := len(tt) - 1; i >= 0; i-- {
	ti := tt[i]
	kv := strings.Split(ti, ":")
	if len(kv) == 0 || processed[kv[0]] {
	continue
	}
	processed[kv[0]] = true
	if len(returnTags) > 0 {
	returnTags = " " + returnTags
	}
	returnTags = kv[0] + ":" + kv[1] + returnTags
	}
	return returnTags
	}

[StevenACoffman]

Suggested change

	want: "json:\"name3\" something:\"name2\"",
	want: "something:\"name2\" json:\"name3\"",

[StevenACoffman]

Hey, I hope it's ok, but I merged the tweaks I made in #2533 as they seemed a little easier to read. I would be happy to accept a PR if you feel like yours is better.

Thanks!

[valllabh]

Hey @StevenACoffman Thank you for taking a look at it.

Gqlgen can be used as just a model generator and in that case we may not have a full control over backend.
In such conditions json:fieldName can not be same as GoLang field name conventions.
Thats why my code was preferring to last value. It allows to override the values easily.
Other work around to override is to write own plugin and replace it during generation call, which is lengthy.

[StevenACoffman]

That makes sense! Hit me up with a PR if you feel like it should change to the behavior you originally proposed.

[valllabh]

Sure thing. Let me report issue and fork again