Change default for queryStringDelete

[brianchance]

Summary of the changes (Less than 80 chars)
Change the default value to be opposite of queryStringAppend

Addresses #21464

[brianchance]

Not sure why the tests are failing here, the test in question is not related to my changes. I am not able to build the aspnet core project, it complains about needing a new version of MSBuild but I have the latest version of VS2019 installed.

C:\Code\aspnetcore\eng\tools\RepoTasks\RepoTasks.csproj : error : Version 5.0.100-preview.5.20251.2 of the .NET Core SDK requires at least version 16.5.0 of MSBuild. The current available version of MSBuild is 16.3.0.46305. Change the .NET Core SDK specified in global.json to an older version that requires the MSBuild version currently available.
C:\Code\aspnetcore\eng\tools\RepoTasks\RepoTasks.csproj : error MSB4236: The SDK 'Microsoft.NET.Sdk' specified could not be found.

[BrennanConroy]

@jkotalik @halter73

[halter73]

Not sure why the tests are failing here

The VerifyTrackAllCaptures and VerifyTrackAllCapturesRuleAndConditionCapture tests don't expect a query string but now there is one. The asserts need updating.

https://dev.azure.com/dnceng/public/_build/results?buildId=635298&view=ms.vss-test-web.build-test-results-tab&runId=19775132&resultId=110875&paneView=debug

I am not able to build the aspnet core project, it complains about needing a new version of MSBuild but I have the latest version of VS2019 installed.

https://github.com/dotnet/aspnetcore/blob/master/docs/BuildFromSource.md#install-pre-requisites

[halter73]

Looking at this a little more, I now see that queryStringAppend defaults to true.

The rule in VerifyTrackAllCaptures is one that is pulling data out of the query string and putting it into the path. Appending the query string in this case would be unexpected and breaking. I don't think we can actually

[halter73]

Looking at this a little more, I now see that UrlRewriteFileParser creates RedirectActions with queryStringAppend defaulted to true.

The rule in VerifyTrackAllCaptures is one that is pulling data out of the query string and putting it into the path. Appending the query string in this case would be unexpected and breaking.

We have to decide how the UrlRewriteFileParser should behave when appendQueryString is unspecified. Right now, it looks like it appends the request query string if there's a querystring in the pattern, and does not append the request querystring if there's no querystring in the pattern. I doubt this is how IIS behaves. @jkotalik Do you know what the behavior is supposed to be?

[brianchance]

I tested with IIS, it appends the query string every time unless appendQueryString="false"

[jkotalik]

Yeah, that makes sense.

[jkotalik]

Could we go ahead and remove this constructor and fix the call site in UrlRewriteFileParser to pass in queryStringDelete as !queryStringAppend.

I realize a lot of this confusion is coming from the fact that this class is shared between UrlRewrite and Apache ModRewrite.

[dnfclas]

All CLA requirements met.

[brianchance]

Thanks @halter73 - I am up and running now, thought I had all the pre-requisites installed. Tests updated.

[brianchance]

FWIW - not sure this needs QueryStringDelete, is there a case where append and delete would both be false or true?

Line 85 - If (QueryStringDelete`)

Could be just if (!QueryStringAppend)

Seems to have been added to keep the same parameters as RewriteAction, it's usage there seems to really mean remove it before continuing. Again it might be that line 131 - if (QueryStringDelete) could also just be if (!QueryStringAppend)

[halter73]

I tested with IIS, it appends the query string every time unless appendQueryString="false"

Seems good to me then. @jkotalik Do you think this warrants a breaking change announcement?

[jkotalik]

Hm, on the fence whether we should have a breaking change announcement for this. It's a bug fix that does generally seem like bad behavior, but at the same time we may want to still announce it.

I'm not sure what our policy is on breaking changes now; should we announce any breaking change at all? @pranavkm thoughts?

[halter73]

FWIW - not sure this needs QueryStringDelete, is there a case where append and delete would both be false or true?

Line 85 - If (QueryStringDelete`)

Could be just if (!QueryStringAppend)

I haven't looked at IIS's behavior, so I could be wrong here. But it seems to me by looking at the existing buggy code that QueryStringAppend was meant apply to the pattern and QueryStringDelete was meant to apply to the request query string.

Configuration	Behavior
Append = false; Delete = true	No query string ever. Works today.
Append = false; Delete = false	Only request query string. Works today.
Append = true; Delete = false	Merge query string. Works today.
Append = true; Delete = true	Only pattern query string. Still broken.

That said. If you wanted to strip the query string from the pattern, you could just not include a query string in the pattern. It would be a lot simpler if QueryStringAppend controlled whether or not to keep the request query string and have the pattern query string always appended if present. I'm not sure if this is compatible with IIS's behavior though.

[BrennanConroy]

I'm not sure if this is compatible with IIS's behavior though.

You can test IIS's behavior by installing the rewrite module and then it has a UI for testing different rules.

[jkotalik]

I'm going to take a look at how IIS rules play into this here to verify the behavior here. It seems clear we aren't 100% certain on what the behavior should be here.

[jkotalik]

Looked at the code and seems good now. IIS only has an option for appendQueryString, so the fix makes sense as appendQueryString being true implies queryStringDelete is false.

I'll make a breaking change announcement and add something to our 5.0 migration guide after this PR is merged.

[BrennanConroy]

@jkotalik Ping

[jkotalik]

@pranavkm where is there a migration guide?