Bug report: Updating a content type property with an empty value throws an exception

[martinlingstuyl]

Description

If I run the following command:

m365 spo contenttype set --webUrl '<url>' --id '<some-id>' --listTitle '<some-title>' --Description ''  --debug

The command throws a 400 exception.

The reason this happens is because the Description option is translated to a boolean when calling the API:

"data": {
    "Description": true
  }

Steps to reproduce

1. Update a content type with a description
2. Now try to empty it by running the same command with an empty value in string quotes.

Expected results

The property is emptied.

Actual results

The command crashes.

CLI for Microsoft 365 version

5.8 (next)

nodejs version

16.15.0

Operating system (environment)

Windows

Shell

PowerShell

Additional Info

This issue does not occur on bash.

[waldekmastykarz]

Good catch! Would it solve the problem to force all unknown properties to be treated as a string or is the Description an exception here and we'd solve it by simply forcing just it to be treated as a string?

[martinlingstuyl]

I ran into it because I wanted to reset EditFormClientSideComponentId 😁 So it's multiple options.
It's not just unknowns I am just finding out.

I tried spo field set => did not have the same problem, as it is using the ProcessQuery API

I tried spo list set --description '' (which is not an unknown option) => and that fails with the same error.

Could it be a problem with minimist? that it parses '' to true?

[martinlingstuyl]

By the way, this is in powershell.

In zsh spo list set --description "" leads to an empty data object that's posted. ({ }) The request succeeds, but the description is not emptied.

It seems the check

if (options.description) {
      requestBody.Description = options.description;
}

does not add the property to the requestBody object. (I logged it to the console to see) So I guess this means it must be the minimist parser that's responsible.

[martinlingstuyl]

I tried spo field set => did not have the same problem, as it is using the ProcessQuery API

Correction, it has nothing to do with the API. The call is going through successfully. But the Description is updated with value "true".

[waldekmastykarz]

First thing we should try is to force these options to be treated as string by minimist and see if that solves it.

[waldekmastykarz]

Also, if (options.description) { will return false for an empty script, so we'd need to adjust it to check explicitly for undefined

[martinlingstuyl]

Hi @waldekmastykarz,

TL;DR

Explicitly treating the option as a string resolves the issue.

What happens below the hood

So I've added some logging statements, and this is what happens:

Shell	Script	rawArgs	parsedArgs
bash/zsh	m365 spo contenttype set --Description ""	"--Description",""	"Description":""
posh	m365 spo contenttype set --Description ""	"--Description"	"Description": true
bash/zsh	m365 spo contenttype set --Description "1"	"--Description","1"	"Description": 1
posh	m365 spo contenttype set --Description "1"	"--Description","1"	"Description": 1

I see a subtle difference in the rawArgs coming in. And aside from that, minimist apparently tries to determine the type based on the value.

I've tested this with spo contenttype set (with the unknown option --Description) and with spo list set (and the known option --description)

[waldekmastykarz]

Good to get this verified. The question stands: shall we treat all unknown options as strings to avoid side-effects of some values being treated as numbers or booleans?

[martinlingstuyl]

I think we should @waldekmastykarz !

But this behavior evidently also occurs on known options (spo list set --description ""). Should we start adding every relevant property to the string and boolean arrays?

And by the way, this is actually also the reason for the discussion here In that situation, true is actually interpreted as a boolean type by minimist, while the code expects 'true' as a string type, thereby blocking me in the validation phase.

We need some bigger solution to encompass every scenario I fear.

[waldekmastykarz]

Should we start adding every relevant property to the string and boolean arrays?

Where it makes sense, yes.

Should we start adding every relevant property to the string and boolean arrays?

Let's start small and avoid trying to boil the ocean.

[martinlingstuyl]

If we would let all unknown options be parsed as strings, would we get into trouble with actual numbers or boolean fields that are saved to (for example) SharePoint

[waldekmastykarz]

Possibly. One way about it, it so leave unknown options as-is, and for the few options, define known equivalents which we handle properly. If we start adding specific unknown options to types, they won't make much of an unknown, wouldn't you say? 😄

[waldekmastykarz]

@martinlingstuyl to bring this to a conclusion, shall we introduce a new --description property and ensure it can be set to an empty string?

[waldekmastykarz]

@martinlingstuyl is this issue still relevant? Shall we proceed with what I proposed? Final ping before we close it as stale

[martinlingstuyl]

Hi @waldekmastykarz, it is still very relevant. I got a little sidetracked though.
I think I'm not yet convinced of the named solutions in the comments.

The issue does not only affect the description option. It affects any string option.
I think we can limit the possibly
problematic situations to set commands though, as there's no reason to use an empty string in an option on an add command.

Thinking about all this, I think we need a firmer solution that encompasses all these potential situations.

As I see it we can go a few routes:

Route 1: fix the immediate symptom

We could solve it by going through all known string options on set commands and add them to the types.strings array. This way we'd be sure that they are be interpreted correctly.

cons:

• People might technically run into the same problems with number options, and minimist offers no setting for those, it seems.

• Also, unknown options are still a problem in this scenario.

Route 2: fix the culprit -> flag parsing

There's another route: we fix the culprit: Flag parsing. Minimist just assumes that any option without a value is a flag and parses it to true.

We could therefore go down another route: help minimist by creating a types.flags array, and add all flags to that list.
Before passing the arguments to mimimist, we can now go through the list (just like we did with booleans) and add an empty string to any option that is not in the flags array.

As an alternate solution we could also add the flags to the types.booleans array instead and enhance the parsing in the same manner as written above.

pros

• this fixes the parsing of unknown and known options and probably for integers as well, though an empty string is technically no number, so it could go wrong somewhere.

• we've already implemented a part of this with the fix for the Boolean problem. This would be along the same lines.

cons

• it might impact performance slightly because of the added code in the parsing process. Not sure how much though.

• it impacts the execution of all commands, not just those that are affected.

I'm very interested to hear your take on this.
Point is: the parsing problem affects the entire CLI, but probably only the set commands are really impacted by it. I'd personally favor we go down route 2 and enhance the parsing by going updating the arguments before handing them to minimist, as we're already doing for booleans.

[martinlingstuyl]

By the way. Important side note here: I must first test if route 2 actually works :) I assume it does but I'm not sure.

