Org-wide linter #2943

End-rey · 2024-09-19T17:06:21Z

Closes #2940.

codecov · 2024-09-19T17:09:18Z

Codecov Report

Attention: Patch coverage is 28.96552% with 206 lines in your changes missing coverage. Please review.

Project coverage is 23.47%. Comparing base (7365c7c) to head (3ebb2d8).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/object/util/chain.go	0.00%	18 Missing ⚠️
cmd/neofs-cli/modules/storagegroup/put.go	0.00%	16 Missing ⚠️
cmd/neofs-cli/modules/object/head.go	0.00%	11 Missing ⚠️
pkg/core/object/fmt.go	40.00%	6 Missing and 3 partials ⚠️
cmd/neofs-lens/internal/printers.go	0.00%	8 Missing ⚠️
cmd/neofs-cli/modules/storagegroup/get.go	0.00%	7 Missing ⚠️
cmd/neofs-cli/modules/util/acl.go	0.00%	7 Missing ⚠️
pkg/services/object/put/local.go	0.00%	7 Missing ⚠️
pkg/services/object/split/verify.go	0.00%	7 Missing ⚠️
pkg/services/object/acl/acl.go	0.00%	5 Missing ⚠️
... and 68 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2943      +/-   ##
==========================================
- Coverage   23.49%   23.47%   -0.03%     
==========================================
  Files         776      776              
  Lines       46594    46550      -44     
==========================================
- Hits        10949    10929      -20     
+ Misses      34780    34756      -24     
  Partials      865      865

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pkg/local_object_storage/blobstor/fstree/fstree_write_generic.go

pkg/local_object_storage/shard/metrics_test.go

cmd/neofs-adm/internal/modules/config/config_test.go

pkg/local_object_storage/metabase/delete_test.go

pkg/local_object_storage/metabase/exists_test.go

pkg/local_object_storage/pilorama/forest_test.go

pkg/local_object_storage/shard/shutdown_test.go

pkg/services/control/service_test.go

pkg/local_object_storage/pilorama/forest_test.go

Refs #2943.

End-rey · 2024-09-25T12:18:54Z

@roman-khimov @carpawell I don't know what to do with object.SplitInfo.SetSplitID and storagegroup.StorageGroup.ExpirationEpoch.
When I try to use object.SplitInfo.SetFirstPart some tests fail, for example:

Error Trace:	~/neofs-node/pkg/services/object/get/get_test.go:821
        	Error:      	Should be in error chain:
        	            	expected: "status: code = 2049 message = object not found"
        	            	in chain: 
        	Test:       	TestGetRemoteSmall/VIRTUAL/linking/get_chain_element_failure

Error Trace:	~/neofs-node/pkg/services/object/get/get_test.go:903
        	Error:      	Not equal: 
        	            	expected: &object.Object{objectID:(*refs.ObjectID)(0xc0000134a0), idSig:(*refs.Signature)(nil), header:(*object.Header)(0xc000273f10), payload:[]uint8{0x70, 0x97, 0x21, 0x3, 0xcd, 0x81, 0x40, 0xe, 0x8c, 0x56, 0x6a, 0x48, 0xee, 0x75, 0xc3, 0xd7, 0x3a, 0x41, 0x8, 0xa2}}
        	            	actual  : &object.Object{objectID:(*refs.ObjectID)(0xc0000134a0), idSig:(*refs.Signature)(nil), header:(*object.Header)(0xc000273f10), payload:[]uint8(nil)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -35,6 +35,3 @@
        	            	  }),
        	            	- payload: ([]uint8) (len=20) {
        	            	-  00000000  70 97 21 03 cd 81 40 0e  8c 56 6a 48 ee 75 c3 d7  |p.!...@..VjH.u..|
        	            	-  00000010  3a 41 08 a2                                       |:A..|
        	            	- }
        	            	+ payload: ([]uint8) <nil>
        	            	 })
        	Test:       	TestGetRemoteSmall/VIRTUAL/linking/OK

