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

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

Open
cameronbraid opened this issue May 2, 2020 · 4 comments

Comments

@cameronbraid
Copy link

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 cameronbraid changed the title make generated 'New<Type>' functions fill in defaults for UnionNull<X> types make generated 'New<Type>' functions fill in defaults for UnionNull<X> fields May 2, 2020
@actgardner
Copy link
Owner

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

@cameronbraid
Copy link
Author

cameronbraid commented May 4, 2020

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"),
}

@actgardner
Copy link
Owner

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.

@markfarnan
Copy link

markfarnan commented May 16, 2020

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.

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

No branches or pull requests

3 participants