aerospike / aerospike-client-go

Aerospike Client Go
Apache License 2.0
432 stars 198 forks source link

Add anonymous struct field support. #287

Closed andot closed 4 years ago

andot commented 4 years ago

Fixed #286 & #180

khaf commented 4 years ago

@andot Thanks for the PR. Could you please add a good test in the client_reflect_test.go to make sure things work?

andot commented 4 years ago

@khaf I add a new test file anonymous_fields_test.go.

khaf commented 4 years ago

@andot Thanks. I believe the mapping code is not sophisticated enough and will fail. Take the following example:

var _ = Describe("Aerospike", func() {

    Describe("PutObject and GetObject with anonymous fields", func() {
        // connection data
        var err error
        var ns = *namespace
        var set = randString(50)

        type anonymousStructA struct {
            A int `as:"a"`
        }

        type anonymousStructB struct {
            B string `as:"b"`
        }

        type anonymousStructABC struct {
            anonymousStructA
            anonymousStructB
            A bool    `as:"ace"`
            B int     `as:"b"`
            C float64 `as:"c"`
        }

        type anonymousStructABCD struct {
            ABC *anonymousStructABC
            D   bool `as:"d"`
        }

        type testStruct struct {
            anonymousStructABC
            anonymousStructABCD
        }

        makeTestObject := func() *testStruct {
            obj := &testStruct{}
            obj.A = true
            obj.B = 10
            obj.anonymousStructA.A = 10
            obj.C = 3.14159
            obj.ABC = &anonymousStructABC{}
            obj.ABC.A = false
            obj.ABC.B = 42
            obj.ABC.anonymousStructB.B = "World!"
            obj.ABC.C = 2.17828
            obj.D = true
            return obj
        }

        Context("PutObject & GetObject operations", func() {
            It("must save an object with anonymous fields", func() {
                key, _ := as.NewKey(ns, set, randString(50))
                expected := makeTestObject()
                err = client.PutObject(nil, key, expected)
                Expect(err).ToNot(HaveOccurred())

                var actual testStruct
                err = client.GetObject(nil, key, &actual)
                Expect(err).ToNot(HaveOccurred())
                Expect(actual).To(Equal(*expected))

                rec, err := client.Get(nil, key)
                Expect(err).ToNot(HaveOccurred())
                                // make sure the returned BinMap here reflects what you
                                // expect the final marshalled object should be.
                Expect(rec.Bins).To(Equal(as.BinMap{}))
            })
        })
    })
})

Since there are multiple B fields, the unmarshal has trouble finding out which value belongs to which, and panics. The mapper code should be enhanced to support this use case when the types are incompatible (like int and string for B), but I worry about the compatible types (like bool and int or even int and int for A). So I guess the mapping should become a map[string]interface{} to allow for nested maps.

andot commented 4 years ago

I think change mapping from map[string]string to map[string][]int is better than map[string]interface{}, the []int element stores the Index of the field.

        type anonymousStructB struct {
            B string `as:"b"`
        }

        type anonymousStructABC struct {
            anonymousStructA
            anonymousStructB
            A bool    `as:"ace"`
            B int     `as:"b"`
            C float64 `as:"c"`
        }

But I think the B in anonymousStructB and anonymousStructABC is conflicted by tag. Define different tags can solve this problem.

khaf commented 4 years ago

I believe the mapper can easily detect this conflict and panic if it detects the conflict exists.

andot commented 4 years ago

@khaf The new test is passed.

andot commented 4 years ago

I will change the metaMappings (ttlMap, genMap) from []string to [][]int too.

khaf commented 4 years ago

Can you please also make sure that ambiguous fields with the same name or alias are not allowed and the library panics if it detects such a situation?

andot commented 4 years ago

@khaf I find in https://github.com/aerospike/aerospike-client-go/blob/5990c8c084f2e2d6da98598312b333a29d661bda/client_object_test.go#L307-L309

EmpNstdObjSlic and EmpNstdObjPSlc have the same alias, the modified code detects it and panics.

khaf commented 4 years ago

perfect!

khaf commented 4 years ago

I don't understand how the test for TTL fields work. I expected that the TTL fields for anonymousStructA and anonymousStructB to conflict with each other and panic, since they have the same level of precedence and have the same tag. Do you treat the meta tags differently than the data tags?

andot commented 4 years ago

https://github.com/aerospike/aerospike-client-go/blob/5990c8c084f2e2d6da98598312b333a29d661bda/client_object_test.go#L822-L823

TTL1, TTL2 is also in the same level of precedence and have the same tag, but the test didn't treat them to conflict with each other and panic. so I did the same thing.

khaf commented 4 years ago

Fair enough. They will be the same value anyway.

andot commented 4 years ago

@khaf Can you merge this PR? We need this feature urgently in production.

khaf commented 4 years ago

Released in v2.9.0. Thank you for your contribution.