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

Do not use pointers on named map types #1116

Closed
wants to merge 2 commits into from

Conversation

bastianccm
Copy link

Named map types such as
type Foo map[string]interface{}
are nilable and named, which means they should not be references as a
pointer in generated code.

I am unsure if the test code is in the proper location.

This resolves #878

Describe your PR and link to any relevant issues.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

Named map types such as
type Foo map[string]interface{}
are nilable and named, which means they should not be references as a
pointer in generated code.
@@ -84,8 +84,10 @@ func (b *builder) buildObject(typ *ast.Definition) (*Object, error) {
func (o *Object) Reference() types.Type {
switch v := o.Type.(type) {
case *types.Named:
_, isInterface := v.Underlying().(*types.Interface)
if isInterface {
if _, isInterface := v.Underlying().(*types.Interface); isInterface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be calling

func IsNilable(t types.Type) bool {
(and make it public) to stop these falling out of sync again in the future?

@bastianccm
Copy link
Author

Hey @vektah,

in general I agree, however updating it I stumbled upon another issue: the same issue this change fixes for maps applies to slices as well.
To have correct generated code I would use

func (o *Object) Reference() types.Type {
	if config.IsNilable(o.Type) {
		return o.Type
	}
	return types.NewPointer(o.Type)
}

for the object generation (the change you suggested), and update IsNilable to include a check for types.Slice as well

_, isSlice := t.(*types.Slice)
return isPtr || isMap || isInterface || isSlice

However this breaks TestIntrospection/enabled_by_default and TestSlices/nulls_vs_empty_slices.
I can fix the nulls_vs_empty_slices tests by using

return &Slices{
	Test1: nil,
	Test2: nil,
	Test3: make([]*string, 0),
	Test4: make([]string, 0),
}, nil

to mark explicitly what is null and what is empty.
However for the introspection I am unsure if I am on the right path doing these changes.

I pushed this as a new commit, with the failing tests, so you have a chance to see what I did.

Maybe you have an idea/opinion on this topic?

@bastianccm bastianccm requested a review from vektah June 22, 2020 07:21
@lwc lwc mentioned this pull request Jul 10, 2020
2 tasks
@lwc
Copy link
Member

lwc commented Jul 10, 2020

Replaced with #1242 thanks for the contribution!

@lwc lwc closed this Jul 10, 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.

map[string]interface has issue when generating in v0.10.0 or v0.10.1
3 participants