Implement nested structs for columns and values

[ricci2511]

Closes #54.

Support for embedded structs has been added for both scan.Columns and scan.Values through good old recursion.
I just to want to explain a little bit more in detail my changes to scan.Values.

So first of all I opted to store a slice of ints in the cache map, representing the struct field index. This allows us to directly access the corresponding struct field value while iterating over the provided cols in the main Values function.
Hopefully this image of the debugger which shows the value of fields := loadFields(model) can make it more clear:

The test that was run for the image above:

func TestValuesScansNestedFields(t *testing.T) {
	type Address struct {
		Street string
		City   string
	}

	type Person struct {
		Name string
		Age  int
		Address
	}

	p := &Person{Name: "Brett", Address: Address{Street: "123 Main St", City: "San Francisco"}}

	vals, err := Values([]string{"Name", "Street", "City"}, p)
	require.NoError(t, err)

	assert.EqualValues(t, []interface{}{"Brett", "123 Main St", "San Francisco"}, vals)
}

[coveralls]

coverage: 98.79% (-0.8%) from 99.563% when pulling f64314d on ricci2511:columns-values-nested-structs-feat into da49f61 on blockloop:master.

[blockloop]

Hey @ricci2511 this is awesome! I'll take a look at this later this evening and merge it if all is well. Sorry for the late response. I fixed my notifications.

[blockloop]

func TestColumnsAddsComplexTypesWhenStructTag(t *testing.T) {
	type person struct {
		Address struct {
			Street string
		} `db:"address"`
	}

	cols, err := Columns(&person{})
	assert.NoError(t, err)
	assert.EqualValues(t, []string{"address", "Street"}, cols)
}

I'll be honest, I can't remember my original intention with this test. I don't know what use address is as a column name since it won't map to anything in a query. If you had this value how would you even query? For example, if I used this object and it gave me ["address", "Street"] and I tried to query this database:

+------------+
| addresses  |
+------------+
| id (PK)    |
| person_id  |--------+
| street     |        |
+------------+        |
                      |
+-----------+         |
|   persons |         |
+-----------+         |
| id (PK)   |<--------+
| name      |
| age       |
+-----------+

I think the expected outcome of this test should be []string{"Street"}. The test should look like this I think:

func TestColumnsAddsComplexTypes(t *testing.T) {
	type person struct {
		Address struct {
			Street string
		}
	}

	cols, err := Columns(&person{})
	assert.NoError(t, err)
	assert.EqualValues(t, []string{"Street"}, cols)
}

[ricci2511]

@blockloop Yeah, before I looked into the code for the columns scanning I was a little confused too. I honestly didn't think too much about it and didn't want to introduce any "breaking" changes, but you're right, the Address is actually a related table field and not a column, so I guess it doesn't really make sense to include it. But currently it only is included if it has a db tag, see (tests before the nested columns and values feat):

