Deduplicate object request fields' handling by cthulhu-rider · Pull Request #4007 · nspcc-dev/neofs-node

cthulhu-rider · 2026-06-03T14:02:14Z

No description provided.

codecov · 2026-06-03T14:05:41Z

Codecov Report

❌ Patch coverage is 25.88235% with 252 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.71%. Comparing base (6b61bea) to head (6e15806).

Files with missing lines	Patch %	Lines
pkg/services/object/server.go	15.26%	155 Missing and 6 partials ⚠️
pkg/services/object/common.go	35.00%	25 Missing and 14 partials ⚠️
pkg/services/object/acl/v2/service.go	48.07%	22 Missing and 5 partials ⚠️
pkg/services/object/get.go	0.00%	13 Missing and 1 partial ⚠️
pkg/services/object/util.go	0.00%	4 Missing ⚠️
cmd/neofs-node/object.go	0.00%	3 Missing ⚠️
pkg/services/object/acl/acl.go	0.00%	2 Missing ⚠️
pkg/services/object/util/prm.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4007      +/-   ##
==========================================
- Coverage   27.78%   27.71%   -0.07%     
==========================================
  Files         681      681              
  Lines       46797    46725      -72     
==========================================
- Hits        13001    12951      -50     
+ Misses      32557    32551       -6     
+ Partials     1239     1223      -16

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

carpawell · 2026-06-04T10:45:43Z

 	return code, nil
 }
+
+func checkRequiredObjectAddress(m *protorefs.Address) (cid.ID, oid.ID, string) {


why returning string, not an error?

it's written to status message which is of string type

Still string is confusing here and makes using this function unobvious (its declaration reads as error-free), in its essence it's exactly an error, let the caller deal with how it's stringified or not.

carpawell · 2026-06-04T10:51:35Z

 					// first object not defined, unexpected, do not attach any header
 				}
 			}
-		case *protoobject.SearchRequest:


why dont we have SearchV2Request there? it also has cID

it's never been there 🤷‍♂️
refs nspcc-dev/neofs-testcases#1327

carpawell · 2026-06-04T10:55:22Z

 }

-func WithOID(v *oid.ID) Option {
+func WithOID(v oid.ID) Option {


isnt it a separate change?

it could be, but i decided to change everything related to CID/OID in one change

roman-khimov · 2026-06-09T14:32:20Z

-		return RequestInfo{}, user.ID{}, fmt.Errorf("invalid container ID: %w", err)
-	}
-
+func (b Service) PutRequestToInfo(request *protoobject.PutRequest, initPart *protoobject.PutRequest_Body_Init, cID cid.ID, obj oid.ID) (RequestInfo, user.ID, error) {


I'd prefer cnr here, both cID and cnr were used previously, so some code has to be changed anyway and cnr is used in most of other places.

roman-khimov · 2026-06-09T14:35:37Z

 	return code, nil
 }
+
+func checkRequiredObjectAddress(m *protorefs.Address) (cid.ID, oid.ID, string) {


Still string is confusing here and makes using this function unobvious (its declaration reads as error-free), in its essence it's exactly an error, let the caller deal with how it's stringified or not.

roman-khimov · 2026-06-09T14:36:26Z

+	return cnr, obj, ""
+}
+
+func checkRequiredContainerID(m *protorefs.ContainerID) (cid.ID, string) {


Same for other functions here, it's Go, "smth + error" is what everyone expects.

roman-khimov · 2026-06-09T14:46:42Z

-		if reqInfo, objOwner, err := s.reqInfoProc.PutRequestToInfo(req); err != nil {
+		part, ok := req.Body.GetObjectPart().(*protoobject.PutRequest_Body_Init_)
+		if !ok {
+			if err = ps.forwardRequest(req); err != nil {


Just as an idea, maybe it's worth moving init handling outside of the loop, currently it's not immediately clear why we're forwarding here.

roman-khimov · 2026-06-09T14:50:24Z

+	HeadRequestToInfo(*protoobject.HeadRequest, cid.ID, oid.ID, *protosession.SessionTokenV2, *sessionv2.Token, *protosession.SessionToken, *session.Object, *protoacl.BearerToken, *bearer.Token) (aclsvc.RequestInfo, error)
+	GetRequestToInfo(*protoobject.GetRequest, cid.ID, oid.ID, *protosession.SessionTokenV2, *sessionv2.Token, *protosession.SessionToken, *session.Object, *protoacl.BearerToken, *bearer.Token) (aclsvc.RequestInfo, error)
+	RangeRequestToInfo(*protoobject.GetRangeRequest, cid.ID, oid.ID, *protosession.SessionTokenV2, *sessionv2.Token, *protosession.SessionToken, *session.Object, *protoacl.BearerToken, *bearer.Token) (aclsvc.RequestInfo, error)
+	SearchV2RequestToInfo(*protoobject.SearchV2Request, cid.ID, *protosession.SessionTokenV2, *sessionv2.Token, *protosession.SessionToken, *session.Object, *protoacl.BearerToken, *bearer.Token) (aclsvc.RequestInfo, error)


I don't like how we're mixing proto/non-proto here (technically they're duplicating) and the overall parameter count becomes bigger than desired. Can we at least avoid proto/non-proto in the same call?

roman-khimov · 2026-06-09T15:13:59Z

+
+	if meta.SessionTokenV2 != nil {
+		var token sessionv2.Token
+		if err := token.FromProtoMessage(meta.SessionTokenV2); err != nil {


This means we're decoding tokens for all requests instead of taking the decoded one from cache, that's kinda regression.

Check once and reuse wherever needed. Signed-off-by: cthulhurider <ctulhurider@gmail.com>

Stale since e4a034c. Signed-off-by: cthulhurider <ctulhurider@gmail.com>

Check once and reuse wherever needed. Signed-off-by: cthulhurider <ctulhurider@gmail.com>

cthulhu-rider marked this pull request as ready for review June 3, 2026 15:01

cthulhu-rider requested review from End-rey, carpawell and roman-khimov as code owners June 3, 2026 15:01

carpawell reviewed Jun 4, 2026

View reviewed changes

roman-khimov reviewed Jun 9, 2026

View reviewed changes

cthulhu-rider force-pushed the object-deduplicate-refs branch from 53ac095 to da26707 Compare June 10, 2026 16:22

cthulhu-rider added 3 commits June 10, 2026 19:31

sn/object: Deduplicate requested container/object ID checking

8207ea7

Check once and reuse wherever needed. Signed-off-by: cthulhurider <ctulhurider@gmail.com>

sn/object: Drop no longer needed code for deprecated SEARCH

3d6f9c2

Stale since e4a034c. Signed-off-by: cthulhurider <ctulhurider@gmail.com>

sn/object: Deduplicate requested tokens' checking

6e15806

Check once and reuse wherever needed. Signed-off-by: cthulhurider <ctulhurider@gmail.com>

cthulhu-rider force-pushed the object-deduplicate-refs branch from da26707 to 6e15806 Compare June 10, 2026 16:31

Conversation

cthulhu-rider commented Jun 3, 2026

Uh oh!

codecov Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cthulhu-rider Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jun 3, 2026 •

edited

Loading

cthulhu-rider Jun 5, 2026 •

edited

Loading