[Go] Object API support

[iceboy233]

This PR adds object API support to golang. Related to #5072.

[aardappel]

Very cool! Code generally looks good to me. @rw can you review?

[aardappel]

Looks like you should run sh generate_code.sh again.

[rw]

@iceb0y Thanks for submitting this PR! The ergonomic improvement this could bring would be large.

Before I can merge this, I'd like to see comprehensive tests added to the test suite. Would you be able to do this? Perhaps we can take some ideas from the test suites of other language ports?

[iceboy233]

@rw The generate_code.sh already has good coverage about the changes here. Do you mean to actually test the generated code? In that case, the C++ object API is also not tested (search for MonsterT in this repo). I think we can merge this and keep this experimental for a while.

[aardappel]

@iceb0y maybe you don't find MonsterT because you underestimate the power of auto ? ;)

flatbuffers/tests/test.cpp

Lines 445 to 529 in b56d60f

	void ObjectFlatBuffersTest(uint8_t *flatbuf) {
	// Optional: we can specify resolver and rehasher functions to turn hashed
	// strings into object pointers and back, to implement remote references
	// and such.
	auto resolver = flatbuffers::resolver_function_t(
	[](void **pointer_adr, flatbuffers::hash_value_t hash) {
	(void)pointer_adr;
	(void)hash;
	// Don't actually do anything, leave variable null.
	});
	auto rehasher = flatbuffers::rehasher_function_t(
	[](void *pointer) -> flatbuffers::hash_value_t {
	(void)pointer;
	return 0;
	});
	// Turn a buffer into C++ objects.
	auto monster1 = UnPackMonster(flatbuf, &resolver);
	// Re-serialize the data.
	flatbuffers::FlatBufferBuilder fbb1;
	fbb1.Finish(CreateMonster(fbb1, monster1.get(), &rehasher),
	MonsterIdentifier());
	// Unpack again, and re-serialize again.
	auto monster2 = UnPackMonster(fbb1.GetBufferPointer(), &resolver);
	flatbuffers::FlatBufferBuilder fbb2;
	fbb2.Finish(CreateMonster(fbb2, monster2.get(), &rehasher),
	MonsterIdentifier());
	// Now we've gone full round-trip, the two buffers should match.
	auto len1 = fbb1.GetSize();
	auto len2 = fbb2.GetSize();
	TEST_EQ(len1, len2);
	TEST_EQ(memcmp(fbb1.GetBufferPointer(), fbb2.GetBufferPointer(), len1), 0);
	// Test it with the original buffer test to make sure all data survived.
	AccessFlatBufferTest(fbb2.GetBufferPointer(), len2, false);
	// Test accessing fields, similar to AccessFlatBufferTest above.
	TEST_EQ(monster2->hp, 80);
	TEST_EQ(monster2->mana, 150); // default
	TEST_EQ_STR(monster2->name.c_str(), "MyMonster");
	auto &pos = monster2->pos;
	TEST_NOTNULL(pos);
	TEST_EQ(pos->z(), 3);
	TEST_EQ(pos->test3().a(), 10);
	TEST_EQ(pos->test3().b(), 20);
	auto &inventory = monster2->inventory;
	TEST_EQ(inventory.size(), 10UL);
	unsigned char inv_data[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
	for (auto it = inventory.begin(); it != inventory.end(); ++it)
	TEST_EQ(*it, inv_data[it - inventory.begin()]);
	TEST_EQ(monster2->color, Color_Blue);
	auto monster3 = monster2->test.AsMonster();
	TEST_NOTNULL(monster3);
	TEST_EQ_STR(monster3->name.c_str(), "Fred");
	auto &vecofstrings = monster2->testarrayofstring;
	TEST_EQ(vecofstrings.size(), 4U);
	TEST_EQ_STR(vecofstrings[0].c_str(), "bob");
	TEST_EQ_STR(vecofstrings[1].c_str(), "fred");
	auto &vecofstrings2 = monster2->testarrayofstring2;
	TEST_EQ(vecofstrings2.size(), 2U);
	TEST_EQ_STR(vecofstrings2[0].c_str(), "jane");
	TEST_EQ_STR(vecofstrings2[1].c_str(), "mary");
	auto &vecoftables = monster2->testarrayoftables;
	TEST_EQ(vecoftables.size(), 3U);
	TEST_EQ_STR(vecoftables[0]->name.c_str(), "Barney");
	TEST_EQ(vecoftables[0]->hp, 1000);
	TEST_EQ_STR(vecoftables[1]->name.c_str(), "Fred");
	TEST_EQ_STR(vecoftables[2]->name.c_str(), "Wilma");
	auto &tests = monster2->test4;
	TEST_EQ(tests[0].a(), 10);
	TEST_EQ(tests[0].b(), 20);
	TEST_EQ(tests[1].a(), 30);
	TEST_EQ(tests[1].b(), 40);
	}

And actually I find further references to MonsterT in that file.

Yes, generated code should generally be tested. Writing something to the level of those C++ tests should not be that much work?

[aardappel]

Also keeping things "experimental" is hard, because there is no easy way for people to tell what code in this repo they can rely on, and what code is "use at your own risk" kind of deal.

If something "experimental" gets merged, and the original author stops maintaining it, users may show up with issues, and now @rw has to go in and fix them. I'd rather features go in with a reasonable level of confidence that they're solid, which means tests.

It doesn't have to be full featured, but it should be tested. I'd rather have partial functionality that is fully tested than full functionality that is partially tested :)

