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

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

Merged
merged 17 commits into from
Dec 23, 2024
68 changes: 54 additions & 14 deletions clientgenv2/source_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ type ResponseField struct {
ResponseFields ResponseFieldList
}

func (r ResponseField) FieldTypeString() string {
fullFieldType := r.Type.String()
parts := strings.Split(fullFieldType, ".")
return parts[len(parts)-1]
}

type ResponseFieldList []*ResponseField

func (rs ResponseFieldList) IsFragmentSpread() bool {
Expand All @@ -40,24 +46,40 @@ func (rs ResponseFieldList) IsFragmentSpread() bool {
func (rs ResponseFieldList) StructType() *types.Struct {
vars := make([]*types.Var, 0)
structTags := make([]string, 0)
for _, filed := range rs {
// クエリーのフィールドの子階層がFragmentの場合、このフィールドにそのFragmentの型を追加する
if filed.IsFragmentSpread {
typ, ok := filed.ResponseFields.StructType().Underlying().(*types.Struct)
if !ok {
continue
}
for j := range typ.NumFields() {
vars = append(vars, typ.Field(j))
structTags = append(structTags, typ.Tag(j))
}
for _, field := range rs {
vars = append(vars, types.NewVar(0, nil, templates.ToGo(field.Name), field.Type))
structTags = append(structTags, strings.Join(field.Tags, " "))
}
return types.NewStruct(vars, structTags)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

res := make(ResponseFieldList, 0)
fragmentChildrenFields := make(ResponseFieldList, 0)
removeTypeNames := make([]string, 0)
for _, field := range rs {
if field.IsFragmentSpread {
fragmentChildrenFields = append(fragmentChildrenFields, field.ResponseFields...)
removeTypeNames = append(removeTypeNames, field.FieldTypeString())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@igaryo0506 igaryo0506 Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done with this commit.
b1cbb79

} else {
vars = append(vars, types.NewVar(0, nil, templates.ToGo(filed.Name), filed.Type))
structTags = append(structTags, strings.Join(filed.Tags, " "))
res = append(res, field)
}
}

return types.NewStruct(vars, structTags)
postMergedResponseFields := make(map[string]*ResponseFieldList)
for _, field := range fragmentChildrenFields {
for _, resField := range res {
if field.Name == resField.Name {
resField.ResponseFields = append(resField.ResponseFields, field.ResponseFields...)
postMergedResponseFields[resField.FieldTypeString()] = &resField.ResponseFields
removeTypeNames = append(removeTypeNames, field.FieldTypeString())
removeTypeNames = append(removeTypeNames, resField.FieldTypeString())
break
}
}
}
return res, postMergedResponseFields, removeTypeNames
}

func (rs ResponseFieldList) IsFragment() bool {
Expand Down Expand Up @@ -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()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// remove pre-merged struct
for _, preMergedTypeName := range preMergedTypeNames {
for i, source := range r.StructSources {
if source.Name == preMergedTypeName {
r.StructSources = append(r.StructSources[:i], r.StructSources[i+1:]...)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks add comment with this commit
0ff4a3f

break
}
}
}
// append post-merged struct
for postMergedName, responseFieldList := range postMergedResponseFields {
postMergedStructType := responseFieldList.StructType()
r.StructSources = append(r.StructSources, &StructSource{
Name: postMergedName,
Type: postMergedStructType,
})
}
structType := fieldsResponseFields.StructType()
r.StructSources = append(r.StructSources, &StructSource{
Name: typeName,
Expand Down
Loading