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

add support for map nested in struct #521

Merged
merged 7 commits into from
Nov 22, 2019
Merged

Conversation

lvandenbrink
Copy link
Contributor

Mappings nested in structs are not generated with additional properties. If a field is of a MapType, add the field with additional properties. If needed add the key and value type to the scheme.

Relation issue
#517

@codecov-io
Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #521 into master will increase coverage by 0.06%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #521      +/-   ##
=========================================
+ Coverage   87.24%   87.3%   +0.06%     
=========================================
  Files           7       7              
  Lines        1560    1599      +39     
=========================================
+ Hits         1361    1396      +35     
- Misses        117     119       +2     
- Partials       82      84       +2
Impacted Files Coverage Δ
parser.go 82.35% <84.61%> (+0.32%) ⬆️

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 b564023...07be9c2. Read the comment docs.

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.

can you help add tests for it?

@lvandenbrink
Copy link
Contributor Author

I added a FooBarMap struct to the api in composition test. With view-covered I see that the tests the covers the added code. Except for when the map is required or the parsing of the value fails.

If that doesn't suffice, can you indicate which part should further tested and where to add it?

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

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

Please see dictionaries spec, The actual code inserts a definition to the object "api.MapValue" that is not referenced anywhere.

}
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Definition

type FooBarMap struct {
	Field3 map[string]MapValue
}

type MapValue struct {
	Field4 string
}

should produce

api.FooBarMap:
    type: object
    properties:
      field3:
        type: object
        additionalProperties:
          $ref: '#/definitions/api.MapValue'

  api.MapValue:
    type: object
    properties:
      field4:
        type: string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be indeed a better approach. Although I see if there is a map in operations it doesn't use the reference, but also inlines the value.

So to do this. the scheme of the additionalProperties needs to be something like:

mapValueScheme :=  &spec.Schema{
	SchemaProps: spec.SchemaProps{
		Ref: spec.Ref{
			Ref: jsonreference.MustCreateRef("#/definitions/" + pkgName + "." + ???),
		},
	},
}

But I have some questions about how to get the ???.

With itemSchema, err := parser.parseTypeExpr(pkgName, "", astTypeMap.Value) the MapValue is parsed and added to the definitions. The name api.MapValue is refTypeName := fullTypeName(pkgName, expr.Name) in the case that astTypeMap.Value is of type *ast.Ident. Although I don't see a clear way to get the name api.MapValue from either itemSchema or astTypeMap.Value. Is there are already a function that returns "api.MapValue"?

I case no, what would be the best solution?

  1. Write a function that returns api.MapValue given astTypeMap?
  2. Or let parser.parseTypeExpr also return the definition key (api.MapValue)?

Copy link
Contributor

@ubogdan ubogdan Nov 21, 2019

Choose a reason for hiding this comment

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

This will register your api.MapValue to definitions

		itemSchema, err := parser.parseTypeExpr(pkgName, "", astTypeMap.Value)
		if err != nil {
			return properties, nil, err
		}

Neighter of 2 solutions.

You need to use the astTypeMap.Value to add it to object..

fullTypeName, err := getFieldType(astTypeMap.Value)
if err != nil {
	return ....
}
mapValueScheme :=  &spec.Schema{
	SchemaProps: spec.SchemaProps{
		Ref: spec.Ref{
			Ref: jsonreference.MustCreateRef("#/definitions/" + fullTypeName),
		},
	},
}

I'm quite busy these days and I don't have much time to spend here. I hope i was clear enough with the last response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You made it clear. Almost did it yourself ><

Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

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

LGTM

@ubogdan ubogdan merged commit 68ab45d into swaggo:master Nov 22, 2019
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.

4 participants