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.

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

Separate type for object header #652

Open
cthulhu-rider opened this issue Dec 6, 2024 · 8 comments
Open

Separate type for object header #652

cthulhu-rider opened this issue Dec 6, 2024 · 8 comments
Labels
enhancement Improving existing functionality I2 Regular impact S2 Regular significance U4 Nothing urgent
Milestone

Comments

@cthulhu-rider
Copy link
Contributor

Is your feature request related to a problem? Please describe.

currently, object provides Object structure type, but no Header one. This type would be helpful for HEAD ops (Client, node storage, etc.). Now such ops return instances of the Object type with unset payload field. I see following drawbacks in this approach:

  1. header is always requested by object ID, and the resulting instance has the one. This is redundant now
  2. header encoding becomes more tricky. This leads to additions like https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go/object#Object.MarshalHeaderJSON, which is not rly bad, but could be provided apriori. For binary encoding, https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go/object#Object.CutPayload is used, but it adds ID to the result while these BLOBS are usually stored by ID key itself
  3. entity appears in the protocol and in different system parts, it is quite natural for it to have a type
  4. in code, the header is not distinguished from the object, although it is a narrower type. Because of this, comments have to be added, which weakens the type system

Describe the solution you'd like

type Header struct { /* container, owner, ... */ }
type Object struct {
	Header
	id        oid.ID
	signature neofscrypto.Signature
	payload   []byte
}
func (*Client) HeadObject(...) (object.Header, neofscrypto.Signature, error)

Additional context

iirc @carpawell had questions why there is no header type. I couldnt find any issue, pls share if smth already exists

@cthulhu-rider cthulhu-rider added discussion Open discussion of some problem feature Completely new functionality labels Dec 6, 2024
@carpawell
Copy link
Member

I always did not understand why there is no header while IMO it is a natural understanding for our HEAD operation and objects in general: every object has a header and every object has a payload. Sometimes we need only a header and vice versa. I have not opened any issues about it.

@roman-khimov
Copy link
Member

Yeah, we need this type, passing object.Object and doing CutPayload() tricks is very confusing, you never know what you have in code: a complete object or header only. But we need it to follow proto spec as well, signature is a part of the header IIRC.

@roman-khimov roman-khimov added enhancement Improving existing functionality U4 Nothing urgent S2 Regular significance I2 Regular impact and removed discussion Open discussion of some problem feature Completely new functionality labels Dec 9, 2024
@roman-khimov roman-khimov added this to the v1.0.0-rc13 milestone Dec 9, 2024
@cthulhu-rider
Copy link
Contributor Author

signature is not a part of header itself https://github.com/nspcc-dev/neofs-api/blob/e66b25d4bf2afc4472023e1e2c2467f694f5a0e1/object/types.proto#L226, so it's transmitted beside https://github.com/nspcc-dev/neofs-api/blob/e66b25d4bf2afc4472023e1e2c2467f694f5a0e1/object/service.proto#L462

@roman-khimov
Copy link
Member

Header, SignedHeader, Object?

@cthulhu-rider
Copy link
Contributor Author

Header, SignedHeader, Object?

SignedHeader includes ID here?

@roman-khimov
Copy link
Member

It's so tempting for me to make it a method with cached internal value (transaction.Transaction). We need to evaluate real use cases, I know we have a number of uses for SignedHeader (maybe even more so than for Header alone).

@cthulhu-rider
Copy link
Contributor Author

cached values ​​are often a source of errors. I would stick with simple tuples with open logic

real use cases

https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go/object#Object.MarshalHeaderJSON is what i remember. https://github.com/nspcc-dev/neo-go/blob/5f92da21fa43e4fe80fb965c78d5d244b6003ffd/pkg/services/oracle/neofs/neofs.go#L185. The getObjHeader could return object.Header

various tuples can be used:

and i remember no usecase where we need (ID, signature, header) tuple

finally, for now i see HeaderHeaderWithSignatureObject type system

@cthulhu-rider
Copy link
Contributor Author

cthulhu-rider commented Dec 9, 2024

it must be noted that transition to the heading types must be carried out carefully. For example, CutPayload returns an object. So, https://github.com/nspcc-dev/neofs-node/blob/918307b0739feb086416e667497c676170e0c9ca/pkg/local_object_storage/metabase/put.go#L186 means that metabase stores (ID, signature, header) tuples. Not rly good to store storage key in the value, but it is what it is, updated code must take this into account. Normally, metabase could store HeaderWithSignature

ALSO!!!: do not forget about field numbers. Object and HeaderWithSignature have different ones. Field nums are essential for Marshal and any protobuf functionality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving existing functionality I2 Regular impact S2 Regular significance U4 Nothing urgent
Projects
None yet
Development

No branches or pull requests

3 participants