Weakly typed path parameters and nullable response properties (C#)

[kimbirkelund]

I'm trying to use Kiota to generate a client for an internal API, but I'm running into a few issues that I can't seem to find documentation for - either in the actual documentation or previous issues, but I apologize if I'm just not looking hard enough :)

I'm using the following spec snippet for the issues below but it happens in samples as well:

"/foo/{fooId}": {
  "get": {
    "parameters": [
      {
        "name": "fooId",
        "in": "path",
        "required": true,
        "schema": {
          "type": "integer",
          "format": "int32"
        }
      }
    ],
    "responses": {
      "200": {
        "description": "OK",
        "content": {
          "application/json": {
            "schema": {
              "$ref": "#/components/schemas/FooResponse"
            }}}}}}}

"FooResponse": {
  "required": [
    "fooId"
  ],
  "type": "object",
  "properties": {
    "fooId": {
      "type": "integer",
      "format": "int32"
    }
  },
  "additionalProperties": false
}

In the generated client I see the following issues:

1) In the request builder the indexer parameter is string position although the spec clearly states that it is an integer named fooId.

2) The generated WithFooItemRequestBuilder.GetAsync returns Task<FooResponse?> even though spec says it always returns a response. I could understand if this was to map 404 to null, but this does not appear to be the case.

3) In the FooResponse model the property for fooId, which in the spec is a required integer, is also nullable and this appears to be the case for all properties.

From what I've read Kiota is intended to generate easily usable clients from the consumer point of view so consistency is more important than tailoring the client to something the producer would like.

I'm fully on board with this perspective, but these issues, to me, does not align with that. So I'm curious as to why these choices were made.

[baywet]

Hi @kimbirkelund Thanks for your interest in Kiota and for reaching out.

gaps in documentation

This is something we're aware of and we're working to address it. In the meantime we encourage people reaching out to outline specific gaps and get their questions answered.

indexer argument type

Arguably this could be classified as a bug/feature gap. If you had multiple path parameters in the same path segment you'd instead get a method with multiple parameters. And those parameters would be properly typed. This is somewhat related to #2318. Reasons for that are somewhat historical, indexers were added at the beginning of kiota's journey, we haven't revisited them much since then, and the API we were using to test things (Microsoft Graph) probably only ever has string path parameters for indexers.

I think we could make that change safely, although it might be a breaking change to some people, the impact would be limited. As for the name, I'm not sure the name of the indexer parameter really matters? But I'd be happy to read your thoughts on that topic. Would you be willing to submit a pull request for the indexer type?

nullable return types

Our experience with descriptions out there is they are often lacking enough information around that. Not only people would need to be explicit about 200 response and its nullability, but they'd have to make sure the API respects that. Also, maybe for post/put/patch... and not for get, you have to account for 204's that are often left undescribed and other cases like that which are fairly common. To provide consistency and reliability for inaccurate descriptions we made that opiniated decision.

nullable properties on models

The reason why kiota is opiniated this way is because most descriptions don't consider the client experience with nullability and selection. In lots of cases even though "property foo is required", the client can still provide a selection that doesn't include that property, and the payload won't contain it, which means it might be null at runtime. Also required is not specific enough, is it required in the context of getting the resource? in the context of creating it or updating it? etc... There is design work going on at the standards level to address that gap. Once it's addressed there, we can imagine improving the experience in kiota.

I hope that lengthy post answers your questions, don't hesitate to follow up with additional questions or remarks.

[darrelmiller]

Fixing this would be good but it is a breaking change so I have moved it into our v2 milestone.

[kimbirkelund]

I haven't the time right now to make a proper answer. I just Ed wanted to say that I'd be happy to try to fix the indexer issue.

I was actually starting to look at it but couldn't find where the code for the language specific generators were located.

If you could point me in the right direction I'll try and put a PR together.

Also thanks for a cool tool and a quick reply :-)

[baywet]

