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

Parsing FlatBuffers with Python in 22.9.24 is broken #7569

Closed
stewartmiles opened this issue Oct 6, 2022 · 7 comments
Closed

Parsing FlatBuffers with Python in 22.9.24 is broken #7569

stewartmiles opened this issue Oct 6, 2022 · 7 comments

Comments

@stewartmiles
Copy link
Contributor

stewartmiles commented Oct 6, 2022

Python's code generator was broken by #7529 (FYI @joshua-smith8).

Given the table Foo, when generating code for the object API FooT now skips flatbuffer file headers.

For example, before pull/7529, tests/monster_test_generated.py:

@classmethod
def InitFromBuf(cls, buf, pos):
monster = Monster()
monster.Init(buf, pos)
return cls.InitFromObj(monster)

After:

@classmethod
def InitFromBuf(cls, buf, pos):
n = flatbuffers.encode.Get(flatbuffers.packer.uoffset, buf, 0)
monster = Monster()
monster.Init(buf, pos+n)
return cls.InitFromObj(monster)

This is cool as FooT.InitFromBuf() interprets the buf argument in the same way as Foo.GetRootAs(), i.e it assumes buf points at the start of a packed flatbuffer file and skips the header. However, this has some issues as:

  • It breaks the existing users of the object API who were skipping the header themselves. (I doubt there were many, e.g we were doing foo = FooT.InitFromObj(Foo.GetRootAs(buf)).
  • InitFromObj is used to parse nested union tables which is broken as they end up skipping the header block:

def AnyCreator(unionType, table):
from flatbuffers.table import Table
if not isinstance(table, Table):
return None
if unionType == Any().Monster:
return MonsterT.InitFromBuf(table.Bytes, table.Pos)
if unionType == Any().TestSimpleTableWithEnum:
return TestSimpleTableWithEnumT.InitFromBuf(table.Bytes, table.Pos)
if unionType == Any().MyGame_Example2_Monster:
return MonsterT.InitFromBuf(table.Bytes, table.Pos)
return None

Since FooT.InitFromBuf() is now better? since it aligns with GetRootAs() perhaps the solution is to extend InitFromBuf() to support parsing nested tables?

@stewartmiles
Copy link
Contributor Author

Oh also, smells like the tests for nested tables needs to be fixed to deal catch this case.

@stewartmiles
Copy link
Contributor Author

Hey folks, 22.9.24 is still the latest on pypi and it's broken. Any chance someone (@dbaileychess @rw @aardappel) could build and publish a release?

dbaileychess pushed a commit that referenced this issue Oct 26, 2022
…7576)

* feat: Fixed the issue with nested unions relying on InitFromBuf.
Problem: Issue #7569
Nested Unions were broken with the introduction of parsing buffers with an initial encoding offset.

Fix:
Revert the InitFromBuf method to the previous version and introduction of InitFromPackedBuf that allows
users to read types from packed buffers applying the offset automatically.

Test:
Added in TestNestedUnionTables to test the encoding and decoding ability using a nested table with a
union field.

* fix: Uncommented generate code command
@dbaileychess
Copy link
Collaborator

@stewartmiles Sorry for the delay, I just merged the fix.

I am trying to automate publishing to our package managers (including Pypi) on releases, do you mind if I take a bit to set this up before releasing again with this fix in?

@stewartmiles
Copy link
Contributor Author

@dbaileychess no worries, we're all good we just pinned to the previous release of FlatBuffers. I'm primarily concerned about newcomers to the project who will pull the latest from pypi and have a broken experience. Is it possible to publish the previous version as a new version manually to at least unbreak head on pypi?

@stewartmiles
Copy link
Contributor Author

If it's a pain, I get it .. just folks will likely have a bad experience and not join the cult of "Flat" :)

@dbaileychess
Copy link
Collaborator

Should be up now: https://pypi.org/project/flatbuffers/22.10.26/

@stewartmiles
Copy link
Contributor Author

Sweet thanks @dbaileychess 😄

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

2 participants