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

CLI generation does not handle DateTime path parameters #4615

Closed
baywet opened this issue May 7, 2024 · 10 comments · Fixed by #4679
Closed

CLI generation does not handle DateTime path parameters #4615

baywet opened this issue May 7, 2024 · 10 comments · Fixed by #4679
Assignees
Labels
CLI Work to support generating CLIs with Kiota priority:p0 Blocking issue. Loss of critical functions eg security/privacy violation. Bug SLA<=48hrs status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:bug A broken experience
Milestone

Comments

@baywet
Copy link
Member

baywet commented May 7, 2024

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Source Build

Client Library/SDK Language

CLI

Describe the bug

Microsoft Graph beta CLI generation is failing with the following error:

src/generated/App/OnlineMeetings/GetAllRecordingsmeetingOrganizerUserIdMeetingOrganizerUserIdWithStartDateTimeWithEndDateTime/GetAllRecordingsmeetingOrganizerUserIdMeetingOrganizerUserIdWithStartDateTimeWithEndDateTimeRequestBuilder.cs(105,97): error CS0029: Cannot implicitly convert type 'string' to 'System.DateTimeOffset?'

(x12)

Expected behavior

CLI to handle DateTime path parameters properly.

How to reproduce

  1. generate a CLI client with the date time as path parameters
  2. build the CLI

Open API description file

openapi: 3.0.3
info:
  title: Microsoft Graph get user API
  version: 1.0.0
servers:
  - url: https://graph.microsoft.com/v1.0/
paths:
 '/app/onlineMeetings/getAllRecordings(meetingOrganizerUserId=''@meetingOrganizerUserId'',startDateTime=@startDateTime,endDateTime=@endDateTime)':
    description: Provides operations to call the getAllRecordings method.
    get:
      responses:
        2XX:
          description: Success
          content:
            application/json:
              schema:
                type: object
                properties:
                  id:
                    type: string
    parameters:
      - name: meetingOrganizerUserId
        in: query
        description: 'Usage: meetingOrganizerUserId=''@meetingOrganizerUserId'''
        schema:
          type: string
          nullable: true
      - name: startDateTime
        in: query
        description: 'Usage: startDateTime=@startDateTime'
        schema:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          format: date-time
          nullable: true
      - name: endDateTime
        in: query
        description: 'Usage: endDateTime=@endDateTime'
        schema:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])T([01][0-9]|2[0-3]):[0-5][0-9]:[0-5][0-9]([.][0-9]{1,12})?(Z|[+-][0-9][0-9]:[0-9][0-9])$'
          type: string
          format: date-time
          nullable: true

Kiota Version

1.14.0

Latest Kiota version known to work for scenario above?(Not required)

No response

Known Workarounds

No response

Configuration

No response

Debug output

https://microsoftgraph.visualstudio.com/Graph%20Developer%20Experiences/_build/results?buildId=147067&view=logs&j=e7ad2393-6f47-5a6c-43fe-f034bda9890e&t=ec6c7d4d-190c-5a0e-65ac-93472f7d2fef