[iceboy233]

Looks like flatbuffers support unions with multiple aliases with the same type.

flatbuffers/tests/monster_test.fbs

Line 22 in 8d86b53

union AnyAmbiguousAliases { M1: Monster, M2: Monster, M3: Monster }

In this case, we cannot simply use interface{} as union type. Do you have ideas on the native types for unions?

[iceboy233]

Some alternatives:

type AnyAmbiguousAliasesT struct {
	M1 *MonsterT
	M2 *MonsterT
	M3 *MonsterT
}

type AnyAmbiguousAliasesT struct {
	Type AnyAmbiguousAliases
	Value interface{}
}

3. Inline 2 into the parent struct.

[aardappel]

Ah yes, that may be a problem. Also, union members can also be strings, can Interface handle those?

2 is what C++ does, not sure if it comes natural to Go, but it appears that it would work. It has the advantage that if there are no aliases, then the Go programmer can choose to ignore the type field?

Inlining sounds fine to me, given that go lacks "inline" structs, so this would be more efficient. The problem is of course with having 2 of the same union in one table, though that could be fixed by always pre-fixing with the field name.

@rw may know better.

[rw]

@iceb0y Personally, I like (1) the most. But, I think most of our users would want something closer to (2).

4. There is a fourth option, which would be to create an interface for this union type, implement it on the type, and use it as the type of the "interface value":

type AnyAmbiguousAliasesTValue interface {
        AnyAmbiguousAliasesTValue()
}

func (_ *MonsterT) AnyAmbiguousAliasesTValue() {}

type AnyAmbiguousAliasesT struct {
	Type AnyAmbiguousAliases
	Value AnyAmbiguousAliasesTValue
}

That gives us more type safety.

[aardappel]

I guess when it comes to the object API, design should be guided by what would give the most ergonomic typical Go code that is closest to hand-written.

[tigrato]

@aardappel can you check this PR now.

@googlebot I consent.

[aardappel]

How did we gain this import? why is it necessary when previously it wasn't

[tigrato]

The refactor for the Union Pack/UnPack methods requires access to flatbuffers.(Builder/Table) and so we need to import it

[aardappel]

The existing code already required access to these types?

[tigrato]

Yes

[aardappel]

So why didn't the existing code need this import?

[tigrato]

