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

[Rust] Restore public visibility of previously-public fields #7700

Merged
merged 3 commits into from Dec 8, 2022

Conversation

maxburke
Copy link
Contributor

@maxburke maxburke commented Dec 6, 2022

We relied on these two fields being public in the Table interface and this change restores their visibility.

@enum-class
Copy link
Contributor

I think they became invisible in this P.R. #7518

@maxburke
Copy link
Contributor Author

maxburke commented Dec 7, 2022

Yeah, that looks right. Unfortunately we were using these public fields.

@CasperN
Copy link
Collaborator

CasperN commented Dec 8, 2022

Can you talk more about your use case? maybe we can provide getter/setter methods - it will likely have to be unsafe though

@maxburke
Copy link
Contributor Author

maxburke commented Dec 8, 2022

Why would it have to be unsafe?

We compute hashes of some classes of our flatbuffer serialized objects. Our objects also include modification timestamps. But we also wish to ignore the timestamp when calculating the integrity hash, we force the modification field to zero in the object before hashing it.

@CasperN
Copy link
Collaborator

CasperN commented Dec 8, 2022

Why would it have to be unsafe?

Because tables are assumed to be verified on construction, arbitrary editing can invalidate that (e.g. by modifying a relative pointer)

@CasperN
Copy link
Collaborator

CasperN commented Dec 8, 2022

We compute hashes of some classes of our flatbuffer serialized objects. Our objects also include modification timestamps. But we also wish to ignore the timestamp when calculating the integrity hash, we force the modification field to zero in the object before hashing it.

Ideally, we'd expose a safe mutation API (assuming the timestamp is in the table schema), but absent this you'd need to modify the slice yourself and that has to be unsafe given the pre-verified assumption

@maxburke
Copy link
Contributor Author

maxburke commented Dec 8, 2022

I've changed the PR to expose getters for the buf and loc fields; I don't think we need setters.

@CasperN CasperN enabled auto-merge (squash) December 8, 2022 05:42
@CasperN CasperN disabled auto-merge December 8, 2022 05:43
@CasperN CasperN merged commit 3be296e into google:master Dec 8, 2022
sunwen18 pushed a commit to sunwen18/flatbuffers that referenced this pull request Dec 25, 2022
…7700)

* Restore public visibility of previously-public fields

* code review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants