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

eacl: Improvements and test coverage #605

Merged
merged 22 commits into from
Aug 9, 2024
Merged

eacl: Improvements and test coverage #605

merged 22 commits into from
Aug 9, 2024

Conversation

cthulhu-rider
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 97.80822% with 8 lines in your changes missing coverage. Please review.

Project coverage is 54.43%. Comparing base (c30d691) to head (402b83c).
Report is 1 commits behind head on master.

Files Patch % Lines
eacl/record.go 91.66% 2 Missing and 2 partials ⚠️
eacl/table.go 88.46% 2 Missing and 1 partial ⚠️
eacl/target.go 97.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage   53.69%   54.43%   +0.74%     
==========================================
  Files         164      164              
  Lines       19113    19172      +59     
==========================================
+ Hits        10262    10436     +174     
+ Misses       8447     8338     -109     
+ Partials      404      398       -6     

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

@cthulhu-rider cthulhu-rider marked this pull request as ready for review July 23, 2024 08:09
eacl/enums.go Outdated Show resolved Hide resolved
eacl/enums.go Outdated Show resolved Hide resolved
eacl/enums.go Outdated
// OperationRangeHash is an object payload range hashing Operation.
OperationRangeHash
OperationUnknown Operation = iota // Deprecated: use zero instead.
OperationGet // ObjectService.Get RPC
Copy link
Member

Choose a reason for hiding this comment

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

what is ObjectService?

Copy link
Contributor Author

@cthulhu-rider cthulhu-rider Jul 24, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i understand, just not sure if go SDK should fix such "proto package links" in comments. however, i think everybody can understand what is meant here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so - keep it as is?

eacl/enums.go Outdated Show resolved Hide resolved

return true
}
// Deprecated: compare [Table.Marshal] instead.
Copy link
Member

Choose a reason for hiding this comment

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

it was added in #180 and i really think this comparison is faster than marshaling + bytes comparison. waiter does not use it just because it happened so i think. otherwise, #180 (approved and closed) was a mistake

waiter.ContainerSetEACLWaiter
compares binary representations which is correct

i do not agree, why? binary representation depends on many things

In particular, the eACL does not change when targets are shuffled

this is one more pro to manual comparison, not just "are bytes equal?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

waiter does not use it just because it happened so i think

instead, i think it behaves as expected. Since specific method to exact table presence in the network, it compares its state in the Container contract

binary representation depends on many things

false, it is stable

this is one more pro to manual comparison, not just "are bytes equal?"

i mean its really hard to declare comparison op for such a complex type which is not comparable overall

i do not agree, why?

instances may equal binary, structurally, essentially, etc. when code compares bytes explicitly, it emphasizes the criterion of equality. Again, i think waiter is doin well


i dont mind to keep it, but i wouldnt recommend using it in the app code, only for test purposes mb

Copy link
Member

Choose a reason for hiding this comment

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

binary representation depends on many things

false, it is stable

array order? table version? both look like the same rule set to me, although may have a little bit different byte representation

i do not agree, why?

instances may equal binary, structurally, essentially, etc. when code compares bytes explicitly, it emphasizes the criterion of equality. Again, i think waiter is doin well

can understand you. but to me, acl table equality is a table that leads to the same access for every possible request sender

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ofc array order will change proto binary, so equality is very context-dependent. Binary equality is what, for example, waiter expects. Otherwise, nobody should compare eACLs outside testing purposes imo

acl table equality is a table that leads to the same access for every possible request sender

ur talkin about behavioral identity, and the current implementation violates it since a false negative return is possible: array of independent records may be shuffled w/o access changes. In this case, EqualTables returns false. Such tables are essentially the same, but bin diff

eacl/filter.go Show resolved Hide resolved
eacl/enums.go Outdated Show resolved Hide resolved
eacl/enums.go Outdated Show resolved Hide resolved
eacl/enums.go Outdated
}
//
// Deprecated: do not use it! The protocol does not define a consistent string
// format. Declare your own string mapping.
Copy link
Member

Choose a reason for hiding this comment

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

Why can't SDK provide this? Why should everyone implement some mappings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are not string types in their essence, so it is redundant to introduce a consistent string format imo. Can be implemented in a subpackage if this is such a popular need. But i wouldnt leave it as a core interface

