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

refs #4707 bug fix to handle string array querry parameters. #4708

Merged
merged 3 commits into from
May 24, 2024

Conversation

rohitkrsoni
Copy link
Contributor

@rohitkrsoni rohitkrsoni commented May 23, 2024

fixes #4707

Working SS:
Build of sample project
image

Option Type - string[]:
var testStringArrayParameterOption = new Option<string[]>("--test-string-array-parameter", description: "Usage: newParameter=@newParameter") { Arity = ArgumentArity.ZeroOrMore };

Parameter Type:
[QueryParameter("testStringArrayParameter")] public string[]? TestStringArrayParameter { get; set; }

Tests Passing:
image

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran another smoke tests and this seems to now be failing with DateOnly error type at https://dev.azure.com/microsoftgraph/Graph%20Developer%20Experiences/_build/results?buildId=150372&view=logs&j=1b56eac7-e29c-5ce3-ffd5-9a54c8c4c465&t=be4c365e-b886-57e3-cf39-0a172b791b42

Any chance you can validate with a parameter of date format?

  '/reports/getYammerActivityUserDetail(date={date})':
    description: Provides operations to call the getYammerActivityUserDetail method.
    get:
      tags:
        - reports.Functions
      summary: Invoke function getYammerActivityUserDetail
      operationId: reports.getYammerActivityUserDetail-41fe
      responses:
        2XX:
          description: Success
          content:
            application/octet-stream:
              schema:
                type: object
                properties:
                  value:
                    type: string
                    format: base64url
        4XX:
          $ref: '#/components/responses/error'
        5XX:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: function
    parameters:
      - name: date
        in: path
        description: 'Usage: date={date}'
        required: true
        schema:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])$'
          type: string
          format: date
    x-ms-docs-grouped-path:
      - '/reports/getYammerActivityUserDetail(period=''{period}'')'

@rohitkrsoni
Copy link
Contributor Author

rohitkrsoni commented May 23, 2024

Hi @andrueastman and @calebkiage, I reproduced the issue on my local and yes the Property is of Type Date and the type of option is DateOnly
image

image

I investigated further on why this is happening and found out that after the SourceModel Code is generated, the generated code is refined and that is changing the type of parameter/property from DateOnly to Date

public async Task<bool> GenerateClientAsync(CancellationToken cancellationToken)
{
return await GenerateConsumerAsync(async (sw, stepId, openApiTree, CancellationToken) =>
{
// Create Source Model
sw.Start();
var generatedCode = CreateSourceModel(openApiTree);
StopLogAndReset(sw, $"step {++stepId} - create source model - took");
// RefineByLanguage
sw.Start();
await ApplyLanguageRefinement(config, generatedCode, cancellationToken).ConfigureAwait(false);
StopLogAndReset(sw, $"step {++stepId} - refine by language - took");
// Write language source
sw.Start();
await CreateLanguageSourceFilesAsync(config.Language, generatedCode, cancellationToken).ConfigureAwait(false);
StopLogAndReset(sw, $"step {++stepId} - writing files - took");
return stepId;
}, cancellationToken).ConfigureAwait(false);
}

Code for Property Refinement:

private static readonly Dictionary<string, (string, CodeUsing?)> DateTypesReplacements = new(StringComparer.OrdinalIgnoreCase)
{
{
"DateOnly",("Date", new CodeUsing
{
Name = "Date",
Declaration = new CodeType
{
Name = AbstractionsNamespaceName,
IsExternal = true,
},
})
},
{
"TimeOnly",("Time", new CodeUsing
{
Name = "Time",
Declaration = new CodeType
{
Name = AbstractionsNamespaceName,
IsExternal = true,
},
})
},
};

And coming to type of option, it is generated with the help of generatorMethod.PathQueryAndHeaderParameter which is not getting refined.

