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] Consider using bstr for deserialized strings #110

Open
TethysSvensson opened this issue Jul 6, 2022 · 6 comments
Open

[Rust] Consider using bstr for deserialized strings #110

TethysSvensson opened this issue Jul 6, 2022 · 6 comments

Comments

@TethysSvensson
Copy link
Collaborator

We should consider using bstr for strings in the deserializer once it reaches 1.0.

@TethysSvensson TethysSvensson added enhancement New feature or request breaking-change labels Jul 8, 2022
@TethysSvensson
Copy link
Collaborator Author

My current plan:

  • Create a feature flag bstr in the planus crate.
  • Add support in the planus create for deserializing into bstr hidden behind this feature flag.
  • Create a #[doc(hidden)] type-alias in the planus crate which is either str or BStr depending on the feature flag.
  • Change the codegen so the deserialized type uses this type-alias instead of hardcoding str. This means that the same generated code will work both with and without the bstr feature.

Open questions:

  • Should the bstr feature be turned on by default? If yes, then it would technically be a breaking change, though a fairly small one. Should we consider the change large enough for a SemVer version bump?
  • Should we also add support for serializing bstr and BString into Offset<str>? The downside is that this would allow us to create invalid flatbuffer data by misusing the APIs, but perhaps that's an okay trade-off in this case?
  • If yes, should we also use a type-alias trick to put BStrings in the generated code for owned tables or unions?

@kristoff3r @jorgecarleitao Do you have any opinions on this?

@jorgecarleitao
Copy link
Contributor

In general I am in favor.

My usual questions on this are:

  • how does dependencies change with this (i.e. more dependencies?)
  • how does it affect performance?
  • how does it affect safety / security?

I think it is fine to bump version - do what you feel to make this crate better, even if that requires breaking the public API.

@TethysSvensson
Copy link
Collaborator Author

* how does dependencies change with this (i.e. more dependencies?)

Yes, but only indirectly. You will not need to change your Cargo.toml.

* how does it affect performance?

It improves it. It will make it optional to pay for utf-8 validation.

* how does it affect safety / security?

It does not affect it directly. If we allow encoding BStr as Offset<str>, then we make it easier to create non-compliant data. In some code-bases this might be a security issue, but it consider it fairly high up on the unlikeliness scale.

I think it is fine to bump version - do what you feel to make this crate better, even if that requires breaking the public API.

👍

@kristoff3r
Copy link
Collaborator

Parsing strings as bstr seems like a good idea, writing not as much. In that case you could also just use unsafe to skip the validation? That seems like a better signal to send.

I personally think it should be optional, but I don't have a strong opinion.

@TethysSvensson
Copy link
Collaborator Author

@kristoff3r I think I feel the same way. If you want to re-export a string, you will have to check if it's valid utf-8.

@TethysSvensson
Copy link
Collaborator Author

Related: #8

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

Successfully merging a pull request may close this issue.

3 participants