msgraph-cli icon indicating copy to clipboard operation
msgraph-cli copied to clipboard

Avoid complex objects as arguments

Open CarolKigoonya opened this issue 2 years ago • 9 comments

Commands that create/update objects require users to pass the object to create as a JSON object. This is inconvenient, because it requires users to manually build these strings and properly escape quotes. In shells like bash, where everything’s a string, building such complex objects is inconvenient and complicates the user experience.

Recommendation: expose options for the different properties of complex objects so that users don’t need to handle JSON strings in the shell

CarolKigoonya avatar Oct 03 '23 12:10 CarolKigoonya

The idea I have is to allow the user to specify --body multiple times while passing the properties of the object using a variation of JsonPath.

For example:

mgc users post --body "displayName=value" --body "address.streetName=xxx" --body "businessPhones[]=phone0"

Should send the object:

{
    "displayName": "value",
    "address": {
        "streetName": "xxx"
    },
    "businessPhones": [
        "phone0"
    ]
}

Potential issues with this approach:

  1. JsonPath is a query language so using it for updates may be confusing.
  2. Inferring types from the strings is going to be tricky. i.e. how do we distinguish between the number 1 and the string "1"? Or the identifier null vs the string "null"? We want to avoid having the users escape quotes in the string as that will have the same escaping problem we're trying to solve. Maybe we can use PowerShell's convention of adding a special prefix before an identifier, but that doesn't solve the problem of numbers.

calebkiage avatar Oct 10 '23 17:10 calebkiage

related to #24

calebkiage avatar Oct 10 '23 18:10 calebkiage

@calebkiage, how about something like mgc users post --displayName "value" --addressStreetName "xxx" --businessPhones "phone0" --businessPhones "phone1". This way, we can have overloads for --body for Json objects and flattened parameters.

peombwa avatar Oct 11 '23 16:10 peombwa

I thought about that because it would make for a more pleasant experience. I however saw 2 potential problems with that approach.

  1. adding those options would increase the binary size because each command would require a definition of all the options with their descriptions.
  2. there might be issues with option conflicts. For example, consider a command like:
    mgc users drives share create --user-id {my user id} --body '{"folder": "test", "user-id": "other user's id"}'
    
    In this situation, then there will be 2 options with 2 separate meanings but with the same name user-id.

calebkiage avatar Oct 11 '23 17:10 calebkiage

there might be issues with option conflicts. For example, consider a command like: mgc users drives share create --user-id {my user id} --body '{"folder": "test", "user-id": "other user's id"}'

Sure, but that would be as a result of a poorly designed API. I don't think such APIs exist in Microsoft Graph as they would have resulted in parameter name conflict in PowerShell. Request bodies are flattened as parameters to the first level in PowerShell with an overload to provide body parameter as a hashtable (this would be JSON object in CLI) - https://learn.microsoft.com/powershell/module/microsoft.graph.files/new-mgdrive?view=graph-powershell-1.0.

peombwa avatar Oct 11 '23 19:10 peombwa

🤔Considering that one of our goals is to support non-graph APIs, we'll have to accept the possibility that some of them won't follow graph's conventions. I believe we want to allow any valid OpenAPI description to generate a working CLI. That and the binary size concern is the main reason I didn't suggest the exploded options approach. I'm thinking of what name to use instead of body.

calebkiage avatar Oct 11 '23 20:10 calebkiage

What if we used shorthand -b for the updated syntax? i.e.

mgc users post -b "displayName=value" -b "address.streetName=xxx" -b "businessPhones[]=phone0"

calebkiage avatar Oct 12 '23 10:10 calebkiage

IMO, we should align with existing CLI design of flattening request bodies into parameters if discovery and ease of use is what we are aiming for. For example, this how existing CLIs flatten request body parameters today:

  • Azure CLI - https://learn.microsoft.com/cli/azure/image/builder?view=azure-cli-latest#az-image-builder-create()
  • GitHub CLI - https://cli.github.com/manual/gh_issue_create
  • AWS CLI - https://awscli.amazonaws.com/v2/documentation/api/latest/reference/amp/create-workspace.html

The -b or --body syntax can still lead to discovery challenges for customers as they may not know which parameters to set for the body or which parameters are required without consulting the API reference docs.

I think it's worth checking to see if the parameter name conflict exists in this APIs covered by Kiota today.

peombwa avatar Oct 12 '23 15:10 peombwa

@peombwa, I agree that we should follow conventions and ease friction where possible, but in this situation, I don't think it wise to narrow our scope to match those 3 CLIs. Each of them can ensure that there are no conflicts because they only aim to support 1 API. Our goal is to support many unknown APIs. I can investigate the possibility of adding a configuration to enable parameter flattening on kiota and respond with my findings.

@CarolKigoonya, what do you think?

calebkiage avatar Oct 12 '23 16:10 calebkiage