gogen-avro icon indicating copy to clipboard operation
gogen-avro copied to clipboard

make generated 'New<Type>' functions fill in defaults for UnionNull<X> fields

Open cameronbraid opened this issue 4 years ago • 4 comments

example, I have a schema with many union null types since to be backwards compatible this is how they need to be added

schema

{
  "type": "record",
  "namespace": "event",
  "name": "Foo",
  "fields": [
 {
      "name": "bar",
      "type": "string"
    }
    {
      "name": "baz",
      "type": ["null", "string"],
      "default": null
    }
]

I would like to be able to do this :

e = avro.NewFoo()

var b bytes.Buffer
writer := bufio.NewWriter(&b)
err = e.Serialize(writer)

but this fails with a

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa3e2b2]

So I have to manually remember to add in

e.baz = avro.NewUnionNullString()

So this causes my code to break each time I upgrade to a new avro schema, if I forget any of these

cameronbraid avatar May 02 '20 13:05 cameronbraid

Have you tried the newest major release? Unions with a null as the first type are now represented as nullable pointers, which should resolve this issue

actgardner avatar May 02 '20 14:05 actgardner

v7 solves that issue thanks :)

One thing I did notice that I thought was strange is that the enum value for '0' fo null type was removed

Used to be like

const (
	UnionNullStringTypeEnumNull UnionNullStringTypeEnum = 0
	UnionNullStringTypeEnumString UnionNullStringTypeEnum = 1
)

now it is

const (
	UnionNullStringTypeEnumString UnionNullStringTypeEnum = 1
)

So my code that did

func process(s UnionNullString) {
  if s.UnionType == avro.UnionNullStringTypeEnumNull {
    ...
  }
}

was broken, did this enum value need to be removed, since it is still possible to have a UnionNullString with a 0 UnionType

Also, it would be nice if there was a method

func NewUnionNullStringString(s string) *UnionNullString {
	return &UnionNullString{
		UnionType: UnionNullStringTypeEnumString,
		String: s,
	}
}

so that when you use it can create instances using literals

instead of

msg := &avro.Foo{
	Timestamp: 0,
	Guid: &avro.UnionNullString{
		String:    "test",
		UnionType: avro.UnionNullStringTypeEnumString,
	},
}

you can write

msg := &avro.Foo{
	Timestamp: 0,
	Guid: avro.NewUnionNullStringString("test"),
}

cameronbraid avatar May 04 '20 05:05 cameronbraid

Sorry for the slow reply.

since it is still possible to have a UnionNullString with a 0 UnionType

This actually isn't possible - we'll never decode this (it'll decode as a nil pointer) and if you try to encode it to Avro you'll get an error for an illegal value in the UnionType field. You should check if s == nil now. This change was made in a major release because it has breaking API changes that require you to update your code.

Also, it would be nice if there was a method

Providing some convenience methods for working with unions is a good suggestion, they're historically one of the most confusing parts of the library. I'll add this when I have time.

actgardner avatar May 10 '20 15:05 actgardner

As an aside. I recently upgraded to v7 and like the changes to map and union VERY MUCH !.
So thanks for that.

As a suggestion (which seems similar to the above), could they be cleaned up a little further when there are only 2 entries ? (i.e. it is just a nullable type).

Currently it still generates a type with an enum you have to set, when it isn't needed. i.e.

const (
	LastChangedFilterUnionTypeEnumLong LastChangedFilterUnionTypeEnum = 1
)

type LastChangedFilterUnion struct {
	Null      *types.NullVal
	Long      int64
	UnionType LastChangedFilterUnionTypeEnum
}

And in the refering type.

  LastChangedFilter *LastChangedFilterUnion `json:"lastChangedFilter"

Where I think this could just be

LastChangedFilter *long     `json:"lastChangedFilter"`

Would greatly simplify nullable types.

markfarnan avatar May 16 '20 11:05 markfarnan