pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
886 stars 314 forks source link

Bug report: Impossible to filter on boolean values using `m365 entra user list` #6064

Open MathijsVerbeeck opened 1 month ago

MathijsVerbeeck commented 1 month ago

Priority

(Low) Something is a little off

Description

Currently, when using the command m365 entra user list, we allow unknown options to be used for filtering. This seems to work, however, we always use startsWith as a filter operator.

This means, that when we for example want to list only the disabled accounts, we cannot do this with the current implementation, as this is a boolean field in Graph which only supports eq, ne, not or ÃŽn`, and an error is thrown.

Steps to reproduce

  1. Execute m365 entra user list --accountEnabled $false

Expected results

A list of all the disabled users

Actual results

Error: Invalid filter clause: No function signature for the function with name 'startsWith' matches the specified arguments. The function signatures considered are: startsWith(Edm.String Nullable=true, Edm.String Nullable=true).

CLI for Microsoft 365 version

v7.8.0

nodejs version

v20.11.0

milanholemans commented 1 month ago

Seems like a good improvement indeed. However, I don't know if it was ever intended to support this, so in that case maybe it's an enhancement instead of a bug.

MathijsVerbeeck commented 1 month ago

Seems like a good improvement indeed. However, I don't know if it was ever intended to support this, so in that case maybe it's an enhancement instead of a bug.

Could be, however, accountEnabled is just a property to filter on, so I think this should be intended by the command by default, but I'm fine with labeling it as an enhancement rather than a bug 😃

milanholemans commented 1 month ago

The docs say:

To filter the list of users, include additional options that match the user property that you want to filter with. For example --displayName Patt will return all users whose displayName starts with Patt. Multiple filters will be combined using the and operator.

Here it's mentioned that startsWith is used (I find that a weird way to implement filters, but anyway). So I think only string properties were intended.

MathijsVerbeeck commented 1 month ago

Then let's label it as an enhancement, as it makes a lot of sense for example to only list enabled users 😄 . The startsWith is indeed quite weird too, wouldn't it make more sense to use contains?

milanholemans commented 1 month ago

Then let's label it as an enhancement, as it makes a lot of sense for example to only list enabled users 😄 . The startsWith is indeed quite weird too, wouldn't it make more sense to use contains?

Maybe, or just an equals. But that will be a breaking change anyway.

MathijsVerbeeck commented 1 month ago

Then let's label it as an enhancement, as it makes a lot of sense for example to only list enabled users 😄 . The startsWith is indeed quite weird too, wouldn't it make more sense to use contains?

Maybe, or just an equals. But that will be a breaking change anyway.

I have just tested, and contains doesn't seem to work properly on let's say displayName, so I'm not sure how to go about this. I suggest that we start with adding filters for booleans and see if there is a need for more options after that.

milanholemans commented 1 month ago

We can always revisit the way of filtering in a V8 issue.

MathijsVerbeeck commented 1 month ago

We can always revisit the way of filtering in a V8 issue.

Would you like to close this issue and have me create a new one or rather that I rework this one?

Adam-it commented 1 month ago

We can always revisit the way of filtering in a V8 issue.

Would you like to close this issue and have me create a new one or rather that I rework this one?

I think it's fine to rework this one and target this for v8 👍

milanholemans commented 1 month ago

We can implement filtering on booleans and create another issue to discuss the way of filtering which probably will take some time.

waldekmastykarz commented 1 month ago

Since unknown properties are, well unknown, it's hard to choose the right operator given that we don't know what we're comparing against. One way to solve it, would be to get smarter based on the passed value. If we see a string, we use startsWith, if we see a bool or a number, we use equals. That way, we retain the current functionality and allow more flexibility for non-string properties.

milanholemans commented 1 month ago

It would definitely be a breaking change, but I find startsWith a weird way to use a filter. If you specify a filter I would expect it to be an equals.