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

Conflicting package + struct name #629

Merged
merged 5 commits into from Feb 23, 2020
Merged

Conflicting package + struct name #629

merged 5 commits into from Feb 23, 2020

Conversation

ghost
Copy link

@ghost ghost commented Feb 19, 2020

Describe the PR
If code uses two packages that contains types with same name, swag generate wrong json/yaml. This PR fixes this.

Relation issue
Fixes #225

Additional context
This changes need testing on bigger swag files.

@ghost ghost requested a review from easonlin404 February 19, 2020 17:05
@ghost ghost mentioned this pull request Feb 19, 2020
@@ -25,7 +25,7 @@ func TestGetPropertyNameSelectorExpr(t *testing.T) {
"string",
"",
}
propertyName, err := getPropertyName(input, New())
propertyName, err := getPropertyName("test", input, New())
Copy link
Member

Choose a reason for hiding this comment

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

I guess you're testing it so hardcode "test" string, right? 😄

Copy link
Author

Choose a reason for hiding this comment

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

What do you suggest?

Copy link
Member

@easonlin404 easonlin404 left a comment

Choose a reason for hiding this comment

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

I got a little bit confused why are these changes fixing the issue?

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I got a little bit confused why are these changes fixing the issue?

In short: they don`t, in bigger projects.
I will test this in different conditions, and add some code to this PR.

property.go Show resolved Hide resolved
@sdghchj
Copy link
Member

sdghchj commented Feb 22, 2020

Some optimizations. Take a look:

func getPropertyName(pkgName string,expr ast.Expr, parser *Parser) (propertyName, error) {
	switch tp := expr.(type) {
        case *ast.SelectorExpr:
		return parseFieldSelectorExpr(tp, parser, newProperty), nil
	case *ast.StarExpr:
		return getPropertyName(pkgName,tp.X, parser)
	case *ast.ArrayType:
		return getArrayPropertyName(pkgName,tp.Elt, parser), nil
	case *ast.MapType,*ast.StructType,*ast.InterfaceType:
		return propertyName{SchemaType: "object", ArrayType: "object"}, nil
	case *ast.Ident:
		name := tp.Name
		// check if it is a custom type
		if actualPrimitiveType, isCustomType := parser.CustomPrimitiveTypes[pkgName + "." + name]; isCustomType {
			return propertyName{SchemaType: actualPrimitiveType, ArrayType: actualPrimitiveType}, nil
		}

		schemeType := TransToValidSchemeType(name)
		return propertyName{SchemaType: schemeType, ArrayType: schemeType}, nil
	default:
		return propertyName{}, errors.New("not supported" + fmt.Sprint(expr))
	}
}

func getArrayPropertyName(pkgName string, astTypeArrayElt ast.Expr, parser *Parser) propertyName {
	switch elt := astTypeArrayElt.(type) {
	case *ast.StructType, *ast.MapType, *ast.InterfaceType:
		return propertyName{SchemaType: "array", ArrayType: "object"}
	case *ast.ArrayType:
		return propertyName{SchemaType: "array", ArrayType: "array"}
	case *ast.StarExpr:
		return getArrayPropertyName(pkgName,elt.X, parser)
	case *ast.SelectorExpr:
		return parseFieldSelectorExpr(elt, parser, newArrayProperty)
	case *ast.Ident:
		name := elt.Name
		if actualPrimitiveType, isCustomType := parser.CustomPrimitiveTypes[pkgName + "." + name]; isCustomType {
			name = actualPrimitiveType
		}else{
                       name = TransToValidSchemeType(name)
                }
		return propertyName{SchemaType: "array", ArrayType: name}
	default:
		name := fmt.Sprintf("%s", astTypeArrayElt)
		if actualPrimitiveType, isCustomType := parser.CustomPrimitiveTypes[pkgName + "." + name]; isCustomType {
			name = actualPrimitiveType
		}else{
                       name = TransToValidSchemeType(name)
                }
		return propertyName{SchemaType: "array", ArrayType: name}
	}
}

@codecov-io
Copy link

codecov-io commented Feb 22, 2020

Codecov Report

Merging #629 into master will decrease coverage by 0.09%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #629     +/-   ##
=========================================
- Coverage   85.94%   85.84%   -0.1%     
=========================================
  Files           7        7             
  Lines        1700     1703      +3     
=========================================
+ Hits         1461     1462      +1     
- Misses        149      151      +2     
  Partials       90       90
Impacted Files Coverage Δ
parser.go 80.84% <100%> (+0.02%) ⬆️
property.go 91.66% <86.66%> (-2.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f150c13...2c8dd22. Read the comment docs.

@ghost
Copy link
Author

ghost commented Feb 22, 2020

@sdghchj Good job! I used your code, but added FuncType part from original:

case *ast.FuncType:
	return propertyName{SchemaType: "func", ArrayType: ""}, nil

In current state code are passing tests.

@sdghchj
Copy link
Member

sdghchj commented Feb 23, 2020

What does FuncTyp works for? @wsnotify

@ghost
Copy link
Author

ghost commented Feb 23, 2020

What does FuncTyp works for? @wsnotify

In master we have:

	if _, ok := expr.(*ast.FuncType); ok { // if func()
		return propertyName{SchemaType: "func", ArrayType: ""}, nil
	}

Without this part we failing test at parser_test.go:1827 and parser_test.go:1830 (testdata/simple3)

@sdghchj
Copy link
Member

sdghchj commented Feb 23, 2020

What does FuncTyp works for? @wsnotify

In master we have:

	if _, ok := expr.(*ast.FuncType); ok { // if func()
		return propertyName{SchemaType: "func", ArrayType: ""}, nil
	}

Without this part we failing test at parser_test.go:1827 and parser_test.go:1830 (testdata/simple3)

What does FuncTyp works for? @wsnotify

In master we have:

	if _, ok := expr.(*ast.FuncType); ok { // if func()
		return propertyName{SchemaType: "func", ArrayType: ""}, nil
	}

Without this part we failing test at parser_test.go:1827 and parser_test.go:1830 (testdata/simple3)

I found no FuncType members in testdata/simple3

@ghost
Copy link
Author

ghost commented Feb 23, 2020

I found no FuncType members in testdata/simple3

testdata/simple3/web/handler.go:34
Function func()

@sdghchj
Copy link
Member

sdghchj commented Feb 23, 2020

I found no FuncType members in testdata/simple3

testdata/simple3/web/handler.go:34
Function func()

Sorry, I've still got an old branch.

Copy link
Member

@sdghchj sdghchj left a comment

Choose a reason for hiding this comment

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

LGTM

@sdghchj
Copy link
Member

sdghchj commented Feb 23, 2020

@wsnotify Thank you for your contributions.

@sdghchj sdghchj closed this Feb 23, 2020
@sdghchj sdghchj reopened this Feb 23, 2020
@sdghchj sdghchj merged commit 5cd03f0 into swaggo:master Feb 23, 2020
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.

Conflicting package + struct name
4 participants