nspcc-dev/neofs-sdk-go

Fork 0

mirror of https://github.com/nspcc-dev/neofs-sdk-go.git synced 2026-03-01 04:29:18 +00:00

Checking for the fields' presence should not be done by unmarshalers #236

New issue

Open

opened 2025-12-28 18:07:31 +00:00 by sami · 5 comments

sami commented

2025-12-28 18:07:31 +00:00

Owner

Originally created by @cthulhu-rider on GitHub (Nov 19, 2024).

most of the lib data structures provide Unmarshal/UnmarshalJSON interface. Implementations do not check for presence of the fields required by the protocol (example). However, since structure is fully decoded and then verified (as in the example), checking of complex fields requires calling ReadFromV2 (nspcc-dev/neofs-sdk-go@2da6182d23/bearer/bearer.go (L48)) which expects all required fields to be set

this makes it impossible to set partially filled complex sub-field, encode the whole message, and then decode without an error. Although such a process does not seem to be in demand, in general it can cause a problem. Let's say in tests at the moment

Steps to Reproduce

func TestContainerPolicyUnmarshal(t *testing.T) {
	var policy netmap.PlacementPolicy
	var cnr container.Container
	cnr.SetPlacementPolicy(policy)

	b := cnr.Marshal()

	var cnr2 container.Container
	require.NoError(t, cnr2.Unmarshal(b))
	require.Zero(t, cnr2.PlacementPolicy())
}

Current Behavior

=== RUN   TestContainerPolicyUnmarshal
    container_test.go:1017: 
        	Error Trace:	/home/ll/projects/neo/sdk/container/container_test.go:1017
        	Error:      	Received unexpected error:
        	            	invalid placement policy: missing replicas
        	Test:       	TestContainerPolicyUnmarshal
--- FAIL: TestContainerPolicyUnmarshal (0.00s)

Expected Behavior

passed test

being a user, i expect to set the instance in any form (*), encode it and successfully decode it. I also expect to be able to check any field for presence

(*) mostly without specifying required fields. I still expect decoder to make sanity checks for some type of fields. For example, fixed-len IDs must have the exact size: this is just cuz project type system is stronger than Protocol Buffers. If protobuf would have fixed-len array type, we would use it and decoding would fail on the protolib stage

Possible Solution

i see 2 ones:

provide additional method ReadOptionalFromV2 which will check specified fields only (e.g. that IDs have exact bytelen) and use it instead of ReadFromV2
decode the message field-by-field using unmarshalers. This would fasten catching errors overall: if 1st field is invalid, no need to decode the rest. At the same time, implementation will definitely become more complicated, especially for JSON, so i dont actually consider this approach and left it JFYI

Context

https://github.com/nspcc-dev/neofs-node/pull/3005#issuecomment-2483371807

Regression

not really. For example, this became actual for Object after https://github.com/nspcc-dev/neofs-sdk-go/pull/634/commits/3c47093ae4c28b62afba3e3f34bb270d39cf3137, but there were no checks at all before

