Handle duplicate route declaration better (#968)

* make RangeFiles deterministic

* add testcase for duplicate route, expecting an error

* fix issue with files being parsed twice.

* detect when a route is declared multiple times and warn,
or error when in strict mode.

gen/gen.go

@@ -60,6 +60,9 @@ type Config struct {
 	// ParseInternal whether swag should parse internal packages
 	ParseInternal bool
 
+	// Strict whether swag should error or warn when it detects cases which are most likely user errors
+	Strict bool
+
 	// MarkdownFilesDir used to find markdownfiles, which can be used for tag descriptions
 	MarkdownFilesDir string
 
@@ -85,7 +88,8 @@ func (g *Gen) Build(config *Config) error {
 	log.Println("Generate swagger docs....")
 	p := swag.New(swag.SetMarkdownFileDirectory(config.MarkdownFilesDir),
 		swag.SetExcludedDirsAndFiles(config.Excludes),
-		swag.SetCodeExamplesDirectory(config.CodeExampleFilesDir))
+		swag.SetCodeExamplesDirectory(config.CodeExampleFilesDir),
+		swag.SetStrict(config.Strict))
 	p.PropNamingStrategy = config.PropNamingStrategy
 	p.ParseVendor = config.ParseVendor
 	p.ParseDependency = config.ParseDependency

gen/gen_test.go

@@ -423,3 +423,22 @@ func TestGen_cgoImports(t *testing.T) {
 		os.Remove(expectedFile)
 	}
 }
+
+func TestGen_duplicateRoute(t *testing.T) {
+	searchDir := "../testdata/duplicate_route"
+
+	config := &Config{
+		SearchDir:          searchDir,
+		MainAPIFile:        "./main.go",
+		OutputDir:          "../testdata/duplicate_route/docs",
+		PropNamingStrategy: "",
+		ParseDependency:    true,
+	}
+	err := New().Build(config)
+	assert.NoError(t, err)
+
+	// with Strict enabled should cause an error instead of warning about the duplicate route
+	config.Strict = true
+	err = New().Build(config)
+	assert.EqualError(t, err, "route GET /testapi/endpoint is declared multiple times")
+}

packages.go

@@ -3,6 +3,8 @@ package swag
 import (
 	"go/ast"
 	"go/token"
+	"path/filepath"
+	"sort"
 	"strings"
 )
 
@@ -23,26 +25,30 @@ func NewPackagesDefinitions() *PackagesDefinitions {
 }
 
 // CollectAstFile collect ast.file.
-func (pkgs *PackagesDefinitions) CollectAstFile(packageDir, path string, astFile *ast.File) {
+func (pkgs *PackagesDefinitions) CollectAstFile(packageDir, path string, astFile *ast.File) error {
 	if pkgs.files == nil {
 		pkgs.files = make(map[*ast.File]*AstFileInfo)
 	}
 
-	pkgs.files[astFile] = &AstFileInfo{
-		File:        astFile,
-		Path:        path,
-		PackagePath: packageDir,
+	if pkgs.packages == nil {
+		pkgs.packages = make(map[string]*PackageDefinitions)
 	}
 
+	// return without storing the file if we lack a packageDir
 	if len(packageDir) == 0 {
-		return
+		return nil
 	}
 
-	if pkgs.packages == nil {
-		pkgs.packages = make(map[string]*PackageDefinitions)
+	path, err := filepath.Abs(path)
+	if err != nil {
+		return err
 	}
 
 	if pd, ok := pkgs.packages[packageDir]; ok {
+		// return without storing the file if it already exists
+		if _, exists := pd.Files[path]; exists {
+			return nil
+		}
 		pd.Files[path] = astFile
 	} else {
 		pkgs.packages[packageDir] = &PackageDefinitions{
@@ -51,12 +57,29 @@ func (pkgs *PackagesDefinitions) CollectAstFile(packageDir, path string, astFile
 			TypeDefinitions: make(map[string]*TypeSpecDef),
 		}
 	}
+
+	pkgs.files[astFile] = &AstFileInfo{
+		File:        astFile,
+		Path:        path,
+		PackagePath: packageDir,
+	}
+
+	return nil
 }
 
-// RangeFiles for range the collection of ast.File.
+// RangeFiles for range the collection of ast.File in alphabetic order.
 func (pkgs *PackagesDefinitions) RangeFiles(handle func(filename string, file *ast.File) error) error {
-	for file, info := range pkgs.files {
-		if err := handle(info.Path, file); err != nil {
+	sortedFiles := make([]*AstFileInfo, 0, len(pkgs.files))
+	for _, info := range pkgs.files {
+		sortedFiles = append(sortedFiles, info)
+	}
+
+	sort.Slice(sortedFiles, func(i, j int) bool {
+		return strings.Compare(sortedFiles[i].Path, sortedFiles[j].Path) < 0
+	})
+
+	for _, info := range sortedFiles {
+		if err := handle(info.Path, info.File); err != nil {
 			return err
 		}
 	}

parser.go

@@ -81,6 +81,9 @@ type Parser struct {
 	// ParseInternal whether swag should parse internal packages
 	ParseInternal bool
 
+	// Strict whether swag should error or warn when it detects cases which are most likely user errors
+	Strict bool
+
 	// structStack stores full names of the structures that were already parsed or are being parsed now
 	structStack []*TypeSpecDef
 
@@ -159,6 +162,13 @@ func SetExcludedDirsAndFiles(excludes string) func(*Parser) {
 	}
 }
 
+// SetStrict sets whether swag should error or warn when it detects cases which are most likely user errors
+func SetStrict(strict bool) func(*Parser) {
+	return func(p *Parser) {
+		p.Strict = strict
+	}
+}
+
 // ParseAPI parses general api info for given searchDir and mainAPIFile.
 func (parser *Parser) ParseAPI(searchDir string, mainAPIFile string, parseDepth int) error {
 	return parser.ParseAPIMultiSearchDir([]string{searchDir}, mainAPIFile, parseDepth)
@@ -599,23 +609,18 @@ func (parser *Parser) ParseRouterAPIInfo(fileName string, astFile *ast.File) err
 					if pathItem, ok = parser.swagger.Paths.Paths[routeProperties.Path]; !ok {
 						pathItem = spec.PathItem{}
 					}
-					switch strings.ToUpper(routeProperties.HTTPMethod) {
-					case http.MethodGet:
-						pathItem.Get = &operation.Operation
-					case http.MethodPost:
-						pathItem.Post = &operation.Operation
-					case http.MethodDelete:
-						pathItem.Delete = &operation.Operation
-					case http.MethodPut:
-						pathItem.Put = &operation.Operation
-					case http.MethodPatch:
-						pathItem.Patch = &operation.Operation
-					case http.MethodHead:
-						pathItem.Head = &operation.Operation
-					case http.MethodOptions:
-						pathItem.Options = &operation.Operation
+
+					// check if we already have a operation for this path and method
+					if hasRouteMethodOp(pathItem, routeProperties.HTTPMethod) {
+						err := fmt.Errorf("route %s %s is declared multiple times", routeProperties.HTTPMethod, routeProperties.Path)
+						if parser.Strict {
+							return err
+						}
+						Printf("warning: %s\n", err)
 					}
 
+					setRouteMethodOp(&pathItem, routeProperties.HTTPMethod, &operation.Operation)
+
 					parser.swagger.Paths.Paths[routeProperties.Path] = pathItem
 				}
 			}
@@ -625,6 +630,46 @@ func (parser *Parser) ParseRouterAPIInfo(fileName string, astFile *ast.File) err
 	return nil
 }
 
+func setRouteMethodOp(pathItem *spec.PathItem, method string, op *spec.Operation) {
+	switch strings.ToUpper(method) {
+	case http.MethodGet:
+		pathItem.Get = op
+	case http.MethodPost:
+		pathItem.Post = op
+	case http.MethodDelete:
+		pathItem.Delete = op
+	case http.MethodPut:
+		pathItem.Put = op
+	case http.MethodPatch:
+		pathItem.Patch = op
+	case http.MethodHead:
+		pathItem.Head = op
+	case http.MethodOptions:
+		pathItem.Options = op
+	}
+}
+
+func hasRouteMethodOp(pathItem spec.PathItem, method string) bool {
+	switch strings.ToUpper(method) {
+	case http.MethodGet:
+		return pathItem.Get != nil
+	case http.MethodPost:
+		return pathItem.Post != nil
+	case http.MethodDelete:
+		return pathItem.Delete != nil
+	case http.MethodPut:
+		return pathItem.Put != nil
+	case http.MethodPatch:
+		return pathItem.Patch != nil
+	case http.MethodHead:
+		return pathItem.Head != nil
+	case http.MethodOptions:
+		return pathItem.Options != nil
+	}
+
+	return false
+}
+
 func convertFromSpecificToPrimitive(typeName string) (string, error) {
 	name := typeName
 	if strings.ContainsRune(name, '.') {