What I mean is that the rawArgs contains something like the following when it's passed to minimist:

["spo", "contenttype", "set", "--someStringOption", "--anotherOption", "some value"]

--someStringOption will now be inferred to be a flag by minimist, and will get the value true.

My proposition is that we update the array to:

["spo", "contenttype", "set", "--someStringOption", "", "--anotherOption", "some value"]

before passing it to minimist, based on --someStringOption not being in the flags or booleans array.

[milanholemans]

According to me, --someStringOption is not really valid syntax for the CLI because that's indeed a flag. I think that people should use --someStringOption "" or --someStringOption null. However the last one will be parsed to "null".

[martinlingstuyl]

Hi @milanholemans, the point is that --someStringOption "" gets parsed wrong by minimist. It will be parsed to a Boolean value true. This is because "" is nothing. It's no argument. Hence minimist assumes that the preceding option argument is a flag.

So if you want to reset a field to an empty string, you cannot.

[milanholemans]

Aw ok I see, should have read the entire thread, sorry!

[waldekmastykarz]

I'd like to clarify it a bit: this issue only affect string options that allow an empty value. Not all do, so let's keep that in mind when thinking of the impact of this issue on our code base. This also applies to number values that could be set to nothing: I don't know if that's actually possible or something that just might happen in theory without an actual example that we current have in the CLI.

@martinlingstuyl have you tried wrapping the value in two set of quotes, like '""'?

Also, before we set on a specific fix, let's double check that we won't be changing behavior that's native to all CLIs, where it's standard that --foo '' is equal to --foo and treating it as anything else is confusing.

[milanholemans]

I'm not on my PC right now, but have you also tried to use --someStringOption=""?
This previously fixed argument parsing when an argument started with a dash -

[martinlingstuyl]

I'm not on my PC right now, but have you also tried to use --someStringOption=""?

Not sure if I did, will try it asap!

[martinlingstuyl]

That actually works @milanholemans, @waldekmastykarz! using --someStringOption="" takes care of passing an empty string arg to minimist, and it's parsed as a empty string as well.
Apparently I did not try this before with this issue, although I know the syntax.

By the way, as said earlier: the problem with --description "" only occurs in powershell.

We could just leave the arg parsing as is, and add this oddity to the documentation, even if I still find it not very logical that it should work like this.

What's your opinion on this?

[milanholemans]

Nice to know that this works 😃
We already have a section about the argument parsing with a dash https://pnp.github.io/cli-microsoft365/user-guide/using-cli/#values-starting-with-a-dash-

Maybe we can generalize it a bit and include the empty string argument as well.

And what about passing a null value? Or is it the same as passing an empty string?

[waldekmastykarz]

Great finding @milanholemans and it's weird that there's a difference between --option '' and --option=''. I'd expect that they're equal, but it turns out they aren't! I wonder if it originates from minimist or the shell. Either way, let's definitely document this because I'm sure folks will run into it.

[martinlingstuyl]

I wonder if it originates from minimist or the shell.

It's the PowerShell, not minimist. That's clear @waldekmastykarz .

So we're agreed we're seeing this as a valid workaround and that we don't need to influence the args list for this ourselves?

[waldekmastykarz]

Correct @martinlingstuyl.

[martinlingstuyl]

In that case I'll close this issue and create a new one for updating the documentation!