unfortunately we can't fix this now as this would represent a breaking change to existing clients as Darrel was mentioning. We'll get to it whenever we start working on V2

[kimbirkelund]

Hi again,

Finally got the time for a more thorough answer :)

indexer argument type

As mentioned I'd be happy to take a crack at it. Even if it isn't something that can be released anytime soon. However it wouldn't need to be a breaking change. Indexers support overloading, so we could simply add a strongly typed version with the correct name - if the ID is actually a string then we just fix the name. Fixing the name would be a breaking change for anyone specifying the parameter name though - so maybe that should only be done for the added indexers. The current index could then be marked as deprecated if you'd like.

Regarding fixing the name I actually think it would help discoverability a lot. Looking at a name like position and having to make the leap to whatever the name of the ID of the current resource is, isn't exactly intuitive.

However I'm inclined to say that using indexers like Kiota does is neat but not intuitive or discoverable. But that is obviously just my opinion, and maybe recognizability from client to client is enough to outweigh any discoverability issue that may exist.

nullable return types and nullable properties on models

Our experience with descriptions out there is they are often lacking enough information around that. Not only people would need to be explicit about 200 response and its nullability, but they'd have to make sure the API respects that. Also, maybe for post/put/patch... and not for get, you have to account for 204's that are often left undescribed and other cases like that which are fairly common. To provide consistency and reliability for inaccurate descriptions we made that opiniated decision.

The reason why kiota is opiniated this way is because most descriptions don't consider the client experience with nullability and selection. In lots of cases even though "property foo is required", the client can still provide a selection that doesn't include that property, and the payload won't contain it, which means it might be null at runtime. Also required is not specific enough, is it required in the context of getting the resource? in the context of creating it or updating it? etc... There is design work going on at the standards level to address that gap. Once it's addressed there, we can imagine improving the experience in kiota.

I see your point, but doesn't that argument lead to not being able to infer anything? I'd prefer to error on the side of following the spec and let that be a call to action for fixing the spec and implementation.

If the publisher of the spec and client is the same (like in my case) then either I would fix the spec (and thus the client) to make the best UX for my consumer. Or I wouldn't bother with a generic generator because my API was too custom.

On the other hand if I, as the consumer of an API, roll my own client using Kiota I would like options to pick and choose.

The way nullable references are supported currently is like if the .NET team had enabled nullable reference for the CLR and marked all reference types as nullable. Sure it is technically true, but I'd that hadn't bothered then.

Nullable references in .NET does not ensure that what you get is never null, it just indicates that it is outside the spec if it is. Likewise with the OpenAPI spec - there is no guarantee but it is to be considered an error if a route says it produces a body and it doesn't.

In my opinion this would be a good candidate for having choices regarding what to generate:

• Respect the nullability and required parts of the spec or
• Mark everything as optional/nullable or
• Mark nothing as optional/nullable

Obviously this increases the complexity of Kiota quite a bit, but it would make the UX a lot better too. Currently we're not going to adopt Kiota because of the nullability issue. We've spend a good amount of effort on making our specs reflect the actual behavior of the API, and our current Refit based approach reflects that better.

[baywet]

Thanks for the detailed response.

indexers