Error Trace:	~/neofs-node/pkg/services/object/get/get_test.go:1123
        	Error:      	Not equal: 
        	            	expected: []byte{0xcd, 0xeb, 0x33, 0x3a, 0xda, 0xab}
        	            	actual  : []byte(nil)
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,2 @@
        	            	-([]uint8) (len=6) {
        	            	- 00000000  cd eb 33 3a da ab                                 |..3:..|
        	            	-}
        	            	+([]uint8) <nil>
        	            	 
        	Test:       	TestGetRemoteSmall/VIRTUAL/right_child/OK

For storagegroup.StorageGroup.ExpirationEpoch the code used functions from the sdk, and this field is necessary for the storagegroup.StorageGroup object.

roman-khimov · 2024-09-27T06:49:11Z

There are two protocol changes here:

split v1/v2, v1 is deprecated, but we still support it, so maybe the test is relevant as is, v2 is very different in its structure
SG/generic expiration epoch, generic one should be used

First of all, let's check if we want a compatibility test (in which case deprecated features are perfectly fine) or something different.

carpawell · 2024-10-01T03:51:38Z

@End-rey

I don't know what to do with object.SplitInfo.SetSplitID

We still have to support "old" objects with v1 scheme. Deprecations are for client code mostly. All the places with it in the current code are server-side only (and tests). You can just add nolint, there should not be any updates and you should not try to use v2 objects scheme for objects that were created when v2 did not even exist.

For storagegroup.StorageGroup.ExpirationEpoch the code used functions from the SDK

I guess it may be the time when we can drop expiration from SG's body completely (when a new SG is created in cli, it should be just an expiration attribute for sure). Not sure if we can say "there will not be old SGs ever" and do not expect expiration in SG at all @roman-khimov. Mb we need to check if there are any old SGs currently on our nodes.

this field is necessary for the storagegroup.StorageGroup object

Can you explain, please?

End-rey · 2024-10-01T13:03:48Z

@carpawell

Can you explain, please?

I say about, for example, this func storagegroup.WriteToObject, but now I see that, if there is no expiration epoch field, it is set to zero.

But here, I can't easily get object that carries SG, so I have to leave it as it is?

carpawell · 2024-10-01T17:30:55Z

But here, I can't easily get object that carries SG, so I have to leave it as it is?

No, this code will stop working properly if we meet a new one SG then. I think you should either extend the interface with expiration epoch (uint64) or just return a raw object.Object and read its expiration (both ways if we still support both SG schemes @roman-khimov) in every place where this interface is used.

UPD: reread the code, ReadFromObject looks OK, do we really need to deprecate expiration getters if it is read from object's attributes, @cthulhu-rider? It can be considered as a helper struct that is read from objects only and that includes all the things we need to work with SGs (and any SG still can expire).

cthulhu-rider

great job! overall LGTM

i'll leave 2 suggestions here:

there are a lot of .Encode -> copy() changes. For most of them (mb all) we can get rid of the buffer and just use id[:]. The only risky layer could be BoltDB, be careful there
we now have cid.Size, oid.Size and user.IDSize consts. I suggest u to search by sha256.Size, 32 and 25 entries across repo, and add commit with const replacement. This ofc can be done in the next PR. P.S. note that some sha256.Size entries can be unrelated!

cthulhu-rider · 2024-10-02T03:45:58Z

cmd/neofs-cli/modules/acl/extended/create_test.go

 			ok := len(test.jsonRecord) > 0
 			require.Equal(t, ok, err == nil, err)
 			if ok {
-				expectedRecord := eacl.NewRecord()
+				expectedRecord := eacl.ConstructRecord(eacl.ActionUnspecified, eacl.OperationUnspecified, nil, eacl.Filter{})


for decoding its enough to have

Suggested change

expectedRecord := eacl.ConstructRecord(eacl.ActionUnspecified, eacl.OperationUnspecified, nil, eacl.Filter{})

var expectedRecord eacl.Record

i was in his place, docs say "Record should be created using one of the constructors." so IMO it is either wrong doc or @End-rey was right

should not must

Here is the decoding from json, which contains the correct fields for record, so I agree that declared via var will be enough.

cmd/neofs-cli/modules/acl/extended/create_test.go

cmd/neofs-cli/modules/control/synchronize_tree.go

cmd/neofs-cli/modules/storagegroup/get.go

cthulhu-rider · 2024-10-02T04:04:18Z

cmd/neofs-cli/modules/storagegroup/put.go

+			attrs[index].SetValue(expAttrValue)
+		}
+
+		obj.SetAttributes(attrs...)


