Null handling in unwrap library

[roman-khimov]

Another issue I'm thinking about is that array of something can be naturally substituted by Null stackitem and our unwrappers won't accept it. Maybe it's worth to add Null support to unwrap package for arrays?

Originally posted by @AnnaShaleva in #2792 (review)

It's true that we can have Null in many cases. We can have nil in Go at the same time. Maybe it's OK to return it.

[AnnaShaleva]

Ping to this issue, because NNS wrapper (#3291) uses getRecord NNS method that returns Null stackitem in case of missing record of the specified type. And neither unwrap.UTF8String nor unwrap.Bytes can handle Null stackitem. As a result in the described case we've got an error from the NNS wrapper side on attempt to unwrap Null into Go's string while NNS itself doesn't return any error.

IMO, this behaviour of NNS wrapper is not correct and it will be fixed by this issue. We likely should return an empty string and no error form NNS wrapper in the described case. So something like the following will be useful, but we need to evaluate the usages of unwrap.Bytes and similar methods. Maybe we can directly modify unwrap.Bytes and etc. to properly handle Null.

+// Bytes expects correct execution (HALT state) with a single stack item
+// returned. A slice of bytes is extracted from this item and returned.
+func BytesOrNull(r *result.Invoke, err error) ([]byte, error) {
+       itm, err := Item(r, err)
+       if err != nil {
+               return nil, err
+       }
+       if itm.Equals(stackitem.Null{}) {
+               return nil, nil
+       }
+       return itm.TryBytes()
+}
+
 
+// UTF8String expects correct execution (HALT state) with a single stack item
+// returned. A string is extracted from this item and checked for UTF-8
+// correctness, valid strings are then returned.
+func UTF8StringOrNull(r *result.Invoke, err error) (string, error) {
+       b, err := BytesOrNull(r, err)
+       if err != nil {
+               return "", err
+       }
+       if b == nil {
+               return "", nil
+       }
+       if !utf8.Valid(b) {
+               return "", errors.New("not a UTF-8 string")
+       }
+       return string(b), nil
+}
+

@roman-khimov, what do you think?

@AliceInHunterland, this issue is related to error returned from recordData, err := nnsReader.GetRecord(domainName, recordType)

[AnnaShaleva]

I'd propose to include this issue in 0.106.0 milestone.

[roman-khimov]

It's pretty straightforward for unwrap.Bytes (or arrays), but UTF8String is different.

empty string and no error

The problem is, it's not nil. And you can't differentiate NULL and real empty string in this case.

Try to check for more examples of how these APIs are used currently.

[AnnaShaleva]

The problem is, it's not nil. And you can't differentiate NULL and real empty string in this case.

You're right. Then maybe we need to change the NNS wraper itself to return (*string, error)? It doesn't look good to me either, but returning an error also seems not very correct to me.

[roman-khimov]

It's one of:

• special method with special behavior (additional API to choose from)
• special error (keeping the API as is)
• *string (changing the API)

None of them is perfect. But which one to choose depends on typical usage patterns.

[cthulhu-rider]

still gives me goosebumps, very waiting for the release!

didn't see this thread right away, so shared some my opinion in nspcc-dev/neofs-contract#415

[roman-khimov]

Voting for a special ErrNull here:

• compatible with the current behavior
• works for all types in the same way
• whether Null is a problem or not is entirely contract-specific and depends on what contract returns
• in case contract never returns Null it's not a problem already and current behavior is more defensive
• in case contract returns Null it'd require additional handling, but mostly it'd be if err != nil && !errors.Is(err, unwrap.ErrNull)
• returning nil, nil is not common in general
• handling nil, nil is easy for slices of strings and similar things where you iterate over the resulting list, but not so for *string and alike, it'd require about the same amount of code as the snippet above for ErrNull or more

Other opinions and suggestions are welcome.