search

microsoft / kiota

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

How to pass a free-format query string? #3800

Open bkoelman opened 9 months ago

bkoelman commented 9 months ago

Due to the dynamic nature of JSON:API, which I'm trying to generate a C# client for, we need the query string parameters to be unconstrained. Is there a way to do that with Kiota?

The C# Web API produces the following OAS, which results in an IDictionary<string, string?> query method parameter using NSwag:

"parameters": [
  {
    "name": "query",
    "in": "query",
    "description": "...",
    "schema": {
      "type": "object",
      "additionalProperties": {
        "type": "string",
        "nullable": true
      },
      "example": null
    }
  }
]

When passing the following value to the NSwag-generated method:

new Dictionary<string, string?>
{
    ["filter"] = "has(assignedTodoItems)",
    ["sort"] = "-lastName",
    ["page[size]"] = "5"
}

it gets translated to:

?filter=has(assignedTodoItems)&sort=-lastName&page[size]=5

Kiota generates the following instead:

[QueryParameter("query")]
public string? Query { get; set; }

It's unclear to me how to pass a query string to the generated code that produces: ?filter[node.parent.parent.parent...]=equals(name,'X') because it always emits ?query=....

The produced OAS is not set in stone, though it would be preferable if it works with both NSwag and Kiota.

baywet commented 9 months ago

Hi @bkoelman Thanks for your interest in kiota and for reaching out. I'm not super familiar with JSON API, but from what I understand query parameters can be separated in two main categories:

  1. conventions from the standard that are known in advance, regardless of the underlying data.
  2. conventions from the standard that vary based on the underlying data.

Examples of the first category include sort, filter, page (size, offset, limit...). There should be described as first class query parameters in the OpenAPI description so everyone relying on OpenAPI with their tooling can benefit from it.

For the second category, I guess it depends on how this description gets created. If there's a single description for all instances of JSON API, then it's going to be hard to add them without complex layering. If the JSON API instance can generate the description from the underlying data, it could also include those other query parameters in the description, and a client should be generated from this "augmented" description instead.

Regardless, I believe parameters you added as part of your initial comment is wrong. I think the intent here was to describe "we can have any query parameter of type string". But this actually describes "there's one query parameter called query, which can have any property of type string". In addition to this wrong aspect of the description, Kiota does NOT support object query parameters as their serialization conventions are not standardized.

Lastly, for the dynamic query parameter, you could use your own class even though it's kind of a hack

// example from https://jsonapi.org/format/#fetching-sparse-fieldsets
public class CustomQueryParameters: BazGetQueryParameters { //alternatively, this could inherit from the default query parameters if no class is generated
    [QueryParameter("fields[articles]")]
    public string ArticleFields {get; set;}
}

var result = await client.Foo.Baz.GetAsync(x => {
    x.QueyParameters = new CustomQueryParameters {
       ArticleFields = "title"
    }
});
bkoelman commented 9 months ago

Hi @baywet, thanks for your response.

I'm not super familiar with JSON API, but from what I understand query parameters can be separated in two main categories

Unfortunately, it's not that simple. When I first came in contact with JSON:API, I found it hard to fully understand. Since then, I've been a maintainer of JsonApiDotNetCore (the most popular server implementation in .NET, there are many in various languages) for several years, have contributed major parts of it and answered hundreds of questions from users. By now, I'm pretty confident I know it very well, so please allow me to help you understand the problem.

JSON:API defines a format for REST APIs, conceptually similar to OData and GraphQL. At its core, an API consists of resource types and fields, where a field is either an attribute or a relationship. Resource types can be compared to database tables, attributes to their columns, and relationships to foreign keys (roughly). In EF Core terminology, a resource is an entity, an attribute is an entity property or scalar value, and a relationship is a navigation.

JsonApiDotNetCore enables developers to expose an EF Core model through JSON:API by annotating their EF Core models with [Attr] (an attribute), [HasOne] (a to-one relationship), and [HasMany] (a to-many relationship).

Consider the following EF Core entity, which has been annotated for JSON:API:

public class Human : Identifiable<long>
{
    [Attr]
    public string Name { get; set; } = null!;

    [HasOne]
    public Human? Father { get; set; }

    [HasMany]
    public ISet<Human> Children { get; set; } = new HashSet<Human>();
}

Based on the model above, JSON:API enables to side-load related resources, for example:

GET /api/humans/1?include=children,father.father.children HTTP/1.1

which returns a compound document that contains (assuming data exists):

This request can be combined with filters at multiple depths (targeting relationship chains ending in a to-many relationship), for example:

GET /api/humans/1
?include=children,father.father.children
&filter[children]=equals(name,'Joe')
&filter[father.father.children]=equals(name,'Jack') HTTP/1.1