so many lines to set one value. nspcc-dev/neofs-sdk-go#630

pkg/services/object/acl/eacl/v2/eacl_test.go

pkg/services/object/get/assembly_v2.go

pkg/services/object/search/search_test.go

pkg/services/object/split/verify.go

pkg/services/tree/signature.go

carpawell

Some places do not need to have a dedicated variable now, quite a valuable change IMO, this is kinda why we did SDK refactoring, but not critical. Waiting for the full test run.

carpawell · 2024-10-03T20:57:15Z

pkg/local_object_storage/metabase/util.go

-	addr.Object().Encode(key[cidSize:])
+	cnr := addr.Container()
+	obj := addr.Object()
+	copy(key[copy(key, cnr[:]):], obj[:])


not sure it is more readable now. moreover copy is not needed at all, addr is a copy already

So make for more readability this?

Suggested change

copy(key[copy(key, cnr[:]):], obj[:])

n := copy(key, cnr[:])

copy(key[n:], obj[:])

And don't understand about copy, how to concat 2 slices without copy and with the same performance?

this is cosmetic. to me something like

key = append(key[:0], cnr[:]...) key = append(key, obj[:]...)

is more preferable, i am scared of brackets, [:]):] confuses me. append(append(key[:0], cnr[:]...), cnr[:]...) in one line if needed

but ok, i benched and now know that your way is faster so ok

And don't understand about copy

i was wrong, i thought that copy here is for saving the original addr untouched (cid and oid were not arrays forever, some time ago there were slices in them), but it is not

So make for more readability this?

but stil my answer is yes. 3 closing brackets in 4 symbols is too much for me

carpawell · 2024-10-03T21:00:11Z

pkg/local_object_storage/shard/control.go

-			cnr, _ := obj.ContainerID()
-			id, _ := obj.ID()
+			cnr := obj.GetContainerID()
+			id := obj.GetID()


not needed var now

Signed-off-by: Andrey Butusov <andrey@nspcc.io>

carpawell · 2024-10-04T16:16:41Z

Tests are ok: https://rest.fs.neo.org/HXSaMJXk2g8C14ht8HSi7BBaiYZ1HeWh2xnWPGQCg4H6/1964-1728045327/index.html

Use: - `eacl.Target.RawSubjects` instead of `eacl.Target.BinaryKeys`. - `eacl.NewTargetByRole` with `eacl.Target.SetRawSubjects` or `eacl.NewTargetByAccounts` with `user.NewFromECDSAPublicKey` for targets with public keys. - `eacl.ConstructTable` instead of `eacl.NewTable`. - `eacl.ConstructRecord` instead of `eacl.NewRecord`. - `*.Get*ID` instead of `*.*ID`. - `ID.IsZero()` to check if ID is set. - comparing with == instead of `ID.Equals`. - `cid.NewFromMarshalledContainer(cnr.Marshal())` instead of. `cid.ID.CalculateID`. - `user.NewFromECDSAPublicKey` instead of `user.ResolveFromECDSAPublicKey`. - direct assign or copy instead of `ID.Encode`. - direct assign instead of `ID.SetSHA256`. - use `rand.Read` from `crypto/rand`. - use expiration epoch of storage group from attribute in header of the object carrying this storage group. - use `oidtest.ID` and `oidtest.IDs` to generate IDs for tests. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

- `unused` - `errorlint: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors` - `variable collides with imported package name` Signed-off-by: Andrey Butusov <andrey@nspcc.io>

Since we get an expiration option from the object that carries the storage group, and if it's not set we get `object.ErrNoExpiration`, now we print information about it. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

End-rey requested review from roman-khimov, carpawell and cthulhu-rider as code owners September 19, 2024 17:06

End-rey marked this pull request as draft September 19, 2024 17:10