private static void WriteRequestInformation(LanguageWriter writer, CodeMethod generatorMethod, string parametersList, string separator, bool isStream)
{
writer.WriteLine($"var requestInfo = {generatorMethod?.Name}({parametersList}{separator}q => {{");
if (generatorMethod?.PathQueryAndHeaderParameters != null)
{
writer.IncreaseIndent();
foreach (var param in generatorMethod.PathQueryAndHeaderParameters.Where(p => p.IsOfKind(CodeParameterKind.QueryParameter)))
{
var paramName = NormalizeToIdentifier(param.Name);
var isStringParam = "string".Equals(param.Type.Name, StringComparison.OrdinalIgnoreCase) && !param.Type.IsCollection;
var indentParam = true;
if (isStringParam)
{
writer.Write($"if (!string.IsNullOrEmpty({paramName})) ");
indentParam = false;
}
var paramProperty = (param.Name.EndsWith("-query", StringComparison.Ordinal) ? param.Name.Replace("-query", "", StringComparison.Ordinal) : param.Name).ToFirstCharacterUpperCase();
writer.Write($"q.QueryParameters.{paramProperty} = {paramName};", indentParam);
writer.WriteLine();
}
writer.CloseBlock("});");
foreach (var param in generatorMethod.PathQueryAndHeaderParameters.Where(p => p.IsOfKind(CodeParameterKind.Path)))
{
var paramName = (string.IsNullOrEmpty(param.SerializationName) ? param.Name : param.SerializationName).SanitizeParameterNameForUrlTemplate();
var paramIdent = NormalizeToIdentifier(param.Name).ToFirstCharacterLowerCase();
writer.WriteLine($"if ({paramIdent} is not null) requestInfo.PathParameters.Add(\"{paramName}\", {paramIdent});");
}
foreach (var param in generatorMethod.PathQueryAndHeaderParameters.Where(p => p.IsOfKind(CodeParameterKind.Headers)))
{
var paramName = string.IsNullOrEmpty(param.SerializationName) ? param.Name : param.SerializationName;
var paramIdent = NormalizeToIdentifier(param.Name).ToFirstCharacterLowerCase();
writer.WriteLine($"if ({paramIdent} is not null) requestInfo.Headers.Add(\"{paramName}\", {paramIdent});");
}
// Set the content type header. Will not add the code if the method is a stream, has no RequestBodyContentType or if there's no body parameter.
if (!isStream && generatorMethod.Parameters.Any(p => p.IsOfKind(CodeParameterKind.RequestBody)))
{
var sanitizedRequestBodyContentType = generatorMethod.RequestBodyContentType.SanitizeDoubleQuote();
if (!string.IsNullOrWhiteSpace(sanitizedRequestBodyContentType))
{
writer.WriteLine($"requestInfo.SetContentFromParsable({RequestAdapterParamName}, \"{sanitizedRequestBodyContentType}\", model);");
}
else
{
// Being here implies a new case to handle.
var url = generatorMethod.Parent is CodeClass c ? c.Properties.FirstOrDefaultOfKind(CodePropertyKind.UrlTemplate)?.Name : null;
if (string.IsNullOrWhiteSpace(url))
{
url = "N/A";
}
throw new InvalidOperationException($"Content for request '{generatorMethod.HttpMethod}: {url}' was not set");
}
}
}
else
{
writer.WriteLine("});");
}
}

Looking into how to handle this, please provide your views, thanks

@andrueastman
Copy link
Member

andrueastman commented May 24, 2024

It looks like the refiner is targeting currentMethod.Parameters while the CLI uses the currentMethod.PathQueryAndHeaderParameters when writing parameters.

CorrectCoreTypes(currentMethod.Parent as CodeClass, DateTypesReplacements, currentMethod.Parameters

@rohitkrsoni Any chance you can make an update for the refiner to also include those parameters as well so that they are aligned. I believe this should resolve it.

@rohitkrsoni
Copy link
Contributor Author

@andrueastman, Thanks for the info, made the changes

Working SS:
Option:
image

Properties:
image

@andrueastman
Copy link
Member

Thanks @rohitkrsoni
This looks to be working now with the generation at microsoftgraph/msgraph-beta-cli#64

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the contribution @rohitkrsoni

@andrueastman andrueastman merged commit 0e195ec into microsoft:main May 24, 2024
206 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression Failure because of #4615
2 participants