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

elebedeva · 2025-01-24T14:27:57Z

elebedeva commented

2025-01-24 14:27:57 +00:00

Changed flag type: --range in object hashand object range, --attributes in object put and --new-attrs in object patch from String to StringSlice so that they could take comma-separated values as arguments and split them accordingly.

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Close #1570 Changed flag type: `--range` in `object hash`and `object range`, `--attributes` in `object put` and `--new-attrs` in `object patch` from `String` to `StringSlice` so that they could take comma-separated values as arguments and split them accordingly. Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>

elebedeva added the

frostfs-cli

label 2025-01-24 14:27:57 +00:00

elebedeva added 2 commits 2025-01-24 14:27:57 +00:00

[#1570 ] cli: Use array type for --range parameter to object hash afbc8fb819

Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>

[#1570 ] cli: Use array type for attributes parameters

Tests and linters / Run gofumpt (pull_request) Successful in 23s

Details

DCO action / DCO (pull_request) Successful in 38s

Details

Vulncheck / Vulncheck (pull_request) Successful in 1m10s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 1m26s

Details

Build / Build Components (pull_request) Successful in 1m47s

Details

Tests and linters / Staticcheck (pull_request) Successful in 2m0s

Details

Tests and linters / Lint (pull_request) Successful in 2m45s

Details

Tests and linters / Tests (pull_request) Successful in 3m42s

Details

Tests and linters / gopls check (pull_request) Successful in 4m36s

Details

Tests and linters / Tests with -race (pull_request) Successful in 4m38s

Details

7bdec4632b

Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>

requested reviews from storage-core-committers, storage-core-developers

2025-01-24 14:27:58 +00:00

elebedeva changed title from ~~cli: Use array type for --range parameter to object hash~~ to cli: Use array type for parameters to object subcommands

2025-01-24 14:29:11 +00:00

dstepanov-yadro approved these changes 2025-01-24 14:50:47 +00:00

Dismissed

a-savchuk reviewed 2025-01-24 16:14:49 +00:00

cmd/frostfs-cli/modules/object/range.go Outdated

					
				@ -196,4 +196,2 @@

				func getRangeList(cmd *cobra.Command) ([]objectSDK.Range, error) {

					v := cmd.Flag("range").Value.String()

					if len(v) == 0 {

a-savchuk commented

2025-01-24 16:10:23 +00:00

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

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

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

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

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

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

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

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

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

OK, fixed.

a-savchuk marked this conversation as resolved

elebedeva force-pushed fix/array-for-range from 7bdec4632b to 76b6ffaf7c

2025-01-28 08:13:37 +00:00

Compare

elebedeva dismissed dstepanov-yadro's review 2025-01-28 08:13:37 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

elebedeva force-pushed fix/array-for-range from 76b6ffaf7c to a788d44773

2025-01-28 08:15:48 +00:00

Compare

requested reviews from storage-core-committers, storage-core-developers

2025-01-28 08:24:06 +00:00

a-savchuk approved these changes 2025-01-28 08:27:29 +00:00

fyrchik approved these changes 2025-01-28 08:29:42 +00:00

fyrchik added this to the v0.45.0 milestone 2025-01-28 08:29:49 +00:00

fyrchik merged commit a788d44773 into master

2025-01-28 08:29:53 +00:00

a-savchuk commented

2025-01-28 08:32:53 +00:00

vulncheck failed

Vulnerability #2: GO-2025-3373
    Usage of IPv6 zone IDs can bypass URI name constraints in crypto/x509
  More info: https://pkg.go.dev/vuln/GO-2025-3373
  Standard library
    Found in: crypto/x509@go1.23
    Fixed in: crypto/x509@go1.23.5

vulncheck failed ``` Vulnerability #2: GO-2025-3373 Usage of IPv6 zone IDs can bypass URI name constraints in crypto/x509 More info: https://pkg.go.dev/vuln/GO-2025-3373 Standard library Found in: crypto/x509@go1.23 Fixed in: crypto/x509@go1.23.5 ```

Sign in to join this conversation.

No reviewers