roman-khimov reviewed Sep 19, 2024

View reviewed changes

End-rey mentioned this pull request Sep 23, 2024

Clean code #2945

Merged

End-rey force-pushed the 2940-adapt-org-wide-linter branch from 7468c36 to b721910 Compare September 23, 2024 11:48

roman-khimov added a commit that referenced this pull request Sep 24, 2024

Clean code (#2945)

a385b78

Refs #2943.

End-rey force-pushed the 2940-adapt-org-wide-linter branch 2 times, most recently from 921ac38 to 6af8199 Compare September 25, 2024 09:23

End-rey force-pushed the 2940-adapt-org-wide-linter branch 2 times, most recently from 3e7856a to 7a048fe Compare October 1, 2024 13:22

End-rey marked this pull request as ready for review October 1, 2024 15:24

cthulhu-rider mentioned this pull request Oct 2, 2024

object: Provide helper methods for system attributes nspcc-dev/neofs-sdk-go#630

Closed

cthulhu-rider reviewed Oct 2, 2024

View reviewed changes

End-rey force-pushed the 2940-adapt-org-wide-linter branch from 7a048fe to 5f77b92 Compare October 3, 2024 09:18

End-rey requested review from roman-khimov and cthulhu-rider October 3, 2024 16:09

carpawell approved these changes Oct 3, 2024

View reviewed changes

cthulhu-rider approved these changes Oct 4, 2024

View reviewed changes

workflows: reuse org-wide linter workflow

31b446d

Signed-off-by: Andrey Butusov <andrey@nspcc.io>

End-rey added 2 commits October 4, 2024 19:20

*: fix lint errors

0504c96

- `unused` - `errorlint: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors` - `variable collides with imported package name` Signed-off-by: Andrey Butusov <andrey@nspcc.io>

cli/storagegroup: print that storage group has no expiration epoch

3ebb2d8

Since we get an expiration option from the object that carries the storage group, and if it's not set we get `object.ErrNoExpiration`, now we print information about it. Signed-off-by: Andrey Butusov <andrey@nspcc.io>

End-rey force-pushed the 2940-adapt-org-wide-linter branch from 5f77b92 to 3ebb2d8 Compare October 4, 2024 16:21

roman-khimov approved these changes Oct 7, 2024

View reviewed changes

roman-khimov merged commit 624f636 into master Oct 7, 2024
21 checks passed

roman-khimov deleted the 2940-adapt-org-wide-linter branch October 7, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Org-wide linter #2943

Org-wide linter #2943

End-rey commented Sep 19, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

End-rey commented Sep 25, 2024 •

edited

Loading

roman-khimov commented Sep 27, 2024

carpawell commented Oct 1, 2024

End-rey commented Oct 1, 2024

carpawell commented Oct 1, 2024 •

edited

Loading

cthulhu-rider left a comment

cthulhu-rider Oct 2, 2024

carpawell Oct 3, 2024

cthulhu-rider Oct 4, 2024

End-rey Oct 4, 2024

cthulhu-rider Oct 2, 2024

carpawell left a comment

carpawell Oct 3, 2024

End-rey Oct 4, 2024

carpawell Oct 4, 2024

carpawell Oct 4, 2024

carpawell Oct 3, 2024

carpawell commented Oct 4, 2024

	expectedRecord := eacl.ConstructRecord(eacl.ActionUnspecified, eacl.OperationUnspecified, nil, eacl.Filter{})
	var expectedRecord eacl.Record

	copy(key[copy(key, cnr[:]):], obj[:])
	n := copy(key, cnr[:])
	copy(key[n:], obj[:])

Org-wide linter #2943

Org-wide linter #2943

Conversation

End-rey commented Sep 19, 2024

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

End-rey commented Sep 25, 2024 • edited Loading

roman-khimov commented Sep 27, 2024

carpawell commented Oct 1, 2024

End-rey commented Oct 1, 2024

carpawell commented Oct 1, 2024 • edited Loading

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Oct 4, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

End-rey commented Sep 25, 2024 •

edited

Loading

carpawell commented Oct 1, 2024 •

edited

Loading