Originally created by @cthulhu-rider on GitHub (Nov 19, 2024). most of the lib data structures provide `Unmarshal`/`UnmarshalJSON` interface. Implementations do not check for presence of the fields required by the protocol ([example](https://github.com/nspcc-dev/neofs-sdk-go/blob/2da6182d23f5677889219e0aaa84930e487ef363/bearer/bearer.go#L376)). However, since structure is fully decoded and then verified (as in the example), checking of complex fields requires calling `ReadFromV2` (https://github.com/nspcc-dev/neofs-sdk-go/blob/2da6182d23f5677889219e0aaa84930e487ef363/bearer/bearer.go#L48) which expects all required fields to be set this makes it impossible to set partially filled complex sub-field, encode the whole message, and then decode without an error. Although such a process does not seem to be in demand, in general it can cause a problem. Let's say in tests at the moment ## Steps to Reproduce ```go func TestContainerPolicyUnmarshal(t *testing.T) { var policy netmap.PlacementPolicy var cnr container.Container cnr.SetPlacementPolicy(policy) b := cnr.Marshal() var cnr2 container.Container require.NoError(t, cnr2.Unmarshal(b)) require.Zero(t, cnr2.PlacementPolicy()) } ``` ## Current Behavior ``` === RUN TestContainerPolicyUnmarshal container_test.go:1017: Error Trace: /home/ll/projects/neo/sdk/container/container_test.go:1017 Error: Received unexpected error: invalid placement policy: missing replicas Test: TestContainerPolicyUnmarshal --- FAIL: TestContainerPolicyUnmarshal (0.00s) ``` ## Expected Behavior passed test being a user, i expect to set the instance in any form (*), encode it and successfully decode it. I also expect to be able to check any field for presence (*) mostly without specifying required fields. I still expect decoder to make sanity checks for some type of fields. For example, fixed-len IDs must have the exact size: this is just cuz project type system is stronger than Protocol Buffers. If protobuf would have fixed-len array type, we would use it and decoding would fail on the protolib stage ## Possible Solution i see 2 ones: 1. provide additional method `ReadOptionalFromV2` which will check specified fields only (e.g. that IDs have exact bytelen) and use it instead of `ReadFromV2` 2. decode the message field-by-field using unmarshalers. This would fasten catching errors overall: if 1st field is invalid, no need to decode the rest. At the same time, implementation will definitely become more complicated, especially for JSON, so i dont actually consider this approach and left it JFYI ## Context https://github.com/nspcc-dev/neofs-node/pull/3005#issuecomment-2483371807 ## Regression not really. For example, this became actual for `Object` after https://github.com/nspcc-dev/neofs-sdk-go/pull/634/commits/3c47093ae4c28b62afba3e3f34bb270d39cf3137, but there were no checks at all before

sami added the

bug

label

2025-12-28 18:07:31 +00:00

sami commented

2025-12-28 18:07:31 +00:00

Author

Owner

@roman-khimov commented on GitHub (Nov 19, 2024):

Who cares about these problems except for obscure tests? Current behavior is more suitable for generic users, so it should stay. Tests can find some ways to break things when they need to.

@roman-khimov commented on GitHub (Nov 19, 2024): Who cares about these problems except for obscure tests? Current behavior is more suitable for generic users, so it should stay. Tests can find some ways to break things when they need to.

sami commented

2025-12-28 18:07:31 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Nov 19, 2024):

any user passing data back and forth i'd say. The main goal of this issue is to highlight current that at the moment the behavior is not strictly fixed, and can sometimes change in one direction or another. I suggest to choose and fix the one

for example, https://github.com/nspcc-dev/neofs-node/pull/3005#issuecomment-2483371807 (p.3) encountered a regression when tested object has session token w/o an issuer (required by the protocol). In the latest lib version, object cannot be received due to recently added invalid header: invalid session token: missing session issuer decoding error. Previously, there was no such error

so, protocol requirement to specify the session issuer now also affects binary objects stored by nodes:

this check is missing at the upload stage. If the error is legit, it must be thrown before attempt to access the object
theoretically, some previously objects could become inaccessible because of this. This follows from the previous fact

in total, smth should be done anyway:

decoding requirements must be weakened (back) as proposed here or
objects must be pre-checked before encoding

im leaning towards the 1 as its more flexible for the general-purpose library

@cthulhu-rider commented on GitHub (Nov 19, 2024): any user passing data back and forth i'd say. The main goal of this issue is to highlight current that at the moment the behavior is not strictly fixed, and can sometimes change in one direction or another. I suggest to choose and fix the one --- for example, https://github.com/nspcc-dev/neofs-node/pull/3005#issuecomment-2483371807 (p.3) encountered a regression when tested object has session token w/o an issuer (required by the protocol). In the latest lib version, object cannot be received due to recently added `invalid header: invalid session token: missing session issuer` decoding error. Previously, there was no such error so, protocol requirement to specify the session issuer now also affects binary objects stored by nodes: 1. this check is missing at the upload stage. If the error is legit, it must be thrown before attempt to access the object 2. theoretically, some previously objects could become inaccessible because of this. This follows from the previous fact in total, smth should be done anyway: 1. decoding requirements must be weakened (back) as proposed here or 2. objects must be pre-checked before encoding im leaning towards the 1 as its more flexible for the general-purpose library

sami commented

2025-12-28 18:07:32 +00:00

Author

Owner

@roman-khimov commented on GitHub (Nov 20, 2024):

My expectation is that no real objects have these problems, so we can catch everything needed at decoding level and have an always nice object structures. Creating an object that violates basic rules must be hard enough for most checks to be not needed at the encoding stage at the same time.

@roman-khimov commented on GitHub (Nov 20, 2024): My expectation is that no real objects have these problems, so we can catch everything needed at decoding level and have an always nice object structures. Creating an object that violates basic rules must be hard enough for most checks to be not needed at the encoding stage at the same time.

sami commented

2025-12-28 18:07:32 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Nov 25, 2024):

i think all requirements imposed on the object and returned by Unmarshal should be transparent and checked before storing the object. This can be covered by SDK docs and changes in the storage

again, if Put returns no error, i dont expect an error from Get. Original issue's context is exactly about this

@cthulhu-rider commented on GitHub (Nov 25, 2024): i think all requirements imposed on the object and returned by `Unmarshal` should be transparent and checked before storing the object. This can be covered by SDK docs and changes in the storage again, if `Put` returns no error, i dont expect an error from `Get`. Original issue's context is exactly about this

sami commented

2025-12-28 18:07:32 +00:00

Author

Owner

@roman-khimov commented on GitHub (Nov 25, 2024):

That's a correct expectation. And we have two sides of it:

remote node receiving some complete object and storing it (it'll fail to decode and refuse anyway)
local node creating an object and pushing it local store (plus remote ones)

The latter is different in that if some node creates and stores a bogus object it's this node's problem. It shouldn't do this.

@roman-khimov commented on GitHub (Nov 25, 2024): That's a correct expectation. And we have two sides of it: * remote node receiving some complete object and storing it (it'll fail to decode and refuse anyway) * local node creating an object and pushing it local store (plus remote ones) The latter is different in that if some node creates and stores a bogus object it's this node's problem. It shouldn't do this.