aerospike / aerospike-client-go

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

Type consts are hidden in `internal/particle_type` #409

Closed jpicht closed 1 year ago

jpicht commented 1 year ago

Due to massive dependency trees in a lot of old projects we've been stuck on old (ancient) library versions forever. I'm trying to get to the latest v4 version at least, as the massive API changes between v4 and v6 seem insurmountable right now. While upgrading I found a lot of things quite straight forward, but one thing seems broken in the latest v4:

The type consts were hidden (without explanation and without a migration guide) by commit c753dc2, but Value.GetType() still returns them. This is very inconsistent and forced me to use an ugly workaround:

const particleType_INTEGER = 1

func init() {
    var iv aerospike.IntegerValue
    if iv.GetType() != particleType_INTEGER {
        panic("aerospike changed ParticleType.INTEGER")
    }
}

Is there any better option, that does not require refactoring to not use GetType? What is the expected use case for GetType if the value is intentionally opaque to the library user?

khaf commented 1 year ago

Argh. I'll move them back again. Give me a day or two.

khaf commented 1 year ago

In practice, Value.GetType() is not really needed or idiomatic in Go, since you could simply type assert on the interface itself like:

v, ok := record.Bins["bin_name"].(int);
if !ok {
  // return some error or set v to a default value
}

or you could:

switch record.Bins["bin_name"].(type) {
case int:
       ///
case float64:
      ////
}

That does not excuse the issue though. I'll return the type so it works as it did before.

jpicht commented 1 year ago

Yeah, I would have done that refactoring if ParticleType.INTEGER was a one to one mapping, but sadly it's used by multiple types. So it's more work than I can justify right now. We'll tackle the refactoring to idomatic use when we move to the newer versions.

khaf commented 1 year ago

Version v4.7.0 released.

jpicht commented 1 year ago

That's great, though that version is unusable, because of #411

khaf commented 1 year ago

Addressed for the #411 in v4.7.1 release.

jpicht commented 1 year ago

Solved by v4.7.1, thanks!