func TestColumnsAddsComplexTypesWhenStructTag(t *testing.T) { 
         type person struct { 
                 Address struct { 
                         Street string 
                 } `db:"address"` 
         } 
  
         cols, err := Columns(&person{}) 
         assert.NoError(t, err) 
         assert.EqualValues(t, []string{"address"}, cols) 
 } 
  
 func TestColumnsIgnoresComplexTypesWhenNoStructTag(t *testing.T) { 
         type person struct { 
                 Address struct { 
                         Street string 
                 } 
         } 
  
         cols, err := Columns(&person{}) 
         assert.NoError(t, err) 
         assert.EqualValues(t, []string{}, cols)

In the columns method while looping through the fields the db tag check currently doesn't care about the supportedColumnType, so it will be included if the struct field has the tag even though reflect.Struct is not a supported column type. See:

		typeField := model.Type().Field(i)
		if tag, ok := typeField.Tag.Lookup(dbTag); ok {
			if tag != "-" && !isExcluded(tag) {
				names = append(names, tag)
			}
			continue
		}

Should we skip this step for embedded struct fields? If Im not mistaken it could be achieved by moving the nested struct traversal before the above block of code that checks for the db tag instead of after:

                if typeField.Type.Kind() == reflect.Struct { 
                         embeddedNames := columnNames(valField, strict, excluded...)
                         names = append(names, embeddedNames...) 
                         continue // skips dbTag check for struct field
                 }

What do you think?

[ricci2511]

Once we know for sure how to handle this, I can make another commit for it and also include the fix for the coverage percentage decrease, the cause probably being the redundant error return type in the columnNames function I wrote.

[blockloop]

Yes I think maybe the original intention was probably to prepend the struct's db tag to the subsequent fields. So for example:

func TestColumnsAddsComplexTypesWhenStructTag(t *testing.T) { 
         type person struct { 
                 Address struct { 
                         Street string 
                 } `db:"address"` 
         } 
  
         cols, err := Columns(&person{}) 
         assert.NoError(t, err) 
         assert.EqualValues(t, []string{"address.Street"}, cols) 
 } 

Even then I think that's a bit too magic for my taste and I'd probably just prefer to be explicit where the column names are ambiguous:

func TestColumnsAddsComplexTypesWhenStructTag(t *testing.T) { 
         type person struct { 
                 Address struct { 
                         Street string `db:"address.street"`
                 }  
         } 
  
         cols, err := Columns(&person{}) 
         assert.NoError(t, err) 
         assert.EqualValues(t, []string{"address.street"}, cols)  // PASSES
 } 

However if the strict mode is enabled then it would require that the Address still have some kind of struct tag or the above would just return nothing:

func TestColumnsStrictAddsComplexTypesWhenStructTag(t *testing.T) { 
         type person struct { 
                 Address struct { 
                         Street string `db:"address.street"`
                 }  
         } 
  
         cols, err := ColumnsStrict(&person{}) 
         assert.NoError(t, err) 
         assert.EqualValues(t, []string{"address.street"}, cols) // FAILS because Address struct is skipped since it has no tag
 } 

I think in strict mode we should just continue recursively when we run into a struct field and let the recursion continue searching through subsequent fields. Unless the db:"-" tag exists, which should explicitly ignore all subsequent fields in the struct.

I think if these tests passed it would make the most sense:

func TestColumnsAddsComplexTypesWhenNoStructTag(t *testing.T) { 
        type person struct { 
                Address struct { 
                        Street string
                }  
        } 
  
        cols, err := Columns(&person{}) 
        assert.NoError(t, err) 
        assert.EqualValues(t, []string{"Street"}, cols)
}

func TestColumnsAddsComplexTypesWhenStructTag(t *testing.T) { 
        type person struct { 
                Address struct { 
                        Street string `db:"address.street"`
                }  
        } 
  
        cols, err := Columns(&person{}) 
        assert.NoError(t, err) 
        assert.EqualValues(t, []string{"address.street"}, cols)
}
 
func TestColumnsStrictAddsComplexTypesWhenStructTag(t *testing.T) { 
        type person struct { 
                Address struct { 
                        Street string `db:"address.street"`
                }  
        } 
  
        cols, err := ColumnsStrict(&person{}) 
        assert.NoError(t, err) 
        assert.EqualValues(t, []string{"address.street"}, cols)
}
 
func TestColumnsStrictDoesNotAddComplexTypesWhenNoStructTag(t *testing.T) { 
        type person struct { 
                Address struct { 
                        Street string
                }  
        } 
  
        cols, err := ColumnsStrict(&person{}) 
        assert.NoError(t, err) 
        assert.EqualValues(t, []string{}, cols)
}
 

func TestColumnsStrictDoesNotAddComplexTypesWhenStructTagIgnored(t *testing.T) { 
        type person struct { 
                Address struct { 
                        Street string `db:"address.street"`
                } `db:"-"`
        } 
  
        cols, err := ColumnsStrict(&person{}) 
        assert.NoError(t, err) 
         
        // I could probably be convinced that this is bad. I'm on the fence about it. 
        // I wouldn't mind if instead this returned ["address.street"]
        assert.EqualValues(t, []string{}, cols)

 }

[ricci2511]

Actually pretty much all the tests you provided are passing with my current implementation, even the ones using ColumnsStrict because the strict check runs after the tag check and the recursive struct field traversal. Now the last test TestColumnsStrictDoesNotAddComplexTypesWhenStructTagIgnored does indeed return "address.street", but I guess we can tweak it to skip the subsequent fields if that's the intended behavior, and if so, what about this exact scenario but with the non strict variant?

Finally, what if the embedded struct field has a db tag? Should we end up skipping it like I mentioned before? Example:

	type person struct {
		Address struct {
			Street string `db:"address.street"`
		} `db:"address"`
	}

	cols, err := Columns(&person{})
	assert.NoError(t, err)
	assert.EqualValues(t, []string{"address.street"}, cols) // Exclude "address"?

[blockloop]

Yes I'm thinking it should just ignore the db tag entirely on struct types because it has no use. If all of the tests are passing I'm happy to merge after the coverage changes you mentioned.

[ricci2511]

All tests are passing and hopefully that coverage decrease is fixed too.

Just one thing, not sure if it bothers you having the same recursive piece of code in two places:

		typeField := model.Type().Field(i)
		if tag, ok := typeField.Tag.Lookup(dbTag); ok {
			if tag != "-" && !isExcluded(tag, excluded...) {
				// Only append names/tags of the subsequent fields and not the struct field itself
				if typeField.Type.Kind() == reflect.Struct {
                                        // THIS
					embeddedNames := columnNames(valField, strict, excluded...)
					names = append(names, embeddedNames...)
				} else {
					names = append(names, tag)
				}
			}

			continue
		}

		if typeField.Type.Kind() == reflect.Struct {
                        // THIS
			embeddedNames := columnNames(valField, strict, excluded...)
			names = append(names, embeddedNames...)
			continue
		}

Maybe names = append(names, columnNames(valField, strict, excluded...)...) is preferred even though it might be less readable.

[blockloop]

Okay I played with it a bit and I actually think we should just ignorestruct tags altogether since they don't really have any use. So lets change the test to:

func TestColumnsStrictAddsComplexTypesRegardlessOfStructTag(t *testing.T) {
	type person struct {
		Address struct {
			Street string `db:"address.street"`
		} `db:"-"`
	}

	cols, err := ColumnsStrict(&person{})
	assert.NoError(t, err)
	assert.EqualValues(t, []string{"address.street"}, cols)
}

and then I refactored the code a bit to make it more clear now that we can just ignore struct tag:

func columnNames(model reflect.Value, strict bool, excluded ...string) []string {
	numfield := model.NumField()
	names := make([]string, 0, numfield)

	for i := 0; i < numfield; i++ {
		valField := model.Field(i)
		if !valField.IsValid() || !valField.CanSet() {
			continue
		}

		typeField := model.Type().Field(i)

		if typeField.Type.Kind() == reflect.Struct {
			embeddedNames := columnNames(valField, strict, excluded...)
			names = append(names, embeddedNames...)
			continue
		}

		fieldName := typeField.Name
		if tag, hasTag := typeField.Tag.Lookup(dbTag); hasTag {
			if tag == "-" {
				continue
			}
			fieldName = tag
		} else if strict {
			// there's no tag name and we're in strict mode so move on
			continue
		}

		if isExcluded(fieldName, excluded...) {
			continue
		}

		if supportedColumnType(valField.Kind()) {
			names = append(names, fieldName)
		}
	}

	return names
}

[ricci2511]

Looks good to me, struct tags don't make a lot of sense and just overcomplicate things.
Should I push these changes or will you take care of them?

[blockloop]

You can push them. Thanks for the contribution. I'll merge once you push and everything passes.

[ricci2511]

You can push them. Thanks for the contribution. I'll merge once you push and everything passes.

Thank you for the great lib. Hopefully now everything should be ready to go 🚀