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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 24, 2024
Merged

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

merged 3 commits into from
May 24, 2024

Conversation

rohitkrsoni
Copy link
Contributor

@rohitkrsoni rohitkrsoni commented May 23, 2024 •
edited

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

@rohitkrsoni rohitkrsoni requested a review from a team as a code owner May 23, 2024 04:47
This was referenced May 23, 2024
Closed
Merged
andrueastman
andrueastman reviewed May 23, 2024
View reviewed changes
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 •
edited

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

kiota/src/Kiota.Builder/KiotaBuilder.cs

Lines 257 to 277 in f109c3b

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:

kiota/src/Kiota.Builder/Refiners/CSharpRefiner.cs

Lines 226 to 250 in f109c3b

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.

kiota/src/Kiota.Builder/Writers/CLI/CliCodeMethodWriter.cs

Lines 721 to 784 in f109c3b

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 •
edited

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

kiota/src/Kiota.Builder/Refiners/CSharpRefiner.cs

Line 220 in f109c3b

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

andrueastman
andrueastman approved these changes May 24, 2024
View reviewed changes
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
@andrueastman andrueastman mentioned this pull request May 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrueastman andrueastman andrueastman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression Failure because of #4615
2 participants
@rohitkrsoni @andrueastman