The difference with the previous request is that this one filters on the sets from the 2nd and last bullet of the returned compound document.

As you can see, the query string parameter name (filter[father.father.children]) depends not only on the model (which is known at startup), but also on the dot-separated chain of inclusions of the current request. Even if we wanted, we couldn't explode all possible combinations in the generated OAS because it can be infinitely recursive (father.father.father.father.father....). That's why we need free-format query string parameter names. Because of this, your proposed hack won't work. Even if we'd plug into Kiota somehow to generate these, we'd face the same infinite recursion problem.

The OAS generation logic within JsonApiDotNetCore lives in a separate branch, built by a former team member using lots of Swashbuckle extensions, but the prototype was never completed. As you may have realized by now, we already tailor the produced OAS to the specific models being used (except for query strings). I'm firm on completing the OAS implementation, because OAS support is our number-one feature request. Though I'm a bit concerned that our existing integration tests using NSwag are leading to a dead end ultimately, because the primary maintainer of NSwag isn't active anymore.

So I thought I'd give Kiota a try. Its code looks clean, is documented and the project is active. Using query strings was just the first issue I ran into, and I doubt it will be the only one. I'm curious if we can work together and find ways to address them. If all goes well, I'd be happy to drop support for NSwag in favor of Kiota, but it's too early for such a decision.

Regardless, I believe parameters you added as part of your initial comment is wrong. I think the intent here was to describe "we can have any query parameter of type string". But this actually describes "there's one query parameter called query, which can have any property of type string".

Yes, that's the intent. I'm still learning OAS (read your series, which is great). The OAS fragment in the first post originates from a stack overflow answer on how to do it with NSwag, which appears to work. I haven't found any other way to make it work with NSwag, which is what we use today. As you said, many things are not standardized in OAS. Would you consider special-casing this OAS structure in Kiota for the sake of compatibility?

baywet commented 9 months ago

Hey @bkoelman, Thanks a lot for the detailed rundown here, it helps frame the issue. Now I see how "deriving from the query parameters class" won't help as you're maintaining the description itself, and you don't want every client to implement that hack.

I'm curious if we can work together and find ways to address them

Absolutely.

Would you consider special-casing this OAS structure in Kiota for the sake of compatibility?

Probably not, here is the rationale for it: Kiota is built by the Microsoft Graph client experience team. Microsoft Graph itself is an OData-ish service. When we started kiota, one of our core principle was to make sure it had "nothing to do" with OData, so it could be used by the broader base of the OpenAPI ecosystem. All the OData conventions are pushed to OpenApi.net.OData. Maintaining that commitment through the years (which was hard at times) has two major benefits: kiota can be used against any REST API with an OpenAPI description, the OData to OpenAPI conversion story got much better. Embedding conventions from another code generator which is not maintained anymore, while making migrations easier, would go against that principle. So would making changes that are specific to JSON-API APIs (outside of general OpenAPI feature support).

If we look at the value of code generation of clients for REST APIs, it is mostly to "guide" client application developers through auto-completion, compile-time validation, etc... I'm not sure how a blanket "this endpoint accepts any query parameter" helps anyone here. (which was the intent of the snippet you provided).

Instead if we look at that request

GET /api/humans/1
?include=children,father.father.children
&filter[children]=equals(name,'Joe')
&filter[father.father.children]=equals(name,'Jack') HTTP/1.1

Maybe we could stage things instead of trying to address it all at once, and drive things based off the community demands. I think there are now 3 cases:

  1. convention based query parameters that do not change from API to API: these should be in the description so everybody knows about them, including code generation tools.
  2. convention based query parameters that depend on the model, and are recursive (e.g. filter[father.father.children]): they could be projected by the OpenAPI description generation logic as well. They bring on two challenges: (a) avoiding the description to get too verbose, and (b) avoiding cyclical expansion (a problem we have for paths in OData).
  3. convention based query parameters that depend on the model, but are not recursive by nature (e.g. filter[children]): this is effectively the same case as 2, with a recursion limit of 1.