Click to expand log ```
</details>


### Other information

_No response_
@baywet baywet added type:bug A broken experience status:waiting-for-triage An issue that is yet to be reviewed or assigned CLI Work to support generating CLIs with Kiota priority:p0 Blocking issue. Loss of critical functions eg security/privacy violation. Bug SLA<=48hrs and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels May 7, 2024
@baywet baywet added this to the Backlog milestone May 7, 2024
@rohitkrsoni
Copy link
Contributor

Hi @baywet, I want to debug and work on this issue, can I get this issue assigned and could you please give me command to reproduce this bug, thanks

@baywet
Copy link
Member Author

baywet commented May 10, 2024

@rohitkrsoni Thanks for volunteering.
I've edited the description in my original post to be complete. Save it to a local file
Here is a project you can use for local repro, nuke the content of "generated" except for the lock file. For the lock file, replace the description URL by the path to the saved description.
Lastly, run kiota update -o pathToGeneratedDirectory to generate the code.
And dotnet build in the project to get the build error.

I'm guessing the problem is located here where the string needs to be parsed before being assigned.

writer.Write($"q.QueryParameters.{paramProperty} = {paramName};", indentParam);

@rohitkrsoni
Copy link
Contributor

Hi @baywet, I am going to pause my investigation on this bug as the other issue I was working on is progressing but here are my investigation so far and I am happy to work on it after that.

The build is failing because our generator is trying to assign a string to System.DateTimeOffSet

image

Here q.QueryParameters.StartDateTime is of type DateTimeOffset

image

And startDateTime variable is of type string as:

  1. We are passing string as the parameter to create the option.
image
  1. When we are getting the value of the option it is not parsing it to DateTimeOffset rather it is keeping it string as string type was mentioned when creating the option.
image

Now, this is happening because:

To create the API Client Class and it's properties (query parameters), GetPrimitiveType() method is used to get the type of the parameter.

private static CodeType? GetPrimitiveType(OpenApiSchema? typeSchema, string? childType = default)

Based on the type and format of the parameter, the type is returned

var format = typeSchema?.Format ?? typeSchema?.Items?.Format;
var primitiveTypeName = (typeName?.ToLowerInvariant(), format?.ToLowerInvariant()) switch
{
("string", "base64url") => "base64url",
("file", _) => "binary",
("string", "duration") => "TimeSpan",
("string", "time") => "TimeOnly",
("string", "date") => "DateOnly",
("string", "date-time") => "DateTimeOffset",
("string", "uuid") => "Guid",
("string", _) => "string", // covers commonmark and html
("number", "double" or "float" or "decimal") => format.ToLowerInvariant(),
("number" or "integer", "int8") => "sbyte",
("number" or "integer", "uint8") => "byte",
("number" or "integer", "int64") => "int64",
("number", "int32") => "integer",
("number", _) => "double",
("integer", _) => "integer",
("boolean", _) => "boolean",
(_, "byte") => "base64",
(_, "binary") => "binary",
(_, _) => string.Empty,
};

But when it comes to the value of the option which our generator is trying to set for the query parameter, things are different.
The method responsible to write the code for creating the options and its type in the BuildGetCommand() is:

private static List<string> WriteExecutableCommandOptions(LanguageWriter writer, List<(string, string, CodeParameter?)> parametersList)

And the parametersList is passed from the method WriteExecutableCommand

private void WriteExecutableCommand(CodeMethod codeElement, CodeClass parentClass, RequestParams requestParams, LanguageWriter writer, string name)
{
if (parentClass
.UnorderedMethods
.FirstOrDefault(x => x.IsOfKind(CodeMethodKind.RequestGenerator) && x.HttpMethod == codeElement.HttpMethod) is not CodeMethod generatorMethod ||
codeElement.OriginalMethod is not { } originalMethod) return;
var parametersList = generatorMethod.PathQueryAndHeaderParameters.Where(static p => !string.IsNullOrWhiteSpace(p.Name)).ToList();

And generatorMethod.PathQueryAndHeaderParameters is set in KiotaBuilder.cs

private static void SetPathAndQueryParameters(CodeMethod target, OpenApiUrlTreeNode currentNode, OpenApiOperation operation)
{
var pathAndQueryParameters = currentNode
.PathItems[Constants.DefaultOpenApiLabel]
.Parameters
.Where(ParametersFilter)
.Select(GetCodeParameterFromApiParameter)
.Union(operation
.Parameters
.Where(ParametersFilter)
.Select(GetCodeParameterFromApiParameter))
.ToArray();
target.AddPathQueryOrHeaderParameter(pathAndQueryParameters);
}

In GetCodeParameterFromApiParameter(), the Type is being set by GetQueryParameterType(x.Schema)

private static readonly Func<OpenApiParameter, CodeParameter> GetCodeParameterFromApiParameter = x =>
{
var codeName = x.Name.SanitizeParameterNameForCodeSymbols();
return new CodeParameter
{
Name = codeName,
SerializationName = codeName.Equals(x.Name, StringComparison.Ordinal) ? string.Empty : x.Name,
Type = x.Schema is null ? GetDefaultQueryParameterType() : GetQueryParameterType(x.Schema),
Documentation = new()

And lastly, GetQueryParameterType(x.Schema) is setting the name of the type of the parameter by just using the type of the parameter and not using the format as it was done by the GetPrimitiveType()

private static CodeType GetQueryParameterType(OpenApiSchema schema) =>
new()
{
IsExternal = true,
Name = schema.Items?.Type ?? schema.Type,
CollectionKind = schema.IsArray() ? CodeTypeBase.CodeTypeCollectionKind.Array : default,
};

Hence the type is always string for the parameter of type 'string' and format 'date-time'
image

And it should give error for all the parameter types in which the type is "string" and format is something else which is creating a different type of Query Parameter in the ClientAPIClass

("string", "base64url") => "base64url",
("file", _) => "binary",
("string", "duration") => "TimeSpan",
("string", "time") => "TimeOnly",
("string", "date") => "DateOnly",
("string", "date-time") => "DateTimeOffset",
("string", "uuid") => "Guid",
("string", _) => "string", // covers commonmark and html

I tried one for "date" format and got the similar error.

Hope I am able to explain the issue well, and I feel like I have added lots of information (maybe even those which are straight forward) but I was not sure how to put it all together.

I have not looked for the solution yet but I am happy to work on it after the completion of the other issue and your insights will be very helpful.

PS: Sorry for the previous notification, it was a mis-click.

Thanks

@baywet
Copy link
Member Author

baywet commented May 13, 2024

Thanks for the detailed investigation.
I believe the reason why the option is a string is because system.commandline probably can only support a limited set of types. But we could validate using a date time offset and see if the system.commandline can parse it for us.
Alternatively, in your first screen, just wrapping with a DateTimeOffset.Parse shuold fix things.

@rohitkrsoni
Copy link
Contributor

Thanks, and yes that did work, I checked manually
image
I was thinking to add a check of format as well in the below method and set the name of type of the parameter based on that. (something similar to GetPrimitiveType()), so that we fix it for other types as well.

private static CodeType GetQueryParameterType(OpenApiSchema schema) =>
new()
{
IsExternal = true,
Name = schema.Items?.Type ?? schema.Type,
CollectionKind = schema.IsArray() ? CodeTypeBase.CodeTypeCollectionKind.Array : default,
};

And this will enable the generator to create options of the specific type rather than just string in BuildGetCommand()
image

I am just not sure about the effect of the change on the project!

@baywet
Copy link
Member Author

baywet commented May 14, 2024

Thanks for the additional information. For the option, the type is hardcoded here I think this is mostly because the system.commandline library doesn't support a lot of types when parsing without extensive configuration, but @calebkiage could confirm this.

I don't think you need to change anything in the main builder at this point. Here you should be able to navigate from the generator method, to the query parameters class, and then get the query parameter property which has the correct type if that makes sense?

@calebkiage
Copy link
Contributor

The System.CommandLine library supports for dates as option and argument types. So the code new Option<DateTimeOffset>(...) should work. @rohitkrsoni, I believe calling the GetPrimitiveType function to get the type name or updating the GetQueryParameterType function to use the same logic as GetPrimitiveType when getting the type name should work fine from the System.CommandLine PoV.

@fey101 fey101 added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label May 16, 2024
@rohitkrsoni
Copy link
Contributor

Thanks for the clarification. @calebkiage
Hi @baywet, should we proceed with the above suggestion?
thanks

@baywet
Copy link
Member Author

baywet commented May 17, 2024

yes, the more work we can delegate to the dependencies, the better.

@rohitkrsoni
Copy link
Contributor

Hi, here is the PR for the change, Please review it, thanks
#4679

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Work to support generating CLIs with Kiota priority:p0 Blocking issue. Loss of critical functions eg security/privacy violation. Bug SLA<=48hrs status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:bug A broken experience
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants