Enhancement: extend command definition with option set

[waldekmastykarz]

In many commands we have a set of options where we require users to specify value for one of them, eg. in aad app get, we require user to specify appId, objectId or name. In all such cases, we implement validation logic in commands that checks if one of the options has been specified. We also check if only one of these options has been specified and not multiple. We repeat the validation for such options set in all commands. Instead, we should extend the command definition to allow commands to specify option sets. We should then extend the central validation logic, where we check if all required options have been specified, with checking if one option for each option set contains a value and return a relevant error message if that's not the case.

class Command {
  public optionSets(): string[][] | undefined {
    return;
  }
}

class AadAppGetCommand extends Command {
  public optionSets(): string[][] | undefined {
    return [
      ['appId', 'objectId', 'name']
    ];
  }
}

In the central validation logic we enumerate option sets and check if for each set, one of the specified options has a value. If neither of the options in a set has a value, we return error like Specify either ${options.join(', ')}. If more than one options have a value, we return error like Specify either ${options.join(', ')} but not multiple.

Once we have this logic, we should identify and refactor all commands to define their option sets and remove the obsolete validation logic and tests.

[arjunumenon]

That will be an excellent enhancement. Code will be super cleaner with introduction of this centralized validation 🔥

[vipulkelkar]

@waldekmastykarz happy to try and work on this, please assign it to me.

[appieschot]

@vipulkelkar all yours 🚀

[waldekmastykarz]

Hey @vipulkelkar, in #2996 @zhenyasav mentioned zod as a possible alternative to manual validation that we're doing now. I wonder if it could help us with validation option sets as well. Perhaps you could have a look at it?

[vipulkelkar]

@waldekmastykarz sure, i will take a look at it

[waldekmastykarz]

@vipulkelkar, when you're looking at zod, you could take as an example the validation logic of aad app add, which has a nice combination of option sets, enums, dependent property and optional properties with specific format (JSON)

[vipulkelkar]

haven't been able to dig deeper into this, here is the initial high level analysis. I was looking at a way to implement oneOf/anyOf type validation using Zod and came across this thread which led to 'Yup'
The dependent property validation, enum etc are available in Yup, below is the sample code (partially implemented) for the aad app add command

let schema = yup.object().shape({
               manifest:yup.string()
               .when('appname',{
                            is:(appname:string) => appname == null,
                            then:yup.string().required()
               }),
               appname:yup.string()
                              .when('manifest',{
                                             is:(manifest:string) => manifest == null,
                                             then:yup.string().required()
                              }),
               redirecturis:yup.string(),
               platform:yup.mixed().oneOf(['spa', 'web', 'publicClient'])
                              .when('redirecturis',{
                                             is:(redirecturis:string) => redirecturis && redirecturis.length > 0,
                                             then:yup.string().required()
                              }),
               aadAppScopeConsentBy:yup.mixed().oneOf(['admins', 'adminsAndUsers']),
               scopeName:yup.string(),
               uri:yup.string()
                              .when('scopename',{
                                             is:(scopename:string) => scopename && scopename.length > 0,
                                             then:yup.string().required()
                              }),
               scopeAdminConsentDescription:yup.string()
                              .when('scopename',{
                                             is:(scopename:string) => scopename && scopename.length > 0,
                                             then:yup.string().required()
                              }),
               scopeAdminConsentDisplayName:yup.string()
                              .when('scopename',{
                                             is:(scopename:string) => scopename && scopename.length > 0,
                                             then:yup.string().required()
                              }),
               
},[['manifest','appname']])

To get some clarification, are we looking at defining the schemas like above on each command or we implement separate validations centrally for 'anyOf', 'oneOf', 'enums' , then pass the arrays for each kind like you have proposed and then validate if the 'keys' on the options object.

Thoughts ?

[waldekmastykarz]

Thanks for looking into it @vipulkelkar. When I heard of zod and based on the few examples I've seen, I had hoped it would let us simplify validation. Looking at the above sample, it doesn't really feel like a simplification. I wonder if there are other ways to handle it. If not, then we could drop the whole idea for now and implement option sets like we speced initially.

[zhenyasav]

this seems like a situation where you're actually looking for a union of multiple kinds of shapes that can be passed into a function. One of the benefits of this approach is that you can obtain typescript types from your validation schemas that are not redundant with them (i.e. you make one expression, and you get both a validator and a type which are guaranteed to be aligned).

Expressing some of the shape's fields as not required depending on what keys are present the way it's done above doesn't make it easier to infer a type, I wonder how much it makes sense to try to re-express this as a union - that way you'll get all the benefits it seems.

[waldekmastykarz]

Expressing some of the shape's fields as not required depending on what keys are present the way it's done above doesn't make it easier to infer a type, I wonder how much it makes sense to try to re-express this as a union - that way you'll get all the benefits it seems.

Do you have an example of how that would look like @zhenyasav?

[vipulkelkar]

Expressing some of the shape's fields as not required depending on what keys are present the way it's done above doesn't make it easier to infer a type, I wonder how much it makes sense to try to re-express this as a union - that way you'll get all the benefits it seems.

@zhenyasav Pardon me but I don't follow, an example would help understand a little better. For both Zod and Yup, the checks for dependent fields/any one field being required don't seem to necessarily reduce the code. But I may be missing something.

[waldekmastykarz]

@vipulkelkar for now, let's use our original idea as we outlined in the spec. We can revisit the schema-based validation in the future as it would be a larger refactoring.