Improve Handling of Duplicate Fields in ResponseField Conversion and Ensure Backward Compatibility #254

igaryo0506 · 2024-12-18T14:24:29Z

abstraction

Previously, when fragments and fields coexisted within a single field, the StructType function—used for generating Go structs from ResponseFields—attempted to handle them directly. This approach occasionally triggered a panic due to the presence of identical fields originating from both fragments and the field itself.

To resolve this, I have consolidated the fields during the conversion from AST objects to ResponseFields. This enhancement not only prevents the panic and ensures a more reliable result, but also maintains backward compatibility.

next to do

Looking ahead, we will also need to implement a GetFragment function.

related issue

#249

Yamashou · 2024-12-18T15:13:19Z

It would be great if you could add a simple example in the example directory to verify that the current changes are working correctly.

Yamashou

Thanks! Looking forward to your reply!

Yamashou · 2024-12-18T14:44:15Z

clientgenv2/source_generator.go

+	for _, field := range rs {
+		if field.IsFragmentSpread {
+			fragmentChildrenFields = append(fragmentChildrenFields, field.ResponseFields...)
+			removeTypeNames = append(removeTypeNames, field.FieldTypeString())


It might be better to separate the process of adding fields to FragmentChildrenFields and adding field.FieldTypeString() to removeTypeNames.

Thanks, done with this commit.
b1cbb79

Yamashou · 2024-12-18T14:54:40Z

clientgenv2/source_generator.go

+			for _, preMergedTypeName := range preMergedTypeNames {
+				for i, source := range r.StructSources {
+					if source.Name == preMergedTypeName {
+						r.StructSources = append(r.StructSources[:i], r.StructSources[i+1:]...)


This part of the process is not very intuitive, so I’d like to either add a comment or extract it into a function with a descriptive name.

Thanks add comment with this commit
0ff4a3f

Yamashou · 2024-12-18T15:04:25Z

clientgenv2/source_generator.go

@@ -130,6 +152,24 @@ func (r *SourceGenerator) NewResponseField(selection ast.Selection, typeName str
 			// if a child field is fragment, this field type became fragment.
 			baseType = fieldsResponseFields[0].Type
 		case fieldsResponseFields.IsStructType():
+			fieldsResponseFields, postMergedResponseFields, preMergedTypeNames := fieldsResponseFields.MergeFragmentFields()


The variable fieldsResponseFields being both the return value and having the same name makes it a bit confusing, so I’d like to use a clearer name.

Yamashou · 2024-12-18T15:10:34Z

clientgenv2/source_generator.go

+}
+
+// MergeFragmentFields returns merged ResponseFieldList, post-merged ResponseFieldList, and remove type names
+func (rs ResponseFieldList) MergeFragmentFields() (ResponseFieldList, map[string]*ResponseFieldList, []string) {


Does my understanding of what MergeFragmentFields is doing match the actual implementation?
1. Retrieving a list of fields for structs that are not IsFragmentSpread.
2. Generating a map for fragment types to prevent duplicates.
3. Creating a list of type string names to ignore duplicate fields.

The function name seems slightly disconnected from these operations, so I wanted to confirm.

igaryo0506 · 2024-12-19T13:53:44Z

Maybe It will be confusing for you, I used StructGenerator and simplified the logic.
And I make it possible to handle same field name recursively.

Yamashou · 2024-12-19T14:03:50Z

clientgenv2/source_generator.go

+			for _, preMergedTypeName := range generator.GetPreMergedStructSources() {
+				for i, source := range r.StructSources {
+					// when name is same, remove it
+					if source.Name == preMergedTypeName.Name {
+						r.StructSources = append(r.StructSources[:i], r.StructSources[i+1:]...)
+						break
+					}
+				}
+			}


Pretty good Fix! In my opinion, the best implementation would be something like this:

Suggested change

for _, preMergedTypeName := range generator.GetPreMergedStructSources() {

for i, source := range r.StructSources {

// when name is same, remove it

if source.Name == preMergedTypeName.Name {

r.StructSources = append(r.StructSources[:i], r.StructSources[i+1:]...)

break

}

}

}

　　　　　　　r.StructSources = generator.MargedStructSources(r.StructSources)

This is pretty nice advice. I will try!

igaryo0506 · 2024-12-20T05:47:36Z

I did

using MergeStructSource
fix bug of merging
changed to better query

Yamashou · 2024-12-21T15:55:24Z

clientgenv2/source_generator.go

+		}
+		// if there is no same name field, append it
+		if !sameNameFieldFlag {
+			targetFields = append(targetFields, sourceField)


It’s not ideal to cause side effects on arguments, so it would be better to define a type for the return value.

okey, I fixed it

Yamashou · 2024-12-21T16:02:06Z

clientgenv2/source_generator.go

+	for _, sourceField := range sourceFields {
+		sameNameFieldFlag := false
+		for _, targetField := range targetFields {
+			if sourceField.Name == targetField.Name && !targetField.ResponseFields.IsBasicType() {


With the current implementation, it seems like there are unnecessary loops, and the nesting feels a bit excessive. I’d like to simplify this by using the following implementation with a map:

func (rs ResponseFieldList) MapByName() map[string]*ResponseField { m := make(map[string]*ResponseField) for _, v := range rs { m[v.Name] = v } return m }

This approach should make the logic cleaner and more efficient.

It's true, I fixed this.

Yamashou · 2024-12-21T16:04:32Z

clientgenv2/source_generator.go

@@ -76,6 +69,97 @@ func (rs ResponseFieldList) IsStructType() bool {
 	return len(rs) > 0 && !rs.IsFragment()
 }

+type StructGenerator struct {
+	currentResponseFieldList *ResponseFieldList


In my opinion

Suggested change

currentResponseFieldList *ResponseFieldList

currentResponseFieldList ResponseFieldList

Yamashou · 2024-12-21T16:13:49Z

clientgenv2/source_generator.go

+			// 子フィールドにFragmentがある場合は、現在のフィールドとマージする
+			// if there is a fragment in child fields, merge it with the current field
+			generator := NewStructGenerator(fieldsResponseFields)
+
+			// restruct struct sources
+			r.StructSources = generator.MergedStructSources(r.StructSources)
+
+			// append current struct
+			structType := generator.GetCurrentResponseFieldList().StructType()


igaryo0506 · 2024-12-22T14:55:51Z

clientgenv2/source_generator.go

+	for _, field := range targetFieldsMap {
+		res = append(res, field)
+	}
+	res = res.SortByName()


I added a sorting step. Because the code uses a map, the field order in client.go could change randomly. This update ensures that the field order remains consistent.

Yamashou · 2024-12-23T06:17:25Z

clientgenv2/source_generator.go

+	}
+}
+
+func mergeFieldsRecursively(targetFields ResponseFieldList, sourceFields ResponseFieldList, preMerged, postMerged *[]*StructSource) ResponseFieldList {


It’s generally not a good practice to implicitly cause side effects by taking a pointer as an argument and modifying its value. I think it would be better to return the value explicitly as a return value.

Suggested change

func mergeFieldsRecursively(targetFields ResponseFieldList, sourceFields ResponseFieldList, preMerged, postMerged *[]*StructSource) ResponseFieldList {

func mergeFieldsRecursively(targetFields ResponseFieldList, sourceFields ResponseFieldList, preMerged []*StructSource) (ResponseFieldList, []*StructSource, []*StructSource) {

Okey, I will follow you.

Yamashou · 2024-12-23T06:18:50Z

clientgenv2/source_generator.go

+	currentResponseFieldList ResponseFieldList
+	preMergedStructSources   []*StructSource
+	postMergedStructSources  []*StructSource


If possible, it would be helpful to include comments explaining each field of the struct.

okey, I did.

Yamashou · 2024-12-23T06:30:53Z

clientgenv2/source_generator.go

+func (g *StructGenerator) MergedStructSources(sources []*StructSource) []*StructSource {
+	res := sources
+	// remove pre-merged struct
+	for _, preMergedTypeName := range g.preMergedStructSources {
+		for i, source := range res {
+			// when name is same, remove it
+			if source.Name == preMergedTypeName.Name {
+				res = append(res[:i], res[i+1:]...)
+				break
+			}
+		}
+	}
+
+	// append post-merged struct
+	res = append(res, g.postMergedStructSources...)
+
+	return res
+}


In my opinion. I prefer this approach of creating a map first because it results in shallower nesting during lookups, making the code cleaner and easier to follow.

Suggested change

func (g *StructGenerator) MergedStructSources(sources []*StructSource) []*StructSource {

res := sources

// remove pre-merged struct

for _, preMergedTypeName := range g.preMergedStructSources {

for i, source := range res {

// when name is same, remove it

if source.Name == preMergedTypeName.Name {

res = append(res[:i], res[i+1:]...)

break

}

}

}

// append post-merged struct

res = append(res, g.postMergedStructSources...)

return res

}

func structSourcesMapByTypeName(sources []*StructSource) map[string]*StructSource {

structSourceMapByTypeName := make(map[string]*StructSource)

for _, source := range sources {

structSourceMapTypeName[source.Name] = source

}

return structSourceMapByTypeName

}

func (g *StructGenerator) MergedStructSources(sources []*StructSource) []*StructSource {

preMargedStructSourceMapByTypeName := structSourcesMapByTypeName(g.preMergedStructSources)

res := sources

// remove pre-merged struct

for i, source := range res {

// when name is same, remove it

if _, ok := preMargedStructSourceMapByTypeName[source.Name]; ok {

res = append(res[:i], res[i+1:]...)

break

}

}

// append post-merged struct

res = append(res, g.postMergedStructSources...)

return res

}

It's true. I use map to avoid double loop.

Yamashou · 2024-12-23T08:04:37Z

clientgenv2/source_generator.go

+	preMergedStructSources := make([]*StructSource, 0)
+	postMergedStructSources := make([]*StructSource, 0)
+
+	for _, field := range responseFieldList {
+		if field.IsFragmentSpread {
+			preMergedStructSources = append(preMergedStructSources, &StructSource{
+				Name: field.FieldTypeString(),
+				Type: field.ResponseFields.StructType(),
+			})
+		}
+	}


Suggested change

preMergedStructSources := make([]*StructSource, 0)

postMergedStructSources := make([]*StructSource, 0)

for _, field := range responseFieldList {

if field.IsFragmentSpread {

preMergedStructSources = append(preMergedStructSources, &StructSource{

Name: field.FieldTypeString(),

Type: field.ResponseFields.StructType(),

})

}

}

preMergedStructSources := make([]*StructSource, 0)

for _, field := range responseFieldList {

if field.IsFragmentSpread {

preMergedStructSources = append(preMergedStructSources, &StructSource{

Name: field.FieldTypeString(),

Type: field.ResponseFields.StructType(),

})

}

}

Yamashou · 2024-12-23T08:05:12Z

clientgenv2/source_generator.go

+		}
+	}
+
+	currentFields, preMergedStructSources, postMergedStructSources = mergeFieldsRecursively(currentFields, fragmentChildrenFields, preMergedStructSources, postMergedStructSources)


Suggested change

currentFields, preMergedStructSources, postMergedStructSources = mergeFieldsRecursively(currentFields, fragmentChildrenFields, preMergedStructSources, postMergedStructSources)

currentFields, preMergedStructSources, postMergedStructSources = mergeFieldsRecursively(currentFields, fragmentChildrenFields, preMergedStructSources, nil)

Yamashou · 2024-12-23T12:18:18Z

@igaryo0506 LGTM!

igaryo0506 and others added 3 commits December 4, 2024 15:02

fix typo

9ef174c

fix source_generator.go

6fa033c

Merge branch 'Yamashou:master' into fix-fragment-panic

33677e6

Yamashou reviewed Dec 18, 2024

View reviewed changes

igaryo0506 added 5 commits December 19, 2024 12:44

add example

ad7dba4

divide two for logics

b1cbb79

add comment

0ff4a3f

use struct generator

8c2a751

add comment

2c1de30

igaryo0506 force-pushed the fix-fragment-panic branch from e0e8b04 to f43b1b6 Compare December 19, 2024 13:52

add recursively merge

f43b1b6

igaryo0506 requested a review from Yamashou December 19, 2024 13:55

Yamashou reviewed Dec 19, 2024

View reviewed changes

igaryo0506 added 2 commits December 20, 2024 11:43

use MergedStructSource

3b09a08

fix bug

4d5423b

Yamashou reviewed Dec 21, 2024

View reviewed changes

igaryo0506 added 2 commits December 22, 2024 23:25

fix query and fix bug

31d7966

use map and sort

994046f

igaryo0506 commented Dec 22, 2024

View reviewed changes

Yamashou reviewed Dec 23, 2024

View reviewed changes

igaryo0506 added 2 commits December 23, 2024 15:45

delete side effect

713c351

add comment

c71d688

use map to avoid double loop

c698679

Yamashou reviewed Dec 23, 2024

View reviewed changes

fix nits

291997c

igaryo0506 requested a review from Yamashou December 23, 2024 10:05

Yamashou approved these changes Dec 23, 2024

View reviewed changes

Yamashou merged commit 8fece25 into Yamashou:master Dec 23, 2024
1 check passed

igaryo0506 mentioned this pull request Jan 8, 2025

Fix generator for multiple fragment pattern #262

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Handling of Duplicate Fields in ResponseField Conversion and Ensure Backward Compatibility #254

Improve Handling of Duplicate Fields in ResponseField Conversion and Ensure Backward Compatibility #254

igaryo0506 commented Dec 18, 2024

Yamashou commented Dec 18, 2024

Yamashou left a comment

Yamashou Dec 18, 2024

igaryo0506 Dec 19, 2024 •

edited

Loading

Yamashou Dec 18, 2024

igaryo0506 Dec 19, 2024

Yamashou Dec 18, 2024

Yamashou Dec 18, 2024

igaryo0506 commented Dec 19, 2024

Yamashou Dec 19, 2024

igaryo0506 Dec 20, 2024

igaryo0506 commented Dec 20, 2024

Yamashou Dec 21, 2024

igaryo0506 Dec 22, 2024

Yamashou Dec 21, 2024 •

edited

Loading

igaryo0506 Dec 22, 2024

Yamashou Dec 21, 2024

Yamashou Dec 21, 2024

igaryo0506 Dec 22, 2024

Yamashou Dec 23, 2024

igaryo0506 Dec 23, 2024

Yamashou Dec 23, 2024

igaryo0506 Dec 23, 2024

Yamashou Dec 23, 2024

igaryo0506 Dec 23, 2024

Yamashou Dec 23, 2024 •

edited

Loading

Yamashou Dec 23, 2024

Yamashou commented Dec 23, 2024

	currentResponseFieldList *ResponseFieldList
	currentResponseFieldList ResponseFieldList

	func mergeFieldsRecursively(targetFields ResponseFieldList, sourceFields ResponseFieldList, preMerged, postMerged []StructSource) ResponseFieldList {
	func mergeFieldsRecursively(targetFields ResponseFieldList, sourceFields ResponseFieldList, preMerged []StructSource) (ResponseFieldList, []StructSource, []*StructSource) {

	currentFields, preMergedStructSources, postMergedStructSources = mergeFieldsRecursively(currentFields, fragmentChildrenFields, preMergedStructSources, postMergedStructSources)
	currentFields, preMergedStructSources, postMergedStructSources = mergeFieldsRecursively(currentFields, fragmentChildrenFields, preMergedStructSources, nil)

Improve Handling of Duplicate Fields in ResponseField Conversion and Ensure Backward Compatibility #254

Improve Handling of Duplicate Fields in ResponseField Conversion and Ensure Backward Compatibility #254

Conversation

igaryo0506 commented Dec 18, 2024

abstraction

next to do

related issue

Yamashou commented Dec 18, 2024

Yamashou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igaryo0506 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igaryo0506 commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igaryo0506 commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yamashou Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yamashou Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yamashou commented Dec 23, 2024

igaryo0506 Dec 19, 2024 •

edited

Loading

Yamashou Dec 21, 2024 •

edited

Loading

Yamashou Dec 23, 2024 •

edited

Loading