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

feat(client/v2): support definitions of inner messages #22890

Merged
merged 12 commits into from
Dec 18, 2024
113 changes: 89 additions & 24 deletions api/cosmos/autocli/v1/options.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 19 additions & 8 deletions api/cosmos/benchmark/module/v1/module.pulsar.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

74 changes: 53 additions & 21 deletions client/v2/autocli/flag/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,38 +162,42 @@ func (b *Builder) addMessageFlags(ctx *context.Context, flagSet *pflag.FlagSet,
messageBinder.hasOptional = true
}

field := fields.ByName(protoreflect.Name(arg.ProtoField))
if field == nil {
return nil, fmt.Errorf("can't find field %s on %s", arg.ProtoField, messageType.Descriptor().FullName())
if arg.FlattenFields {
Copy link
Member

Choose a reason for hiding this comment

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

We actually don't want the user to have to precise this in their autocli options.
I think ideally it should just work.

if you put permissions you it set the whole struct. If you put permissions.level it it checks the inner field. I think splitting by . may do the job here right.
The big q is how many layer do we accept. Can you do permissions.foo.bar as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. If you set only permissions should it flatten the whole struct or should expect a json?

In the first case, should we provide an option to specify a json file? it may be better ux if message is really big.

About the layer that's a good question. I'll try to find a recursive way so there's no limit to it.

Copy link
Member

Choose a reason for hiding this comment

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

I meant, it should expect a json yeah, like today

Copy link
Member

Choose a reason for hiding this comment

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

In the first case, should we provide an option to specify a json file? it may be better ux if message is really big.

It can get messy if you have multiple input expecting a json, so to simplify things I'd say no.
If you want a better UX you break out the json in multiple inputs by using the feature added here

msgFields := messageType.Descriptor().Fields()
for j := range msgFields.Len() { // todo: this loop can be removed if msgFields.ByName(arg.protoField)
field := msgFields.Get(j)
if arg.ProtoField == string(field.Name()) {
msgType := messageType.New().Get(field).Message()
innerFields := msgType.Descriptor().Fields()
for k := range innerFields.Len() {
err := b.addFieldBindingToArgs(ctx, messageBinder, k, innerFields)
if err != nil {
return nil, err
}
}
break
}
}
continue
}

_, hasValue, err := b.addFieldFlag(
ctx,
messageBinder.positionalFlagSet,
field,
&autocliv1.FlagOptions{Name: fmt.Sprintf("%d", i)},
namingOptions{},
)
err := b.addFieldBindingToArgs(ctx, messageBinder, i, fields)
if err != nil {
return nil, err
}

messageBinder.positionalArgs = append(messageBinder.positionalArgs, fieldBinding{
field: field,
hasValue: hasValue,
})
}

totalArgs := len(messageBinder.positionalArgs)
switch {
case messageBinder.hasVarargs:
messageBinder.CobraArgs = cobra.MinimumNArgs(positionalArgsLen - 1)
messageBinder.mandatoryArgUntil = positionalArgsLen - 1
messageBinder.CobraArgs = cobra.MinimumNArgs(totalArgs - 1)
messageBinder.mandatoryArgUntil = totalArgs - 1
case messageBinder.hasOptional:
messageBinder.CobraArgs = cobra.RangeArgs(positionalArgsLen-1, positionalArgsLen)
messageBinder.mandatoryArgUntil = positionalArgsLen - 1
messageBinder.CobraArgs = cobra.RangeArgs(totalArgs-1, totalArgs)
messageBinder.mandatoryArgUntil = totalArgs - 1
default:
messageBinder.CobraArgs = cobra.ExactArgs(positionalArgsLen)
messageBinder.mandatoryArgUntil = positionalArgsLen
messageBinder.CobraArgs = cobra.ExactArgs(totalArgs)
messageBinder.mandatoryArgUntil = totalArgs
}

// validate flag options
Expand Down Expand Up @@ -273,6 +277,34 @@ func (b *Builder) addMessageFlags(ctx *context.Context, flagSet *pflag.FlagSet,
return messageBinder, nil
}

// addFieldBindingToArgs adds a fieldBinding for a positional argument to the message binder.
// The fieldBinding is appended to the positional arguments list in the message binder.
func (b *Builder) addFieldBindingToArgs(ctx *context.Context, messageBinder *MessageBinder, i int, fields protoreflect.FieldDescriptors) error {
field := fields.Get(i)
if field == nil {
return errors.New("can't find filed of inner message") // TODO: this should show names
//return fmt.Errorf("can't find field %s on %s", arg.ProtoField, messageType.Descriptor().FullName())
}

_, hasValue, err := b.addFieldFlag(
ctx,
messageBinder.positionalFlagSet,
field,
&autocliv1.FlagOptions{Name: fmt.Sprintf("%d", len(messageBinder.positionalArgs))},
namingOptions{},
)
if err != nil {
return err
}

messageBinder.positionalArgs = append(messageBinder.positionalArgs, fieldBinding{
field: field,
hasValue: hasValue,
})

return nil
}

// bindPageRequest create a flag for pagination
func (b *Builder) bindPageRequest(ctx *context.Context, flagSet *pflag.FlagSet, field protoreflect.FieldDescriptor) (HasValue, error) {
return b.addMessageFlags(
Expand Down
23 changes: 20 additions & 3 deletions client/v2/autocli/flag/messager_binder.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package flag

import (
"fmt"
"strings"

"github.com/spf13/cobra"
"github.com/spf13/pflag"
Expand Down Expand Up @@ -65,10 +66,25 @@ func (m MessageBinder) Bind(msg protoreflect.Message, positionalArgs []string) e
}
}

msgName := msg.Descriptor().Name()
// bind positional arg values to the message
for _, arg := range m.positionalArgs {
if err := arg.bind(msg); err != nil {
return err
for i := 0; i < len(m.positionalArgs); i++ {
arg := m.positionalArgs[i]
if msgName == arg.field.Parent().Name() {
if err := arg.bind(msg); err != nil {
return err
}
} else {
name := protoreflect.Name(strings.ToLower(string(arg.field.Parent().Name())))
innerMsg := msg.New().Get(msg.Descriptor().Fields().ByName(name)).Message().New()
for j := 0; j < innerMsg.Descriptor().Fields().Len(); j++ {
arg = m.positionalArgs[j+i]
if err := arg.bind(innerMsg); err != nil {
return err
}
}
msg.Set(msg.Descriptor().Fields().ByName(name), protoreflect.ValueOfMessage(innerMsg))
i += innerMsg.Descriptor().Fields().Len()
}
}

Expand All @@ -93,6 +109,7 @@ type fieldBinding struct {
field protoreflect.FieldDescriptor
}

// TODO: how to bind inner messages
func (f fieldBinding) bind(msg protoreflect.Message) error {
field := f.field
val, err := f.hasValue.Get(msg.NewField(field))
Expand Down
Loading
Loading