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

Issue with json struct tag omitempty #20

Closed
PatrLind opened this issue Oct 12, 2023 · 6 comments · Fixed by #21
Closed

Issue with json struct tag omitempty #20

PatrLind opened this issue Oct 12, 2023 · 6 comments · Fixed by #21

Comments

@PatrLind
Copy link

I found this bug in gocsv: gocarina/gocsv#263
and thought I would test the same on your project, since it seems better in many ways.
But unfortunately it seems that this project has the same bug, or at least what I think is a bug.

The error is that the code panics when having json struct tags with omitempty on them. I haven't investigated very deeply, but it seems like it should not panic.

Example panic output:

--- FAIL: TestCSV (0.00s)
    --- FAIL: TestCSV/test_1_json (0.00s)
panic: reflect: call of reflect.Value.Field on zero Value [recovered]
	panic: reflect: call of reflect.Value.Field on zero Value

Test code I used:

package main

import (
	"bytes"
	"encoding/json"
	"fmt"
	"testing"

	"github.com/shigetaichi/xsv"
)

type TestObj struct {
	Name    string     `json:"name,omitempty"`
	SubObj  *TestOb2   `json:"subObj,omitempty"`
	SubObjs []*TestOb2 `json:"subObjs,omitempty"`
}

type TestOb2 struct {
	A string `json:"a,omitempty"`
	B string `json:"b,omitempty"`
}

func TestCSV(t *testing.T) {
	obj1 := []*TestObj{
		{
			Name: "Obj1",
			SubObj: &TestOb2{
				A: "a1",
				B: "b1",
			},
			SubObjs: []*TestOb2{
				{
					A: "a2",
					B: "b2",
				},
				{
					A: "a3",
					B: "b3",
				},
			},
		},
	}

	testCases := []struct {
		TagName string
	}{
		{"json"}, {"csv"},
	}
	for i, tc := range testCases {
		tc := tc
		t.Run(fmt.Sprintf("test_%d_%s", i+1, tc.TagName), func(t *testing.T) {
			xsvWrite := xsv.NewXsvWrite[*TestObj]()
			xsvWrite.TagName = tc.TagName
			buf := bytes.NewBuffer(nil)
			err := xsvWrite.SetBufferWriter(buf).Write(obj1)
			if err != nil {
				t.Error(err)
			}
			fmt.Println("Buf:", buf.String())
			obj2 := []*TestObj{}
			xsvReader := xsv.NewXsvRead[*TestObj]()
			xsvReader.TagName = tc.TagName
			err = xsvReader.SetStringReader(buf.String()).ReadTo(&obj2)
			if err != nil {
				t.Error(err)
			}
			if fmt.Sprint(obj1) != fmt.Sprint(obj2) {
				t.Errorf("objects not equal. Obj1: %v, Obj2: %v", obj1, obj2)
			}
		})
	}
}

func (o *TestObj) String() string {
	data, err := json.Marshal(o)
	if err != nil {
		panic(err)
	}
	return string(data)
}
@PatrLind
Copy link
Author

I investigated a little bit more, and found that I could change this code (https://github.com/shigetaichi/xsv/blob/main/types.go#L220) from:

		if omitEmpty && value == "" {
			return nil
		}

to:

		if omitEmpty && value == "" {
			field.Set(reflect.New(field.Type().Elem()))
			return nil
		}

and it seems to work, not sure if it breaks something...

PatrLind pushed a commit to PatrLind/xsv that referenced this issue Oct 12, 2023
@PatrLind
Copy link
Author

I made a fork with a small fix that seems to work. The tests passes at least. But, I'm actually not sure if it breaks something...
7e010c2

@shigetaichi
Copy link
Owner

@PatrLind
Thank you for your careful description and writing. Your contribution is welcome.
I have also checked with me about this. Some of the information is unclear, but it is information sharing.


I am getting an error with a pointer type of primitive type with omitempty attached.
It seems like the error occurred in the opposite of the way you suggested, correctly.

To verbalize the process that line 220 is doing,
Reassign the underlying type of the pointer type to the 'field' variable in the case of a field of pointer type with "omitempty attached" and "empty string coming across".
is.

If the field is "structure pointer type and omitempty", then the change description you gave us will assign the specified type to the field argument, which will allow us to execute the decode.go line 171 process correctly.

On the other hand, if the field is a "primitive pointer type and omitempty", the process is still not followed properly😅.

@shigetaichi
Copy link
Owner

shigetaichi commented Oct 14, 2023

gocarina/gocsv#135
I have read the above issue.
I am not too sure what the omitempty behavior in the gocsv package means anymore.
I thought it was "if all the data in the field is zero, the entire column is removed from the output", but it seems that this is not the case.
Sorry, I see that omitempty is a function that specifies whether or not a pointer type field should be set to nil when a type string is entered into it.

If you don't need omitempty as a feature, i may want to remove it from the package.

@shigetaichi
Copy link
Owner

@PatrLind
The first argument of setField comes in to get the initial value of each field, plz check setInnerField func.
If each field is a pointer type, the initial value will be nil.

At that point, it would determine if the tag had omitempty or if the value from the csv was an empty string, and if both condition was met, it would do nothing and move on to the next field.

This 👆 part was the factor of this issue.
In the case of fields of nested pointer type structs, setInnerField is executed recursively, so the first argument of setField may contain a struct in the middle of the nest.

Since the existing code only checked for omitempty and empty characters, the processing of the setField function was interrupted when the struct in the middle of the nest was a pointer type, and the processing was not transferred to the field of the nested struct itself.

I have created a PR and plz check here. -> #21
I have also added one test.
Thank you for creating the issue and investigating the problem🙇.

@PatrLind
Copy link
Author

Your changes in the PR seems to fix the issue, thank you!

shigetaichi added a commit that referenced this issue Oct 17, 2023
#21)

* fix: prevent the error occurs when omitempty is present in a field of an embedded pointer type structure

#20

* test: Creating a test when the embedded struct is a pointer type and all its fields have omitempty
@github-staff github-staff deleted a comment from otodak Sep 29, 2024
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 a pull request may close this issue.

2 participants