`CopyTo` works incorrectly in some cases #263

New issue

Open

opened 2025-12-28 18:07:36 +00:00 by sami · 1 comment

sami commented

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

Owner

Originally created by @cthulhu-rider on GitHub (Dec 12, 2024).

CopyTo is provided by several complex struct types. It allows to deep-copy instance for safety

Current Behavior

Container.CopyTo keeps dst attributes when src has none. If dst already had some attributes, resulting copy is incorrect

this could be missed in 2 reasons:

CopyTo is rarely used
dst is usually zero: np in this case

Expected Behavior

correct deep copy

Possible Solution

just do it

Steps to Reproduce

func TestCopyReset(t *testing.T) {
	var src, dst Container
	dst.SetAttribute("key", "val")
	src.CopyTo(&dst)
	require.Zero(t, dst.Attribute("key"))
}

Context

maybe there are other cases, review all of them. To force this, i suggest to pre-add corresponding unit tests

Regression

no (at least for noticed case)

Your Environment

Version of the product used: 0b9bb748ea

Originally created by @cthulhu-rider on GitHub (Dec 12, 2024). `CopyTo` is provided by several complex struct types. It allows to deep-copy instance for safety ## Current Behavior [`Container.CopyTo`](https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go/container#Container.CopyTo) keeps `dst` attributes when `src` has none. If `dst` already had some attributes, resulting copy is incorrect this could be missed in 2 reasons: - `CopyTo` is rarely used - `dst` is usually zero: np in this case ## Expected Behavior correct deep copy ## Possible Solution just do it ## Steps to Reproduce ```go func TestCopyReset(t *testing.T) { var src, dst Container dst.SetAttribute("key", "val") src.CopyTo(&dst) require.Zero(t, dst.Attribute("key")) } ``` ## Context maybe there are other cases, review all of them. To force this, i suggest to pre-add corresponding unit tests ## Regression no (at least for noticed case) ## Your Environment * Version of the product used: 0b9bb748ea16370c9b88090a902413324e86bcf1

sami added the

bug

labels

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

sami commented

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

Author

Owner

@cthulhu-rider commented on GitHub (Dec 12, 2024):

session.Container/Object have similar error in Unmarshal(JSON: context fields aint reset

func TestResetContext(t *testing.T) {
	var c session.Container
	c.ForVerb(1)
	require.True(t, c.AssertVerb(1))
	
	require.NoError(t, c.Unmarshal(nil))
	require.False(t, c.AssertVerb(1))
}

in total, i suggest to add unit tests for resetting a previously set value for the following methods:

CopyTo
ReadFromV2 (future FromProtoMessage)
Unmarshal
UnmarshalJSON

in particular cases, this can be already covered. Example

once all tests are done, some cases incl. mentioned here will FAIL. And then fix

@cthulhu-rider commented on GitHub (Dec 12, 2024): `session.Container/Object` have similar error in `Unmarshal(JSON`: context fields aint reset ```go func TestResetContext(t *testing.T) { var c session.Container c.ForVerb(1) require.True(t, c.AssertVerb(1)) require.NoError(t, c.Unmarshal(nil)) require.False(t, c.AssertVerb(1)) } ``` --- in total, i suggest to add unit tests for resetting a previously set value for the following methods: 1. `CopyTo` 2. `ReadFromV2` (future `FromProtoMessage`) 3. `Unmarshal` 4. `UnmarshalJSON` in particular cases, this can be already covered. [Example](https://github.com/nspcc-dev/neofs-sdk-go/blob/0b9bb748ea16370c9b88090a902413324e86bcf1/container/container_test.go#L486-L492) once all tests are done, some cases incl. mentioned here will FAIL. And then fix