Copy link
Member

Choose a reason for hiding this comment

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

SDK is far from being just a "core interface". It has to provide some sugar as well, this is one of the cases like that. Yeah, we've got String(), but then missing a DecodeString() it can make SDK less user-friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i prefer to leave the ability of changing String() return values. To me, they must be printable and human-readable, nothin more. And i dont mind to provide string mapping, i just dont think it should be a type interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented as functions around enum types. Will be a typical example if the app decides to implement custom mapping

eacl/filter.go Show resolved Hide resolved
eacl/filter.go Show resolved Hide resolved
eacl/target.go Outdated Show resolved Hide resolved
eacl/target.go Outdated Show resolved Hide resolved
eacl/filter.go Outdated Show resolved Hide resolved
eacl/record.go Show resolved Hide resolved
eacl/filter.go Outdated Show resolved Hide resolved
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

But I would wait for @roman-khimov's threads.

@roman-khimov
Copy link
Member

Conflicts.

To get closer to the protocol. `Unknown` constants are deprecated.

Signed-off-by: Leonard Lyubich <[email protected]>
Also drop mention of to-be-deprecated elements.

Signed-off-by: Leonard Lyubich <[email protected]>
Enum types provide `EncodeToString`/`DecodeString` interface
representing stable stringers. Since the string representation is not
declared by the NeoFS protocol, applications are allowed to use this
functionality only for internal conversions (e.g. parsing command line
input). In addition, enumeration types are simple integers. Thus,
implementing this functionality as type methods is quite redundant. At
the same time, exporting consistent in-box mapping can simplify
development of some apps.

`XToString` / `XFromString` function are provided now. Being global
functions they emphasize that if the app is satisfied with the lib
strings - it can take and use them, otherwise reimplement them by
analogy. Methods are deprecated for the sake of new functions.

Signed-off-by: Leonard Lyubich <[email protected]>
The function is unused. For example, `waiter.ContainerSetEACLWaiter`
compares binary representations which is correct. Overall, NeoFS
protocol does not declare comparison operation b/w two eACLs. In
particular, the eACL does not change when targets are shuffled. Thus, to
avoid introducing a synthetic property, the func will be removed soon.

Signed-off-by: Leonard Lyubich <[email protected]>
All other types are components and are not transported autonomously
outside the eACL. Thus, for the sake of minimal support, these functions
will be removed soon.

Signed-off-by: Leonard Lyubich <[email protected]>
Refs #483.

Signed-off-by: Leonard Lyubich <[email protected]>
This allows to be less tied to packages that are about to be abolished.

Signed-off-by: Leonard Lyubich <[email protected]>
This also emphasizes that role and public keys are mutually exclusive.
Additional helpers for ECDSA encoding are provided too.

Refs #483.

Signed-off-by: Leonard Lyubich <[email protected]>
Signed-off-by: Leonard Lyubich <[email protected]>
And improve docs a bit.

Signed-off-by: Leonard Lyubich <[email protected]>
Additionally, provide helpers for object filtering: with them, creating
a rule will be more efficient in terms of reallocations. Due to this,
all `Add*Filter*` methods are marked deprecated.

Refs #483.

Signed-off-by: Leonard Lyubich <[email protected]>
`AddRecord` was inefficient in terms of allocations, so it is deprecated
now.

Signed-off-by: Leonard Lyubich <[email protected]>
Refs #483.

Signed-off-by: Leonard Lyubich <[email protected]>
Also test the functions.

Signed-off-by: Leonard Lyubich <[email protected]>
This allows to use instance by value as interface implementation.

Signed-off-by: Leonard Lyubich <[email protected]>
For symmetry with `RawSubjects` and testing.

Signed-off-by: Leonard Lyubich <[email protected]>
@roman-khimov
Copy link
Member

Linter fails.

Caused by #606 which should make them PASS.

Signed-off-by: Leonard Lyubich <[email protected]>
@roman-khimov roman-khimov merged commit 60e04a2 into master Aug 9, 2024
12 checks passed
@roman-khimov roman-khimov deleted the eacl branch August 9, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants