fix(dpg): protocol/convenience method parameters should order by default value

[archerzz]

• fix the sequence of parameters not sorted by default value (e.g. optional parameters should be at the end)
• fix a method access privilege
• regen samples & tests

fix #2766

Description

Add your description here!

Checklist

To ensure a quick review and merge, please ensure:

• The PR has a understandable title and description explaining the why and what.
• The PR is opened in draft if not ready for review yet.
  • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
• The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

• Build is completely green
  • Submissions with test failures require tracking issue and approval of a CODEOWNER
• At least one +1 review by a CODEOWNER
• All -1 reviews are confirmed resolved by the reviewer
  • Override/Marking reviews stale must be discussed with CODEOWNERS first

[ShivangiReja]

LGTM

[AlexanderSher]

The bug happens in BuildParameters where all path parameters are added as required.

Probably we should add another parameter to the AddUriOrPathParameters, e.g.:

private void AddUriOrPathParameters(string uriPart, IReadOnlyDictionary<string, InputParameter> requestParameters, bool addRequiredParameters)
{
    foreach ((ReadOnlySpan<char> span, bool isLiteral) in StringExtensions.GetPathParts(uriPart))
    {
        if (isLiteral)
        {
            continue;
        }

        var text = span.ToString();
        if (requestParameters.TryGetValue(text, out var requestParameter))
        {
            var isRequired = requestParameter.IsRequired && requestParameter.DefaultValue == null;
            if (addRequiredParameters == isRequired)
            {
                AddParameter(text, requestParameter);
            }
        }
        else
        {
            ErrorHelpers.ThrowError($"\n\nError while processing request '{uriPart}'\n\n  '{text}' in URI is missing a matching definition in the path parameters collection{ErrorHelpers.UpdateSwaggerOrFile}");
        }
    }
}

and then used in BuildParameters:

AddWaitForCompletion();
AddUriOrPathParameters(Operation.Uri, pathParameters, true);
AddUriOrPathParameters(Operation.Path, pathParameters, true);
AddQueryOrHeaderParameters(requiredRequestParameters);
AddBody(bodyParameter, contentTypeRequestParameter);
AddUriOrPathParameters(Operation.Uri, pathParameters, false);
AddUriOrPathParameters(Operation.Path, pathParameters, false);
AddQueryOrHeaderParameters(optionalRequestParameters);
AddRequestConditionHeaders(requestConditionHeaders, requestConditionRequestParameter, requestConditionSerializationFormat);
AddRequestContext();

Sort uses introspective sort, which is unstable sorting algorithm, so it doesn't guarantee that order of parameters that are already in correct order (that's why the field is called _orderedParameters) is preserved.

The order itself is defined here: #1765 (comment)
Test that covers this scenario is this one: https://github.com/Azure/autorest.csharp/pull/1781/files#diff-2129f5efd7dc8c8b923b5972cdbcf7c6afde0fa26aa04fe7228c9d40f4cdff24

One last comment is about the test project. Every separate test project increases build time and makes it harder to find correct test. If there is existing test project that handles similar cases, please add new case there instead of creating new test.

[jsquire]

@m-nash: Would you please take a look at the feedback from Alex and offer your thoughts?

[m-nash]

Let me revive this PR which meant to codify this Azure/azure-sdk#4385. We have had agreement on this ordering in the past but its very loosely documented in many different places

[archerzz]

The bug happens in BuildParameters where all path parameters are added as required.

Probably we should add another parameter to the AddUriOrPathParameters, e.g.:

private void AddUriOrPathParameters(string uriPart, IReadOnlyDictionary<string, InputParameter> requestParameters, bool addRequiredParameters)
{
    foreach ((ReadOnlySpan<char> span, bool isLiteral) in StringExtensions.GetPathParts(uriPart))
    {
        if (isLiteral)
        {
            continue;
        }

        var text = span.ToString();
        if (requestParameters.TryGetValue(text, out var requestParameter))
        {
            var isRequired = requestParameter.IsRequired && requestParameter.DefaultValue == null;
            if (addRequiredParameters == isRequired)
            {
                AddParameter(text, requestParameter);
            }
        }
        else
        {
            ErrorHelpers.ThrowError($"\n\nError while processing request '{uriPart}'\n\n  '{text}' in URI is missing a matching definition in the path parameters collection{ErrorHelpers.UpdateSwaggerOrFile}");
        }
    }
}

and then used in BuildParameters:

AddWaitForCompletion();
AddUriOrPathParameters(Operation.Uri, pathParameters, true);
AddUriOrPathParameters(Operation.Path, pathParameters, true);
AddQueryOrHeaderParameters(requiredRequestParameters);
AddBody(bodyParameter, contentTypeRequestParameter);
AddUriOrPathParameters(Operation.Uri, pathParameters, false);
AddUriOrPathParameters(Operation.Path, pathParameters, false);
AddQueryOrHeaderParameters(optionalRequestParameters);
AddRequestConditionHeaders(requestConditionHeaders, requestConditionRequestParameter, requestConditionSerializationFormat);
AddRequestContext();

Sort uses introspective sort, which is unstable sorting algorithm, so it doesn't guarantee that order of parameters that are already in correct order (that's why the field is called _orderedParameters) is preserved.

The order itself is defined here: #1765 (comment) Test that covers this scenario is this one: #1781 (files)

One last comment is about the test project. Every separate test project increases build time and makes it harder to find correct test. If there is existing test project that handles similar cases, please add new case there instead of creating new test.

@AlexanderSher Thank you very much! I've composed another PR to resolve this issue, according to your suggestion: #2779