This code was in a different file. Since I have refactored the code into a different file it now requires the import in this file.

[aardappel]

Code looks good to me. See 2 small comments above, and also, some minor docs, please?
@rw can you give this a final look?

[llchan]

I'm helping a project experiment with swapping protobuf for flatbuffers and I'm happy to see this PR in the works already. The project's code uses the autogenerated protobuf structs as internal data containers, so having this object API will allow me to do a first pass conversion without reworking too much code.

Something to consider: having an API similar to the protobuf one would reduce friction for people trying to experiment with flatbuffers as a protobuf replacement. More concretely, could we add Marshal/Unmarshal functions that sit a step above than Pack/Unpack? I'd also suggest adding a UnpackTo func so that the user can control where the object lives. And I prefer Unpack over UnPack since it's actually one word.

Here's an off-the-cuff sketch of what the api could look like:

func (m *MonsterT) Marshal() ([]byte, error) {
  b := flatbuffers.NewBuilder(0)
  b.Finish(m.Pack(b)) 
  return b.FinishedBytes(), nil
}

func (m *MonsterT) Unmarshal(data []byte) error {
  GetRootAsMonster(data, 0).UnpackTo(m)
  return nil
}

func (m *MonsterT) Pack(b *flatbuffers.Builder) flatbuffers.UOffsetT {
  ...
}

func (m *Monster) UnpackTo(dst *MonsterT) {
  ...
}

func (m *Monster) Unpack() (*MonsterT) {
  dst := &MonsterT{}
  m.UnpackTo(dst)
  return dst
}

I can also hack on this but don't want to step on anyone's toes. The Marshal/Unmarshal stuff can maybe be a separate PR later to keep this PR moving, but we should consider the Unpack vs UnPack thing before merging.

Edit: oops, pack function above should be a regular func rather than a method, but i'm not sure why that's preferred. No strong preference from me on that, though the method is less verbose.

func MonsterPack(b *flatbuffers.Builder, t *MonsterT) flatbuffers.UOffsetT {
  ...
}

[llchan]

I finally had some time to look at this. Some amendments to my comment above:

• Regarding UnPack, I did not realize we already had this name from the C++ side of things (sorry, I've been away from flatbuffers dev for quite some time). Let's keep UnPack for consistency 👍
• It seems that the C++ side has the UnPackTo function that I coincidentally proposed here. I think that's a good thing to include in the go implementation so the user can control allocation if desired. I'm happy to update this PR with that, if there are no objections?
• I've got a separate PR ready to go once this one is merged in, that adds a opt-in go-protobuf-compat mode that allows the object API here to act as a drop-in replacement for simple use cases.

[aardappel]

@llchan we may want to try and get this PR merged if you think its otherwise reasonable, and do some changes in a followup, since its been hanging for a while now..

@iceb0y there are Go CI errors that can easily be fixed by running sh generate_code.sh.

[llchan]

Yep, LGTM. The UnPackTo will leave the UnPack interface unchanged so it can sit in a separate PR. I have those changes ready as well.

[aardappel]

@iceb0y can you fix the CI like I said above, rebase, and then we can merge?

[llchan]

Nit: spurious whitespace change.

[llchan]

To be more precise, need to rebase before regenerating. I think the CI tests are merging with master before testing, which is introducing new fields to the test fbs.

[llchan]

Btw @iceb0y if you want me to take over the PR I'm happy to do the rebase/squash and open a new one. Will of course credit you (and @tigrato) with the majority of the legwork.

[iceboy233]

Done.

[llchan]

@tigrato I think you may need to consent in a standalone message

[tigrato]

@googlebot I consent.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[tigrato]

done

[llchan]

@aardappel @rw just a friendly ping. Whenever this gets merged I'll push up the UnPackTo change + rebase my other PR.

[aardappel]

Awesome, thanks for the work everyone!

[tsingson]

thanks a lot!!!
wait for this so long..............

thanks all again!!