microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.48k stars 177 forks source link

Generated builder hickup #4814

Open CasperWSchmidt opened 1 month ago

CasperWSchmidt commented 1 month ago

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Nuget tool

Client library/SDK language

None

Describe the bug

I'm trying to generate a client for Umbraco 14 Media delivery API. I've created a project and opened the OpenAPI spec and pointed Kiota in the direction of it (https://localhost:44319/umbraco/swagger/delivery/swagger.json).

While generating the client it pops up with s few errors regarding uniqueness of the endpoints, this might be due to V1 and V2 being present? I don't know.

My main issue though, is that the generated client doesn't quite work as expected.

For the /items/ path I get a single ItemsRequestBuilder with a getAsync() method as expected.

For the /item/ path however, I get two ItemRequestBuilder classes, one nested within another namespace.

The first builder is located in namespace UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item and has only an indexer and some constructors:

namespace UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item
{
    /// <summary>
    /// Builds and executes requests for operations under \umbraco\delivery\api\v2\media\item
    /// </summary>
    public class ItemRequestBuilder : BaseRequestBuilder
    {
        /// <summary>Gets an item from the UcommerceTest.Integrations.Umbraco.umbraco.delivery.api.v2.media.item.item collection</summary>
        /// <param name="position">Unique identifier of the item</param>
        /// <returns>A <see cref="UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.ItemRequestBuilder"/></returns>
        public UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.ItemRequestBuilder this[Guid position]
        {
            get
            {
                var urlTplParams = new Dictionary<string, object>(PathParameters);
                urlTplParams.Add("%2Did", position);
                return new UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.ItemRequestBuilder(urlTplParams, RequestAdapter);
            }
        }
        etc.

As can be seen, the indexer returns a new instance of the very same class.

The second builder is located in namespace UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.Item (note the extra Item in the end compared to the first builder). This builder has a GetAsync() method as expected but is not returned from anywhere so I'm unable to actually make the call:

public class ItemRequestBuilder : BaseRequestBuilder
    {
        /// <summary>
        /// Instantiates a new <see cref="UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.Item.ItemRequestBuilder"/> and sets the default values.
        /// </summary>
        /// <param name="pathParameters">Path parameters for the request</param>
        /// <param name="requestAdapter">The request adapter to use to execute the requests.</param>
        public ItemRequestBuilder(Dictionary<string, object> pathParameters, IRequestAdapter requestAdapter) : base(requestAdapter, "{+baseurl}/umbraco/delivery/api/v2/media/item/{%2Did}{?expand*,fields*}", pathParameters)
        {
        }
        /// <summary>
        /// Instantiates a new <see cref="UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.Item.ItemRequestBuilder"/> and sets the default values.
        /// </summary>
        /// <param name="rawUrl">The raw URL to use for the request builder.</param>
        /// <param name="requestAdapter">The request adapter to use to execute the requests.</param>
        public ItemRequestBuilder(string rawUrl, IRequestAdapter requestAdapter) : base(requestAdapter, "{+baseurl}/umbraco/delivery/api/v2/media/item/{%2Did}{?expand*,fields*}", rawUrl)
        {
        }
        /// <returns>A <see cref="UcommerceTest.Integrations.Umbraco.Models.ApiMediaWithCropsResponseModel"/></returns>
        /// <param name="cancellationToken">Cancellation token to use when cancelling requests</param>
        /// <param name="requestConfiguration">Configuration for the request such as headers, query parameters, and middleware options.</param>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public async Task<UcommerceTest.Integrations.Umbraco.Models.ApiMediaWithCropsResponseModel?> GetAsync(Action<RequestConfiguration<UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.Item.ItemRequestBuilder.ItemRequestBuilderGetQueryParameters>>? requestConfiguration = default, CancellationToken cancellationToken = default)
        {
#nullable restore
#else
        public async Task<UcommerceTest.Integrations.Umbraco.Models.ApiMediaWithCropsResponseModel> GetAsync(Action<RequestConfiguration<UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item.Item.ItemRequestBuilder.ItemRequestBuilderGetQueryParameters>> requestConfiguration = default, CancellationToken cancellationToken = default)
        {
#endif
            var requestInfo = ToGetRequestInformation(requestConfiguration);
            return await RequestAdapter.SendAsync<UcommerceTest.Integrations.Umbraco.Models.ApiMediaWithCropsResponseModel>(requestInfo, UcommerceTest.Integrations.Umbraco.Models.ApiMediaWithCropsResponseModel.CreateFromDiscriminatorValue, default, cancellationToken).ConfigureAwait(false);
        }
        etc.

Expected behavior

Using my generated client I would expect to be able to make the following call: umbracoClient.Umbraco.Delivery.Api.V2.Media.Item[Guid.Parse(id)].GetAsync()

How to reproduce

Follow the installation instructions for Umbraco 14 here. Add the following snippet to appsettings.Development.json under "Umbraco:CMS":

      "DeliveryApi": {
        "Enabled": true,
        "PublicAccess": true,
        "Media": {
          "Enabled": true,
          "PublicAccess": true
        }
      }

Try to generate a client using Kiota while Umbraco is running on your machine: https://localhost:44319/umbraco/swagger/delivery/swagger.json

Open API description file

UmbracoOpenAPI.json

Kiota Version

1.15.0

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

No response

Known Workarounds

I guess I can manually copy the GetAsync into the "top-level" builder.

Configuration

Debug output

How to do that using VSCode plugin?

Other information

No response

baywet commented 1 month ago

Hi @CasperWSchmidt Thanks for using kiota and for reaching out. What I'm guessing is happening in your case is that the "item" path segment is getting confused by the "item" namespace name segment kiota inserts when projecting single parameters path segments (assuming it's dealing with a indexing in a collection). A quick way to check that would be to add a escaping mechanism here I believe. https://github.com/microsoft/kiota/blob/4e584d20be7aa0502cc35e2d4936a0dd6733e061/src/Kiota.Builder/Extensions/OpenApiUrlTreeNodeExtensions.cs#L15

-static x => x.IsPathSegmentWithSingleSimpleParameter() ? "item" : x;
+static x => x.IsPathSegmentWithSingleSimpleParameter() ? "item" : ("item".Equals(x, StringComparison.OrdinalIgnoreCase) ? "item_escaped" : x);

And see if that solves your issue. If so start opening a pull request and we'll help you drive it to completion.

CasperWSchmidt commented 1 month ago

Hi @baywet, and thanks for the quick reply!

Unfortunately it doesn't seem to work. Using the suggested fix generates the following two files:

image

The only difference is the folder/namespace for the outermost builder. However, this builder still returns it's own type:

/// <summary>Gets an item from the UcommerceTest.Integrations.Umbraco.umbraco.delivery.api.v2.media.item_escaped.item collection</summary>
/// <param name="position">Unique identifier of the item</param>
/// <returns>A <see cref="UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item_escaped.ItemRequestBuilder"/></returns>
public UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item_escaped.ItemRequestBuilder this[Guid position]
{
    get
    {
        var urlTplParams = new Dictionary<string, object>(PathParameters);
        urlTplParams.Add("%2Did", position);
        return new UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item_escaped.ItemRequestBuilder(urlTplParams, RequestAdapter);
    }
}

Trying to access an item using umbracoClient.Umbraco.Delivery.Api.V2.Media.Item[id] leads to a dead end (I can only continue indexing):

image
baywet commented 1 month ago

Right, I believe the request builder should also be name item_escaped, this should solve this last issue.

CasperWSchmidt commented 1 month ago

Any hints on where this should be changed? Also, why do you think it will solve the issue? It looks like it simply needs to add the additional namespace suffix to the type returned from the index method so I'm not sure it will help simply renaming the class?

baywet commented 1 month ago

I think you should be able to add it here. https://github.com/microsoft/kiota/blob/7c1c95ccbc037976f6f1742851440e969f8def00/src/Kiota.Builder/KiotaBuilder.cs#L603

The main idea here is that we're having collisions, and things are overwriting each other leading to this confused state. Buy tweaking how the conventions expand, we remove the collision with the "data" (paths segments), and we'll have two separate classes be generated with the right content each. Or at least, that's my hunch on this issue.

CasperWSchmidt commented 4 weeks ago

So I tried looking into this. Changing line 605 into this:

Name = currentNamespace.Name.EndsWith("item_escaped", StringComparison.InvariantCultureIgnoreCase) ? className.CleanupSymbolName().Replace("item", "item_escaped", StringComparison.InvariantCultureIgnoreCase) : className.CleanupSymbolName(),

I get the following structure: image This looks good because Item_escapedRequestBuilder has the following indexer signature:

public UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item_escaped.Item.ItemRequestBuilder this[Guid position]

So now I can get from the first to the second request builder. I'm not quite there yet though as the MediaRequestBuilder returns the wrong item request builder:

public UcommerceTest.Integrations.Umbraco.Umbraco.Delivery.Api.V2.Media.Item_escaped.Item.ItemRequestBuilder Item

So now my code is like this:

var test = kiotaClient.Umbraco.Delivery.Api.V2.Media.Item.GetAsync(cancellationToken: token);

(I can't use the indexer to pick the item based on GUID).

Looking at the following code:

// Add properties for children
foreach (var child in currentNode.Children)
{
    var propIdentifier = child.Value.GetNavigationPropertyName(config.StructuredMimeTypes);
    var propType = child.Value.GetNavigationPropertyName(config.StructuredMimeTypes, child.Value.DoesNodeBelongToItemSubnamespace() ? ItemRequestBuilderSuffix : RequestBuilderSuffix);

(KiotaBuilder line 616-645) there's no way of knowing that the child node name has been escaped (which is the reason for the bug above). So more work is needed where the children are added to a node. However, I can't figure out the right/best place to do this. Perhaps the escaped class shouldn't be considered an item request builder but a request builder? Btw., I think this _escaped naming scheme looks like a big hack that nobody wants to see in their real codebase when using the generated client 😄 Isn't it better to keep track of the classes generated

baywet commented 4 weeks ago

Thanks for the continuous work on this. Can you open a draft PR please? I'm having a hard time understanding the nature of the changes at this point.

CasperWSchmidt commented 3 weeks ago

Hi @baywet sorry for the late reply, I've been out sick for almost a week 😞

I'll make a draft PR with my changes ASAP.

CasperWSchmidt commented 3 weeks ago

Draft PR created 😄