Here my suggestion is to implement 1 and 2 (and get 3 for free). For concern 2a you could have a setting for the depth, default it to 1 or 2 levels, and wait for feedback from the community, adjust plans from there. For concern 2b while not trivial, loop detection is easily solved with a stack or a map, which will bring on some limitations (typically getting the grand father in your example, or the father's children).

For those limitations, we could consider adding an "Arbitrary map of query parameters" in the base query parameters class but only after scenario 1 and 2 have been implemented and if there's strong customer demand for it. The reason why I'm reticent to add it by default is because I'd rather have people improve and fix their descriptions (or reach out to the API provider to do so) than having them use this as an escape hatch all the time and loose some of the value kiota can provide for them. Also, kiota makes use of RFC 6570, and if the query parameter is not described in the URI template, the passed value will be ignored during expansion, so we'd have to somehow "inject" those arbitrary parameters into the template, which could lead to reliability trade-offs.

What do you think of this plan?

darrelmiller commented 9 months ago

Kiota depends on URI Templates (RFC 6570) for resolving URLs. The way to have dynamic query parameter names with URI templates is to use a parameter that is a map and "explode" it. e.g.

/foo{?params*}

If the params value is a dictionary then the keys of the dictionary become query string parameter names and the values in the dictionary become query parameter values. I think we could squint an describe this in OpenAPI. We need it described in OAS before we can address the issue with Kiota.

bkoelman commented 9 months ago

Probably not, here is the rationale for it...

That makes sense. You've convinced me that's the better decision for Kiota. Is there a way to plug this into Kiota through an external NuGet package? I'm asking because both NSwag and SwaggerUI work with this syntax.

If we look at the value of code generation of clients for REST APIs, it is mostly to "guide" client application developers through auto-completion, compile-time validation, etc... I'm not sure how a blanket "this endpoint accepts any query parameter" helps anyone here. (which was the intent of the snippet you provided).

Yes, discovery and documentation are the primary reasons we started the OpenAPI effort. My initial thought was to apply the same principle for query string parameters. Until I started implementing it. The difficulty is not in traversing the relationship graph or detecting recursion. JsonApiDotNetCore already has a setting to constrain the maximum inclusion depth (amongst other constraints), which enables API owners to keep database pressure under control. But the default is unconstrained, for backward compatibility.

Here's the syntax for the query string parameter names:

filterOrSort := ( 'filter' | 'sort' ) ( '[' relationship* to-many-relationship ']' )?
pagination := 'page' ( '[size]' | '[number]' )
inclusion := 'include'
sparseFieldSet := 'fields' '[' resource-type ']'

Consider the following model, which we use in our samples:

If we'd generate all the possible query string parameter names, assuming a maximum inclusion depth of 3, it would result in:

As can be seen above, it gets completely out of control, even in an unrealistically small sample API with only 3 resource types and 6 relationships. Already with this sample, SwaggerUI becomes completely useless (users need to scroll forever). So in my opinion, pointing users to the mechanism of how these parameters work (see the screenshot in https://github.com/json-api-dotnet/JsonApiDotNetCore/pull/1378) is the better solution, from a usability perspective. I wonder how helpful an auto-completion list with thousands of entries would be, aside from the increased generation and compilation effort.

We've only covered the parameter names. The parameter values for filter/sort take compound expressions. My plan is to release untyped query string parameters in the MVP. Then offer additional building blocks (in the form of JSON:API client packages) to compose these parameters and expressions in future releases. The primary goal is to enable OAS, gather feedback, and then improve the experience.

For concern 2a you could have a setting for the depth, default it to 1 or 2 levels, and wait for feedback from the community, adjust plans from there.

The first version of JsonApiDotNetCore had many undocumented limitations, such as "filtering only works one level deep" and "sorting does not work on nested endpoints, such as /blogs/1/posts". Newcomers were initially excited about the project, then quickly became disappointed because they were falling off a cliff all the time. Over time, we've refactored to address all of these. I'm glad the project is in great shape now, so I hope you understand I feel hesitant about your proposal. I don't want to return to that sentiment of "it only works in simple cases".

For those limitations, we could consider adding an "Arbitrary map of query parameters" in the base query parameters class but only after scenario 1 and 2 have been implemented and if there's strong customer demand for it. The reason why I'm reticent to add it by default is because I'd rather have people improve and fix their descriptions (or reach out to the API provider to do so) than having them use this as an escape hatch all the time and loose some of the value kiota can provide for them.

In our case, API providers cannot fix their descriptions, because we auto-generate them. If developers decide to use this as an escape hatch, I'm going to assume they do that intentionally (like me). It's still better than not using Kiota or even OpenAPI at all. Your sentiment, though well intended, comes across to me as treating developers as lazy or stupid: "We know what's best for you, so we won't let you." I believe a great framework should empower developers, not try to hold them back. It's appropriate that it takes extra effort to do something that's not recommended, though. So I'm okay if this feature is disabled by default, and logs a warning when used (that can be turned off).

I'd be interested in the OAS shape that Kiota needs to potentially enable free-format parameters, so I can verify if SwaggerUI understands it and see what NSwag makes of it.

bkoelman commented 9 months ago

What I forgot to mention is that JsonApiDotNetCore has a setting to allow unknown query string parameters to be passed through. When enabled (false by default), API developers can implement a hook to handle their custom parameters. Some devs use it to expose functional-style filters, for example: ?isPremiumAccount or ?hasRecentlyLoggedIn, others use it for debugging or unit-testing, such as ?disableCache=true, ?forceAuthUserId=123, etc.

baywet commented 9 months ago

Thank you for the detailed analysis of paths here. I agree with you that'd be verbose while still posing some limitations. From your example, it seems the only "reasonable" depth for code generation would be to support only 1 level of depth. Which would cause severe limitations if support arbitrary query parameters is not provided.

Question about the hooks: are they registered somewhere? meaning do you have a way to enumerate them so they could also be projected to query parameters?

comes across to me as treating developers as lazy or stupid

I'd like to address this comment head-on so there's no misunderstanding here. The intent is never to be pedantic and shouldn't be perceived as so. But rather to try and iterate on solution designs that lead to better practices across the ecosystem. The thinking process is never "I know better than the rest of you" but rather "let's design together solutions that are practical from an experience perspective while being efficient for the industry". Hopefully this clarifies the tone of some of my comments for future references, as async communications can be lossy at times. 🙂

@darrelmiller considering the RFC 6570 supports the explode scenario with maps, and considering that OAS already support describing the following snippet (the name of the parameter doesn't matter here)

"parameters": [
  {
    "name": "query",
    "in": "query",
    "description": "...",
    "schema": {
      "type": "object",
      "additionalProperties": {
        "type": "string",
        "nullable": true
      },
      "example": null
    }
  }
]

Do you see any reason why we could project this as 

[QueryParameter("query")]
public Dictionary<string, string>? Query { get; set; }

and

{baseUrl}/foo/bar{?query*}

With usage looking like

var result = await client.Foo.Bar.GetAsync(x => x.QueryParameters.Query = new() {  {"filter[ownedTodoItems.owner.ownedTodoItems]" : "equals(priority, 'high')" }});

Which would result in 

GET https://localhost/foo/bar?filter[ownedTodoItems.owner.ownedTodoItems]=equals(priority, 'high')
darrelmiller commented 9 months ago

If someone creates a parameter that is style=form and explode=true and defines a schema that is an object, then yes, we definitely could project that as a dictionary<string,string>.

We could do better than Dictionary<string,string> if we use the style "deepObject". OAS was never able to be explicit as to how to serialize a "deepObject" in a query string. However, we could have a default opinion that looks like what JSONAPI does and use an extension for alternative serializations if they show up.

baywet commented 9 months ago

Thanks for the additional details @darrelmiller I was basing the type of the value on the additionalProperties type, so the full parameter description would be 

"parameters": [
  {
    "name": "query",
    "style": "form", // what do we do with other styles in that scenario?
    "explode": true, // what do we do if it's false?
    "in": "query",
    "description": "...",
    "schema": {
      "type": "object",
      "additionalProperties": { // my assumption was that if additionalProperties is absent here, we project as a "regular property" (i.e. not a map)
        "type": "string", // what do we do if this is anything else than scalar? should we implement scalar types only? as a first stop-gap?
        "nullable": true
      },
      "example": null
    }
  }
]
bkoelman commented 9 months ago

Question about the hooks: are they registered somewhere? meaning do you have a way to enumerate them so they could also be projected to query parameters?

Unfortunately, no. It is a virtual method that users can override, which executes at request time. So the set may not be constant, but depend on the current endpoint, the logged-in user, etc.

I'd like to address this comment head-on so there's no misunderstanding here.

Thanks, no worries.

I was basing the type of the value on the additionalProperties type, so the full parameter description would be

Here's our code to produce that:

operation.Parameters.Add(new OpenApiParameter
{
    In = ParameterLocation.Query,
    Name = "query",
    Style = ParameterStyle.Form,                // added for Kiota
    Explode = true,                             // added for Kiota
    Schema = new OpenApiSchema
    {
        Type = "object",
        AdditionalProperties = new OpenApiSchema
        {
            Type = "string",
            Nullable = true
        },
        Example = new OpenApiNull()
    },
    Description = "..."
});

which uses the object model from https://www.nuget.org/packages/Microsoft.OpenApi to render the OAS. For reasons unknown to me, the resulting OAS file differs from your proposal in that "explode": true isn't being written. I've tried with the latest version of Microsoft.OpenApi. If I change my code to Explode = false, then the property does get written. So perhaps requiring it to be true in OAS is redundant?

image

Other than that, I can confirm this structure works with both SwaggerUI and NSwag.

baywet commented 9 months ago

Thanks for sharing your progress here. Can you open a separate issue about the explode thing at https://github.com/microsoft/OpenAPI.net and mention me please so we can get to the bottom of that? And eventually release a patch.

bkoelman commented 9 months ago

Created https://github.com/microsoft/OpenAPI.NET/issues/1489.