avro icon indicating copy to clipboard operation
avro copied to clipboard

feat: added field.name on MapEncoder errors

Open nascimento opened this issue 3 years ago • 5 comments

Proposal:

Show field name on errors from MapEncoder.

nascimento avatar Aug 01 '22 14:08 nascimento

Good catch, I had obviously missed the error here.

The way you implemented the error however is different from the struct case. I also see I am missing the error on the map decode case. From strictly a performance PoV, allowing the encode to continue is worse than failing fast, but I could probably argue both sides of this. Either way it should probably be consistent across all 4 cases here.

nrwiersma avatar Aug 01 '22 14:08 nrwiersma

yes, I prefer when fail fast too but I like when all erros is returned :D. Showing all errors in generic avro/data is good to quick fix without need to try/retry many times ... here I am using this lib as a Sidecar, for example.

I can change code to work as fail fast, no problem ... what u think?

I also see I am missing the error on the map decode case.

And using generic with many complex types field on avro schema (like array > object > array) ... the function Encode of Map are starting with error from another method (I don't understood this part yet).

panic: []interface {}: []interface {}: []interface {}: []interface {}: []interface {}: []interface {}: avro: float64 is unsupported for Avro long
schema, err := avro.Parse(`{"type":"record","name":"Superhero","namespace":"com.model","fields":[{"name":"id","type":"long"},{"name":"affiliation_id","type":"long"},{"name":"name","type":"string"},{"name":"life","type":"long"},{"name":"energy","type":"double"},{"name":"arrayField","type":{"type":"array","items":["double","string"]}},{"name":"powers","type":{"type":"array","items":{"type":"record","name":"Superpower","fields":[{"name":"id","type":"long"},{"name":"name","type":"string"},{"name":"damage","type":"long"},{"name":"energy","type":"double"},{"name":"passive","type":"boolean"}]}}},{"name":"powers2","type":{"type":"array","items":{"type":"record","name":"Superpower","fields":[{"name":"id","type":"long"},{"name":"name","type":"string"},{"name":"damage","type":"long"},{"name":"energy","type":"double"},{"name":"powers3","type":{"type":"array","items":{"type":"record","name":"Superpower","fields":[{"name":"id","type":"long"},{"name":"name","type":"string"},{"name":"damage","type":"long"},{"name":"energy","type":"double"},{"name":"passive","type":"boolean"}]}}}]}}},{"name":"Status","type":{"type":"enum","name":"Status","symbols":["APROVADO","CANCELADO"]}},{"name":"address","type":{"type":"record","name":"mailing_address","fields":[{"name":"street","type":"string","default":"NONE"},{"name":"city","type":"string","default":"NONE"}]}},{"name":"Document","type":{"type":"array","items":{"name":"Name","type":"record","fields":[{"name":"Language","type":{"type":"array","items":{"name":"Language","type":"record","fields":[{"name":"Country","type":["null","string"]}]}}}]}}}]}`)
	if err != nil {
		panic(err)
	}

	var avroInterface map[string]interface{}
	err = json.Unmarshal([]byte(`{"testesasda":"asdasd","Status":"APROVADO","name":"Wolverine","affiliation_id":1,"energy":32.75,"id":234765,"life":82,"arrayField":[11.1,23.1,"asdasd"],"address":{"street":"Rua dos Bobôs","city":"São Paulo"},"Document":[{"Language":[{"Country":"Brazil"},{"Country":"UnitedStates"}]}],"powers":[{"damage":5,"energy":1.15,"id":2345,"name":"Bone Claws","passive":false},{"damage":-2,"energy":0.55,"id":2346,"name":"Regeneration","passive":true},{"damage":-10,"energy":0,"id":2347,"name":"Adamant skeleton","passive":true}],"powers2":[{"damage":5,"energy":1.15,"id":2345,"name":"Bone Claws","passive":false,"powers3":[{"damage":5,"energy":1.15,"id":2345,"name":"Bone Claws","passive":false},{"damage":-2,"energy":0.55,"id":2346,"name":"Regeneration","passive":true},{"damage":-10,"energy":0,"id":2347,"name":"Adamant skeleton","passive":true}]}]}`), &avroInterface)
	if err != nil {
		panic(err)
	}

_, err := avro.Marshal(schema, avroInterface)
if err != nil {
panic(err)
}

nascimento avatar Aug 01 '22 15:08 nascimento

And using generic with many complex types field on avro schema (like array > object > array) ... the function Encode of Map are starting with error from another method (I don't understood this part yet).

This is from the arrayEncoder.

I totally understand the "return all errors" PoV, but in this situation I think the fail fast is better IMO.

nrwiersma avatar Aug 01 '22 15:08 nrwiersma

@nrwiersma what do you think about break/return all loop fields (map, struct, array, enum...) to fail fast?

nascimento avatar Aug 01 '22 19:08 nascimento

I think any looped decoder should return to fail fast. Decoders like enum are only doing a single value, so it should not be needed here. In general, good idea 👍🏻

nrwiersma avatar Aug 01 '22 20:08 nrwiersma

Superseded by #204

nrwiersma avatar Nov 21 '22 05:11 nrwiersma