TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 106 Pull requests 12 Projects Releases 2 Activity Actions

cli: Use array type for parameters to object subcommands #1615

Merged
fyrchik merged 2 commits from elebedeva/frostfs-node:fix/array-for-range into master 2025-01-28 08:29:53 +00:00
Conversation 1 Commits 2 Files changed 4 +13 -18
4 changed files with 13 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
cmd/frostfs-cli/modules/object/hash.go
View file

@ -41,7 +41,7 @@ func initObjectHashCmd() {
flags.String(commonflags.OIDFlag, "", commonflags.OIDFlagUsage)
_ = objectHashCmd.MarkFlagRequired(commonflags.OIDFlag)
flags.String("range", "", "Range to take hash from in the form offset1:length1,...")
flags.StringSlice("range", nil, "Range to take hash from in the form offset1:length1,...")
_ = objectHashCmd.MarkFlagRequired("range")
flags.String("type", hashSha256, "Hash type. Either 'sha256' or 'tz'")

10
cmd/frostfs-cli/modules/object/patch.go
View file

@ -46,7 +46,7 @@ func initObjectPatchCmd() {
flags.String(commonflags.OIDFlag, "", commonflags.OIDFlagUsage)
_ = objectRangeCmd.MarkFlagRequired(commonflags.OIDFlag)
flags.String(newAttrsFlagName, "", "New object attributes in form of Key1=Value1,Key2=Value2")
flags.StringSlice(newAttrsFlagName, nil, "New object attributes in form of Key1=Value1,Key2=Value2")
flags.Bool(replaceAttrsFlagName, false, "Replace object attributes by new ones.")
flags.StringSlice(rangeFlagName, []string{}, "Range to which patch payload is applied. Format: offset:length")
flags.StringSlice(payloadFlagName, []string{}, "Path to file with patch payload.")
@ -99,11 +99,9 @@ func patch(cmd *cobra.Command, _ []string) {
}
func parseNewObjectAttrs(cmd *cobra.Command) ([]objectSDK.Attribute, error) {
var rawAttrs []string
raw := cmd.Flag(newAttrsFlagName).Value.String()
if len(raw) != 0 {
rawAttrs = strings.Split(raw, ",")
rawAttrs, err := cmd.Flags().GetStringSlice(newAttrsFlagName)
if err != nil {
return nil, err
}
attrs := make([]objectSDK.Attribute, len(rawAttrs), len(rawAttrs)+2) // name + timestamp attributes

10
cmd/frostfs-cli/modules/object/put.go
View file

@ -50,7 +50,7 @@ func initObjectPutCmd() {
flags.String(commonflags.CIDFlag, "", commonflags.CIDFlagUsage)
flags.String("attributes", "", "User attributes in form of Key1=Value1,Key2=Value2")
flags.StringSlice("attributes", nil, "User attributes in form of Key1=Value1,Key2=Value2")
flags.Bool("disable-filename", false, "Do not set well-known filename attribute")
flags.Bool("disable-timestamp", false, "Do not set well-known timestamp attribute")
flags.Uint64VarP(&putExpiredOn, commonflags.ExpireAt, "e", 0, "The last active epoch in the life of the object")
@ -214,11 +214,9 @@ func getAllObjectAttributes(cmd *cobra.Command) []objectSDK.Attribute {
}
func parseObjectAttrs(cmd *cobra.Command) ([]objectSDK.Attribute, error) {
var rawAttrs []string
raw := cmd.Flag("attributes").Value.String()
if len(raw) != 0 {
rawAttrs = strings.Split(raw, ",")
rawAttrs, err := cmd.Flags().GetStringSlice("attributes")
if err != nil {
return nil, err
}
attrs := make([]objectSDK.Attribute, len(rawAttrs), len(rawAttrs)+2) // name + timestamp attributes

9
cmd/frostfs-cli/modules/object/range.go
View file

@ -38,7 +38,7 @@ func initObjectRangeCmd() {
flags.String(commonflags.OIDFlag, "", commonflags.OIDFlagUsage)
_ = objectRangeCmd.MarkFlagRequired(commonflags.OIDFlag)
flags.String("range", "", "Range to take data from in the form offset:length")
flags.StringSlice("range", nil, "Range to take data from in the form offset:length")
flags.String(fileFlag, "", "File to write object payload to. Default: stdout.")
flags.Bool(rawFlag, false, rawFlagDesc)
}
@ -195,11 +195,10 @@ func marshalECInfo(cmd *cobra.Command, info *objectSDK.ECInfo) ([]byte, error) {
}
func getRangeList(cmd *cobra.Command) ([]objectSDK.Range, error) {
v := cmd.Flag("range").Value.String()
if len(v) == 0 {
a-savchuk marked this conversation as resolved Outdated
a-savchuk commented 2025-01-24 16:10:23 +00:00
Outdated
Review
Copy link

Why do we handle the error this way? This error could indicate other issues, e.g., the flag type might not be a string slice. I’d expect the following

vs, err := cmd.Flags().GetStringSlice("range")
if err := nil {
    return nil, err
}
// You might also want this if you want the code to return nil instead of an empty slice ([]objectSDK.Range{}) when the range flag is empty. However, in this context, I think nil and an empty slice are the same
if len(vs) == 0 {
    return nil, nil
}

This code works as expected because (I hope this will be clear)

  • len(vs) == 0 --> err == nil or err != nil
  • len(vs) != 0 --> err == nil

But in my opinion, it could be confusing for a reader

Why do we handle the error this way? This error could indicate other issues, e.g., the flag type might not be a string slice. I’d expect the following ```go vs, err := cmd.Flags().GetStringSlice("range") if err := nil { return nil, err } // You might also want this if you want the code to return nil instead of an empty slice ([]objectSDK.Range{}) when the range flag is empty. However, in this context, I think nil and an empty slice are the same if len(vs) == 0 { return nil, nil } ``` This code works as expected because (I hope this will be clear) - `len(vs) == 0` --> `err == nil` or `err != nil` - `len(vs) != 0` --> `err == nil` But in my opinion, it could be confusing for a reader
fyrchik commented 2025-01-24 17:04:14 +00:00
Outdated
Review
Copy link

I think each possible argument may be used as a string slice (needs do be checked).
So the only error here is when the flag is missing or has the wrong type (all these errors may be statically checked)

I think each possible argument may be used as a string slice (needs do be checked). So the only error here is when the flag is missing or has the wrong type (all these errors may be statically checked)
elebedeva commented 2025-01-27 08:29:20 +00:00
Outdated
Review
Copy link

I think each possible argument may be used as a string slice (needs do be checked).

It seems to be true.

As @fyrchik said, only possible error types here are either flag is missing or it has the wrong type. Both are problems caused by wrong initialization of the flag. Both are not related to the function logic. So IMO explicitly checking an error is more confusing here.

> I think each possible argument may be used as a string slice (needs do be checked). It seems to be true. As @fyrchik said, only possible error types here are either flag is missing or it has the wrong type. Both are problems caused by wrong initialization of the flag. Both are not related to the function logic. So IMO explicitly checking an error is more confusing here.
a-savchuk commented 2025-01-27 12:44:10 +00:00
Outdated
Review
Copy link

Both are not related to the function logic

What's the function logic in this case? Return some library error if the array is empty? It looks strange for me

Why not just return an error if there's an error? What's confusing?

> Both are not related to the function logic What's the function logic in this case? Return some _library_ error if the array is empty? It looks strange for me Why not just return an error if there's an error? What's confusing?
elebedeva commented 2025-01-27 15:17:22 +00:00
Outdated
Review
Copy link

Why not just return an error if there's an error?

That's exactly what happens here

What's confusing?

To explicitly check an error that is related to flag initialization and not to the value of parameter we got, as you suggested. We try to get a slice of passed parameters -> it is empty -> we return nil and any possible error. That's how I see it.

If you still think it's necessary, will change like this do?

if len(vs) == 0 || err != nil {
    return nil, err
}
> Why not just return an error if there's an error? That's exactly what happens [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/7bdec4632b075d2e7a0ad9b70d91817c540d40bf/cmd/frostfs-cli/modules/object/range.go#L200) > What's confusing? To explicitly check an error that is related to _flag initialization_ and _not to the value_ of parameter we got, as you suggested. We try to get a slice of passed parameters -> it is empty -> we return nil and any possible error. That's how I see it. If you still think it's necessary, will change like this do? ```go if len(vs) == 0 || err != nil { return nil, err } ```
a-savchuk commented 2025-01-27 15:37:30 +00:00
Outdated
Review
Copy link

To explicitly check an error that is related to flag initialization and not to the value of parameter we got, as you suggested

I don't suggest to check the error, just return it

if err != nil {
    return nil, err
}
> To explicitly check an error that is related to flag initialization and not to the value of parameter we got, as you suggested I don't suggest to check the error, just return it ```go if err != nil { return nil, err } ```
elebedeva commented 2025-01-27 16:09:03 +00:00
Outdated
Review
Copy link

I still don't get why do you want to do this check separately.

I still don't get why do you want to do this check separately.
a-savchuk commented 2025-01-28 07:26:43 +00:00
Outdated
Review
Copy link

Because

  • the way of error handling I suggest is more common for Go
  • in my opinion, if some code has multiple return values and returns an error, you should depend on no return values except the error
  • I still don't see any logic in checking that the returned slice is empty, both implementations are extensionally equal
Because - the way of error handling I suggest is more common for Go - in my opinion, if some code has multiple return values and returns an error, you should depend on _no_ return values except the error - I still don't see any logic in checking that the returned slice is empty, both implementations are _extensionally equal_
fyrchik commented 2025-01-28 07:49:31 +00:00
Outdated
Review
Copy link

if len(vs) == 0 || err != nil seems idiomatic to me, we have used it multiple times.

`if len(vs) == 0 || err != nil` seems idiomatic to me, we have used it multiple times.
👍 1
elebedeva commented 2025-01-28 08:21:09 +00:00
Review
Copy link

OK, fixed.

OK, fixed.
return nil, nil
vs, err := cmd.Flags().GetStringSlice("range")
if len(vs) == 0 || err != nil {
return nil, err
}
vs := strings.Split(v, ",")
rs := make([]objectSDK.Range, len(vs))
for i := range vs {
before, after, found := strings.Cut(vs[i], rangeSep)