Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

object: Improve testing and increase coverage for Object type #634

Merged
merged 9 commits into from
Nov 15, 2024
Merged

object: Improve testing and increase coverage for Object type #634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3c47093
object: Verify format of `Object` message fields
cthulhu-rider Oct 25, 2024
cc4d4a1
object: Do not declare stateless `Object` methods on a pointer
cthulhu-rider Oct 25, 2024
b20b162
object/test: Assert that generated instance follows the protocol
cthulhu-rider Oct 25, 2024
566bbee
object/test: Make randomizer to return valid protocol instance
cthulhu-rider Oct 25, 2024
5110db2
object: Unify ID-related methods
cthulhu-rider Oct 29, 2024
0c33fbc
object/slicer: Fix missing object owner in test input
cthulhu-rider Oct 29, 2024
ebb37bc
object: Fix version return when unset
cthulhu-rider Oct 29, 2024
0422113
object: Inline redundant unexported methods of `Object`
cthulhu-rider Oct 29, 2024
7d16474
object: Improve testing and increase coverage for `Object`
cthulhu-rider Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/container_statistic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ func TestClientStatistic_ObjectPut(t *testing.T) {
prm.WithinSession(tokenSession)

var hdr object.Object
hdr.SetOwnerID(&usr.ID)
hdr.SetOwner(usr.ID)
hdr.SetContainerID(containerID)

writer, err := c.ObjectPutInit(ctx, hdr, usr, prm)
Expand Down
14 changes: 7 additions & 7 deletions client/object_get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,8 @@

x.remainingPayloadLen = int(objv2.GetHeader().GetPayloadLength())

*dst = *object.NewFromV2(&objv2) // need smth better

return true
x.err = dst.ReadFromV2(objv2)
return x.err == nil

Check warning on line 128 in client/object_get.go

View check run for this annotation

Codecov / codecov/patch

client/object_get.go#L127-L128 

Added lines #L127 - L128 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

immo this is kinda stylish thing: i would prefer to always keep a new line before every return. it used to have it, but now it does not. white space change as @roman-khimov would say. do not insist

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit different, not a pure whitespace change since the logic is changed in these blocks. Works for me both ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not an occasional whitespace change like #634 (comment)

Copy link
Member

@carpawell carpawell Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not an occasional whitespace change

i understand but this is also not a new functionality just the old one was changed. i try not to drop/add new lines in such cases even if i have a different style

}

func (x *PayloadReader) readChunk(buf []byte) (int, bool) {
Expand Down Expand Up @@ -404,10 +403,11 @@
objv2.SetHeader(v.GetHeader())
objv2.SetSignature(v.GetSignature())

obj := object.NewFromV2(&objv2)
obj.SetID(objectID)

return obj, nil
var obj object.Object
if err = obj.ReadFromV2(objv2); err != nil {
return nil, fmt.Errorf("invalid header response: %w", err)
}

Check warning on line 409 in client/object_get.go

View check run for this annotation

Codecov / codecov/patch

client/object_get.go#L408-L409 

Added lines #L408 - L409 were not covered by tests
return &obj, nil
}
}

Expand Down
16 changes: 8 additions & 8 deletions object/fmt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (o *Object) CalculateAndSetPayloadChecksum() {

// VerifyPayloadChecksum checks if payload checksum in the object
// corresponds to its payload.
func (o *Object) VerifyPayloadChecksum() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again: nspcc-dev/.github#29. my biggest concern is why we continue doing this. it is questionable (we do not have the answer on this) but still, we see such commits. we may "fix" it here and have thousands of structs that do not follow this rule. so what's the point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're waiting for benchmarks to show copying happens and matters. Then we'll change it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly, existing ro methods are defined on types, not pointers. And some methods like this differ unreasonably. Im tryin to stick them to the most expected view basically. If there will be a particular reason to have a pointer-based method - we'll add it. Currently, there is no reason to pass pointer for payload verification

func (o Object) VerifyPayloadChecksum() error {
actual := CalculatePayloadChecksum(o.Payload())

cs, set := o.PayloadChecksum()
Expand All @@ -54,7 +54,7 @@ func (o *Object) VerifyPayloadChecksum() error {
}

// CalculateID calculates identifier for the object.
func (o *Object) CalculateID() (oid.ID, error) {
func (o Object) CalculateID() (oid.ID, error) {
return sha256.Sum256(o.ToV2().GetHeader().StableMarshal(nil)), nil
}

Expand All @@ -73,7 +73,7 @@ func (o *Object) CalculateAndSetID() error {

// VerifyID checks if identifier in the object corresponds to
// its structure.
func (o *Object) VerifyID() error {
func (o Object) VerifyID() error {
id, err := o.CalculateID()
if err != nil {
return err
Expand Down Expand Up @@ -113,13 +113,13 @@ func (o *Object) Sign(signer neofscrypto.Signer) error {
// SignedData returns actual payload to sign.
//
// See also [Object.Sign].
func (o *Object) SignedData() []byte {
func (o Object) SignedData() []byte {
return o.GetID().Marshal()
}

// VerifySignature verifies object ID signature.
func (o *Object) VerifySignature() bool {
m := (*object.Object)(o)
func (o Object) VerifySignature() bool {
m := (*object.Object)(&o)

sigV2 := m.GetSignature()
if sigV2 == nil {
Expand Down Expand Up @@ -157,7 +157,7 @@ func (o *Object) SetVerificationFields(signer neofscrypto.Signer) error {
}

// CheckVerificationFields checks all verification fields of the object.
func (o *Object) CheckVerificationFields() error {
func (o Object) CheckVerificationFields() error {
if err := o.CheckHeaderVerificationFields(); err != nil {
return fmt.Errorf("invalid header structure: %w", err)
}
Expand All @@ -172,7 +172,7 @@ func (o *Object) CheckVerificationFields() error {
var errInvalidSignature = errors.New("invalid signature")

// CheckHeaderVerificationFields checks all verification fields except payload.
func (o *Object) CheckHeaderVerificationFields() error {
func (o Object) CheckHeaderVerificationFields() error {
if !o.VerifySignature() {
return errInvalidSignature
}
Expand Down
Loading
Loading