That could be a good compromise. Some of the underlying code in the code-dom would need to be tweaked a little (right now we limit things to one indexer per class, if more it get's automatically converted to methods).

However I'm inclined to say that using indexers like Kiota does is neat but not intuitive or discoverable. But that is obviously just my opinion, and maybe recognizability from client to client is enough to outweigh any discoverability issue that may exist.

Are you saying that's indexers are hard to discover by themselves or that the fact the parameter is not named properly hinders discoverability?

NRT

Our assumption is that the number of people using kiota to generate a client who have limited knowledge of OpenAPI or are not the API producers themselves (can't tweak the description) will outweigh the other segment of the group. So far this assumption seems to be holding, but we'll see with time whether we need to review that.

There is on going work in the specs (JSON schema, OpenAPI) to improve nullability and requirement aspects so API producers can be more precise in what they express, allowing client code generators to produce more precise code.

We thought about adding a switch for NRT on the CLI interface. But this is a problem that's really specific to CSharp (and maybe TypeScript), and if we start going down that route, we'll end up with tons of switches that are language specific, or worst yet, using templates. Which are core design principles we decided to stay out of when starting kiota. The reasons being: keep the tool as simple as possible, reduce maintenance tax and maintain great performance.

[kimbirkelund]

indexers

That could be a good compromise. Some of the underlying code in the code-dom would need to be tweaked a little (right now we limit things to one indexer per class, if more it get's automatically converted to methods).

Let me known if you'd like me to try and make it work. I'd still need a pointer as to where to begin :)

Are you saying that's indexers are hard to discover by themselves or that the fact the parameter is not named properly hinders discoverability?

The first. Indexers, if I'm not mistaken, don't show up in IntelliSense unless you type a [ and personally I generally don't do that when exploring a new API. I had to consult the docs when starting with the Microsoft Graph API to figure out how to get a single user.

NRT

Our assumption is that the number of people using kiota to generate a client who have limited knowledge of OpenAPI or are not the API producers themselves (can't tweak the description) will outweigh the other segment of the group. So far this assumption seems to be holding, but we'll see with time whether we need to review that.

That is totally fair and from that point of view it makes sense.

We thought about adding a switch for NRT on the CLI interface. But this is a problem that's really specific to CSharp (and maybe TypeScript), and if we start going down that route, we'll end up with tons of switches that are language specific, or worst yet, using templates. Which are core design principles we decided to stay out of when starting kiota. The reasons being: keep the tool as simple as possible, reduce maintenance tax and maintain great performance.

I would think that a strict vs relaxed mode could make sense in most languages (I've only tried .NET clients). But wanting to do one thing great instead of doing a lot of things badly is absolutely the better choice.

[baywet]

indexers

Thanks for the feedback, we assumed indexers were making things easier for dotnet developers. FYI the do show up for auto-completion in vscode, I'm not sure about VS classic.

@darrelmiller what do you think about emitting two indexers in that case : the string one marked as obsolete for compatibility and the "correct type" one in addition?

NRT

Thanks for the open discussion. We'll re-evaluate that aspect as the next versions of JSON schema and OpenAPI come out and we implement support for them.

[baywet]

Alright, I got a green light from Darrel on the approach for indexers. Moving this to 1.3 (no the upcoming release, the next one)

In terms of pointers:

• While searching for pointers I noticed we missed something before the GA. Captured here and I believe you'll need points 2 and 3 for this change
• The code that emits the indexer lives here and the related test class lives here
• The code that handles moving from one indexer to two methods lives here it'll need to be updated so it only triggers when both indexers are NOT deprecated. Associated test code
• The code adding the indexer into the DOM lives here and you can call this method to get the proper type. test code lives in this test class

Let me know if you have further questions and thanks in advance for the contribution!

[kimbirkelund]

indexers

Thanks for the feedback, we assumed indexers were making things easier for dotnet developers. FYI the do show up for auto-completion in vscode, I'm not sure about VS classic.

I stand happily corrected then :-) Also it's not like I have a better suggestion.

Alright, I got a green light from Darrel on the approach for indexers.

Great. I'll have a look as soon as possible.

Where did you land regarding renaming the parameter in the case where the ID is actually a string and we thus keep the current indexer?

[baywet]

Thanks for double checking on that aspect. People can technically to the following

gitHubClient.Repos[position: "baywet"]["demo"].Pulls.GetAsync();

Even though I've never seen it. So I'm guessing changing the name for the string version (whether or not a non-obsolete version exists) would technically a breaking change. Adding a condition to maintain compatibility would be required. And please add a big TODO comment saying it should be removed for v2.

[kimbirkelund]

• While searching for pointers I noticed we missed something before the GA. Captured here and I believe you'll need points 2 and 3 for this change

2605 is about marking routes as deprecated in the generated client when they are deprecated in the spec, is it not?

I'm not sure I get how that relates to this? Unless I'm mistaken this feature should only affect the CodeIndexerWriter.

[baywet]

Correct. The reason why I'm mentioning this one here is because the infrastructure to carry the deprecation across the code DOM and to write the deprecation information in the generated code is going to be mutualized and I'd like to avoid having to refactor some of the changes you're about to make if we can get things right.

[kimbirkelund]

Right. I was just considering the C# case and adding it there. But obviously adding the overloads at the general DOM level is a better solution.

[LockTar]

Is it an idea to split this issue in two issues?

We have the same problem/discussion about Nullable Return Types (NRT) as well. We use C# and we are the owner of the spec and of the client. We have a very strict setup in C# that everything is correctly tested and nullable types are defined well.

Example 1 We have a post api (and open api spec) with al non nullable properties. The client now generates a class for the post with all nullable properties. This gives confusing to the people that are using the client.

Example 2 I have an API that will get an order based on it's unique identifier. We have responses for 200, 401, 403 and 404. The 200 response is now generated as nullable but we only return a 200 when the order is found (exists). If it's not found it will return a 404. If you don't have permissions you will get a 403 etc. The 200 should be not nullable...

[baywet]

Thanks for joining the conversation. Again, the OpenAPI spec is not precise enough to make that kind of call today. For the same exact model, for the same exact property, it could be nullable when getting it, but required when creating or updating it. With the second example. What happens if an older application, with an older client (non nullable in that context) suddenly starts getting 204's? (because the API changed). It'll break.

On the NRT front we're probably not changing anything until the spec evolves to exhaustively describe those cases.

On the deprecation front, I forgot a PR was open (sorry), and I started working on the issue. (almost done, I expect the PR to be open in the next few days). Which should give us a strong base to implement "more precise indexers".

[LockTar]

Hi @baywet, Thank you for the reply.

I understand your point of view about a model that can have properties that have sometimes required properties and sometimes not. I also understand about older applications.

This is the case in example the v5 SDK of the Microsoft Graph. Let's in example use the "Users" part of the Microsoft Graph. You can in example:

1. Get a list of users
2. Get a user by it's object id.
3. Create a user by a user object.

Maybe I'm to short in my answer but I see it as follow. And again I get everything above :-).

1. Getting a list of users should never return null. It should always return an empty list. So a nullable return type of a 200 response should not be possible.
2. Get a single user should never return a nullable return type. Because if the user doesn't exist it should return a 404 response. A User? response is then strange.
3. The creating part is different because that really depends how you setup your API. In the example of the MS Graph Users it will (re)use the same User model. There can be nullable properties inside.

But let me explain what we have in our API's: Getting data is easy. So when we have a list or getting data based on it's unique identifier it will never return a nullable type. It will return an empty list or a 404 as explained above. That's why I really liked the idea of respecting the response type in the spec by a 200 result.

For creating a resource we use a different approach. There are not really many use cases when you post complete resources. When you have very small resources it will be fine but sometimes you have larger resources and then it will be difficult. Same is for updating in example the status of an order or adding a (MS Graph) User to a Group. We like more the "action" post model. We use CQRS so we have commands. Just like adding a user to a group or setting the status of an order. In that way we can set the security very strict, is the code easier to understand and there are fewer bugs. Yes there are maybe more actions/api calls on a resource but it's for now always les then 5 different calls. Because we use commands, our validation is very strict as well. We know what is nullable, our properties in the commands are nullable when they really can be nullable. So when I now generate a client with this awesome tool kiota I get an action: Orders/SetStatus where the input is a status. But this status is a nullable type but this is strange because it's of course required.

For the point of older applications with an older client (non nullable in that context) suddenly starts getting 204's? (because the API changed). It'll break. I think this is a different problem. I think (again as reference with the MS Graph) that versioning of the nuget package is already solving this. But I think the Microsoft Graph API it self is here the problem. If you look at API's like Azure Resource Manager or Azure DevOps, they all have versioning. Also Azure API Management supports versioning. So changing the signature of an API with breaking changes (how many times are we really introducing breaking changes in a mature API) should result in a new version. This will result in a new version of the Graph Client nuget package. I have setup many different API's in my live for multiple enterprises and this always worked...

If you use the above setup, the Microsoft Graph security can be more precise as well. I have a lot of scenario's where I automated a process where I needed to create in example an Azure AD security group. But in order to do this, I also need to have to delete groups. This is of course very dangerous in an enterprise....

[baywet]

@locktar thanks for the detailed response here.

Yes, there are cases where we could have been smarter with nullable aspects. Generally speaking I think you're making assumptions that are based on odata semantics in your examples, but one design principle of kiota was to not rely on any odata aspects, only open API so it can be useful to more APIs. One such example is post accepting a nullable value for the request body. Even with odata semantics you can post null to a reference on a navigation property to remove the link between two entities.

The Microsoft graph versioning policy doesn't explicitly call out adding a new http response to an existing endpoint as a breaking can. It generally considers additions as non breaking. Under such conditions we need to consider that kind of scenario and design things in a way that'll avoid applications breaking in production because of an outdated client and an API change.

As for the creation and properties scenario, having a specification that allows for a more precise description would allow us to project types with more precision.

Generally speaking when we started working on kiota, we quickly realized the specification was ambiguous on nullability, lots of descriptions out there are not using it right. Therefore we designed a tolerant approach rather than a stricter one. The growth we're seeing would support the decision.

Of course that doesn't mean things are set in stone. The plan right now is to have the standard evolve so it can carry unambiguous information, and then major Rev kiota while taking advantage of the new capabilities.

@darrelmiller is working on open API v4 with those considerations in mind and the feedback provided here is paramount to designing the right thing.

[darrelmiller]

Thank you to everyone for this valuable conversation. This kind of feedback really helps us in making decisions that often don't have easy answers.

Regarding the choice to always return nullable types, I want to mention a little bit about OpenAPI philosophy and evolvability. Many of the design choices in HTTP are intended to reduce client/server coupling to enable APIs to evolve over time. Having a strict contract about what an HTTP API does at any particular moment in time is contrary to the spirit of HTTP. The OpenAPI specification never uses the word "contract" intentionally. We talk about OpenAPI descriptions, or OpenAPI documents. These documents describe what can do with an API, and what you might expect from a response, but they are never exhaustive in their description. They do not tell you what you will NOT get as a response.

This means that when you describe an API operation as returning a 200, you are not saying that it will never return a 204. There is no expectation that an OpenAPI description describes all of potential status codes. In fact it really can't because intermediaries can return status codes that an API owner is not aware of.

This distinction is a fairly fundamental difference between OpenAPI and WSDL. WSDL tried to define a closed world system that completely described everything that might or might not happen. It was that strictness that brought about the fragility that contributed to the downfall of SOAP.

I understand that it feels "dirty" to have a client experience that suggests that a response could be null when you know it cannot be... today. However, maybe tomorrow it could be. We recommend that when developers incorporate Kiota client code into their applications, they use a service adapter class to provide an isolation boundary. This service adapter can make the nullable type go away and the rest of the application can built without worrying about the potential evolvability issues.

Kiota API client code attempts to project the OpenAPI described HTTP interface into the programing language of your choice. The end result will likely not be the exact language API you were hoping for, but our goal is for it to be as true as possible to the reality of an HTTP API.

We are looking at ways to enable scaffolding the service adapter in ways that would be customizable. This is an area where we can possibly take a more strict view of what is currently supported in an API.

I understand that this may not be particularly satisfying answer, but hopefully it will at least give you a better understanding of our design motivations. I also have more to say about the use of nullable to indicate a property is required but I will have to come back later for that.

[markm77]

I am currently evaluating Kiota and other OpenAPI Generators and appreciate the detailed responses here. My use case is generating models as I have my own client code.

I would definitely support an option for tightened nullability if this can't be the default.

Also I notice model files are very noisy due to the following per-property switch:

/// <summary>The Notes property</summary>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public List<string>? Notes { get; set; }
#nullable restore
#else
        public List<string> Notes { get; set; }
#endif

Since it seems NRTs are all nullable and therefore currently redundant, wouldn't it make sense to clean up the files by removing the per-property switch and using the following at the top of the file (similar to AutoRest):

#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable disable
#endif

Then if/when a tight nullability option is introduced this could be limited to .NET 3.1+ and produce clean model files without the per-property switch.

Just some thoughts after some short experiments, apologies in advance if I've missed something.

[baywet]

Thanks for chiming in @markm77 The main reason why we're keeping a per property/method level definition is that it varies throughout the document. See the following example where the request body is required, therefore non-nullable. Although it's a verbose approach, it gives us a more fine grained definition.

[markm77]

Okay, thanks @baywet for the clarification, appreciated!

[LockTar]

@baywet and @darrelmiller thank you again for your insights and also your detailed answers back. Don't worry about not giving me a satisfying answer @darrelmiller because I understand code has to evolve :-). My own code as well.

I was not really pointing to odata API's but just more in general.

This means that when you describe an API operation as returning a 200, you are not saying that it will never return a 204. There is no expectation that an OpenAPI description describes all of potential status codes. In fact it really can't because intermediaries can return status codes that an API owner is not aware of.

I understand (and I'm glad) that you don't have to point out every response. In a practical way you describe the most common scenario's like in example 200, 201, 204, 400, 401, 403, 404. But then I think, when you describe something you do it for a reason. So when my happy flow returns a 200 single object or a 404 when not found I can describe those very well. This will help my own team or my consumer of my API.

And that is basically what this discussion is about. Maybe Kiota should turn it around? So when you describe a response, it follows that spec very well and when you don't describe it has a more soft touch (non strict) for it (in this case a nullable type). And maybe with an extra parameter you can always use the non strict return type. It forces people (API producer) to follow more the spec for their API. The Microsoft Graph has great documentation and it's getting better every day. You guys do an excellent job for that. But I think that decisions like (always non strict) this will keep the quality of specs low because. Again, I get your feedback and thank you for responding. Love these thought forth and back.

We recommend that when developers incorporate Kiota client code into their applications, they use a service adapter class to provide an isolation boundary. This service adapter can make the nullable type go away and the rest of the application can built without worrying about the potential evolvability issues.

@darrelmiller could you explain this a bit more? Or have a link to a small sample? Because this could indeed help my team a bit more. Also, this could also be a temp workaround for #62 were we talked about. Do we need to place an isolation class for every model then? If you could point me out in the right direct I would love to create a sample for this. Thanks!

[baywet]

I believe Darrel was referring to a service architecture (DDD or otherwise) to be used in combination with the generated client. You can see an example of that here where the generated client is wrapped by a service before being called by the application logic.

[baywet]

Just as a recap for everyone: At this point the only work left to do is to emit "more precise" indexers (typed parameter) as described here (point 1 is already implemented)

[baywet]

Also I don't believe @kimbirkelund is working on this anymore? Is it over if I take this issue over?

[kimbirkelund]

Correct, I'm not.

[bkoelman]

@kimbirkelund There's a follow-up issue at #3911, please upvote if you're still interested.

[maxbeaudoin]

We recommend that when developers incorporate Kiota client code into their applications, they use a service adapter class to provide an isolation boundary. This service adapter can make the nullable type go away and the rest of the application can built without worrying about the potential evolvability issues.

@darrelmiller Sorry for reviving this thread but could you explain how the service adapter gets rids of the nullability issue? All properties of all models are still marked as nullable. It seems to be the case in this example as well. This pill is very hard to swallow in C# projects with <Nullable>enable</Nullable>.