-
Notifications
You must be signed in to change notification settings - Fork 70
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
Improve Handling of Duplicate Fields in ResponseField Conversion and Ensure Backward Compatibility #254
Changes from 3 commits
9ef174c
6fa033c
33677e6
ad7dba4
b1cbb79
0ff4a3f
8c2a751
2c1de30
f43b1b6
3b09a08
4d5423b
31d7966
994046f
713c351
c71d688
c698679
291997c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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) { | ||
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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, done with this commit. |
||
} 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 { | ||
|
@@ -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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:]...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks add comment with this commit |
||
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, | ||
|
There was a problem hiding this comment.
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.