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

storage.Put ambiguity #3381

Open
roman-khimov opened this issue Mar 29, 2024 · 3 comments
Open

storage.Put ambiguity #3381

roman-khimov opened this issue Mar 29, 2024 · 3 comments
Labels
enhancement Improving existing functionality I3 Minimal impact S3 Minimally significant U4 Nothing urgent

Comments

@roman-khimov
Copy link
Member

Is your feature request related to a problem? Please describe.

I'm always frustrated when people try to use storage.Put for structs/arrays/map and it fails in a very unpredictable way. It's documented to only work with simple types, but we better prevent this then explain again and again.

Describe the solution you'd like

Have some lower layer thin wrapper for System.Storage.Put that works with []byte and then have a convenience wrapper that utilizes serialization interop for transparent conversion.

The problem is, we need backwards compatibility for existing code.

Describe alternatives you've considered

Leaving as is. Well, it works. But it's really unobvious.

@roman-khimov roman-khimov added I3 Minimal impact U4 Nothing urgent enhancement Improving existing functionality S3 Minimally significant labels Mar 29, 2024
@lock9
Copy link

lock9 commented Apr 1, 2024

Hello,

I saw your comment on #3372. Do you mean merging PutInt and GetInt into a single issue? I'm not sure if I understand it.

This feature is critical from a user experience point of view. The storage layer's lack of types forces a lot of casting, often leading to runtime errors. All SDKs should include GetInt/PutInt and variations.

@fyfyrchik
Copy link
Contributor

fyfyrchik commented Apr 22, 2024

We already have similar implicit conversion code emitted for len and append to ensure len(nil) == 0

What about using safe typecast for this, like x, ok := storage.Get().(int)?
In the example from #3372 the second return value will be false (and can be ignored). The first one will contain the default value, which plays nicely with Go semantics and idiomatic map code (one can argue that storage is a map). The only question is whether it is desireable to not panic when we convert array to an integer. The drawback is that in this solution ok is overloaded -- it checks for both type and presence of a key, so not very clean.

And because this is done by the compiler (interop still returns 1 value) using _ in place of the second variable can be optimized away by compiler.

@roman-khimov
Copy link
Member Author

conversion code emitted for len and append

Is very much different to me. They just follow regular Go semantics and do ISNULL. Here we CONVERT and if we're to use proper serialization/deserialization then we'd have a different binary value.

This can be solved with a new API around Context, Context.Put/Context.Get and alike. Then deprecate storage.Put/storage.Get and migrate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving existing functionality I3 Minimal impact S3 Minimally significant U4 Nothing urgent
Projects
None yet
Development

No branches or pull requests

3 participants