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

Avoid complex objects as arguments #361

Open
Tracked by #368
CarolKigoonya opened this issue Oct 3, 2023 · 9 comments
Open
Tracked by #368

Avoid complex objects as arguments #361

CarolKigoonya opened this issue Oct 3, 2023 · 9 comments
Labels
enhancement New feature or request

Comments

@CarolKigoonya
Copy link
Contributor

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 CarolKigoonya added the enhancement New feature or request label Oct 3, 2023
@calebkiage
Copy link
Collaborator

calebkiage commented Oct 10, 2023

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
Copy link
Collaborator

related to #24

@peombwa
Copy link
Member

peombwa commented Oct 11, 2023

@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.

@calebkiage
Copy link
Collaborator

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.

@peombwa
Copy link
Member

peombwa commented Oct 11, 2023

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.

@calebkiage
Copy link
Collaborator

🤔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
Copy link
Collaborator

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"

@peombwa
Copy link
Member

peombwa commented Oct 12, 2023

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:

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.

@calebkiage
Copy link
Collaborator

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants