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

Partial post/patch support #3846

Closed
bkoelman opened this issue Dec 2, 2023 · 10 comments
Closed

Partial post/patch support #3846

bkoelman opened this issue Dec 2, 2023 · 10 comments
Assignees
Labels
Csharp Pull requests that update .net code
Milestone

Comments

@bkoelman
Copy link

bkoelman commented Dec 2, 2023

I'm looking for a way to mark properties in request bodies to be explicitly included in the payload, even when their values are null. This is required for JSON:API compliance, specifically:

Any or all of a resource’s attributes MAY be included in the resource object included in a PATCH request.

If a request does not include all of the attributes for a resource, the server MUST interpret the missing attributes as if they were included with their current values. The server MUST NOT interpret missing attributes as null values.

Omitting a property in the request body means leaving its stored value unchanged, whereas sending it with value null instructs to update the stored value. This poses a problem for C# clients. There's no way to distinguish whether a property is null because it was never set, or that it was intentionally set to null because it needs to be cleared.

To address this, we use the following with NSwag:

var patchRequest = new PersonPatchRequestDocument
{
    Data = new PersonDataInPatchRequest
    {
        Id = "1",
        Attributes = new PersonAttributesInPatchRequest
        {
            // FirstName = null,
            LastName = "Doe"
        }
    }
};

// This line results in sending "firstName: null" instead of omitting it.
using (apiClient.WithPartialAttributeSerialization<PersonPatchRequestDocument, PersonAttributesInPatchRequest>(
    patchRequest, person => person.FirstName))
{
    await apiClient.PatchPersonAsync(1, null, patchRequest);
}

where WithPartialAttributeSerialization() is a method provided by our NSwag extension library that hooks into the underlying Newtonsoft serializer by registering a custom ContractResolver that overrides NullValueHandling/DefaultValueHandling at runtime. The using ensures the registration for firstName is cleared afterwards, so the API client can be reused.

I'm looking for a way to provide a similar feature using Kiota. Any pointers on where to hook in would be greatly appreciated.

@github-project-automation github-project-automation bot moved this to Todo in Kiota Dec 2, 2023
@baywet baywet self-assigned this Dec 4, 2023
@baywet baywet added question Csharp Pull requests that update .net code labels Dec 4, 2023
@baywet baywet added this to the Backlog milestone Dec 4, 2023
@baywet
Copy link
Member

baywet commented Dec 4, 2023

Thanks for reaching out.
Have you tried your scenario while using the backing store? (--backing-store|-b argument)
This enables dirty tracking on the generated models, which should solve your challenge here.

@bkoelman
Copy link
Author

bkoelman commented Dec 6, 2023

Thanks. I read about that, but I suspect it's not going to work in our case for two reasons:

  1. Clients don't typically fetch-before-update in a connected way. For example, a Razor page fetches data using JSON:API, reshapes it into a UI model, and renders it. A few minutes later, the user clicks a command, which makes the C# app send an update command using JSON:API. This potentially submits fields that weren't fetched earlier.
  2. JsonApiDotNetCore enables to configure capabilities, which makes the models returned on GET different from those being accepted in POST and PATCH. Aside from that, the nullability/requiredness is different. I read somewhere that Kiota treats everything as nullable (I don't fully understand why), but that's not how NSwag works. We've spent a long time tweaking these. Our NSwag solution results in a serialization error when required properties are missing, or non-nullable properties are assigned null (so it never sends out the request).

@baywet
Copy link
Member

baywet commented Dec 6, 2023

Thanks for the additional information.
The reason why kiota models properties are nullable is because OpenAPI doesn't contain enough information at the moment.
Nullable could mean:

  • value might be null
  • value might not be selected on a GET scenario
  • value might be optional on a POST/PATCH/PUT scenario

And it doesn't account for readonly properties, init only properties, etc... So rather than picking a design that would lock a lot of scenarios out, we picked one that was a bit broader, even though it might come at a cost of not being specific enough in some cases. As the standard evolves, we'll review those decisions.

While the backing store was primarily designed for a fetch before update scenario, I believe it could be used to cover the one you've shared.
Have you tried the following (with the backing store enabled)

var modelInstance = new MyModel();
modelInstance.PropA = null;
var result = KiotaJsonSerializer.SerializeAsString(modelInstance);

Do you get {} or { "propA": null}?
(it's been a while since I've worked around that space, so I can't recall the specifics)

@bkoelman
Copy link
Author

bkoelman commented Dec 7, 2023

Well, using the backing store worked out pretty nicely! It's more convenient than our approach with NSwag. So it's simply a matter of explicitly setting the property to null and then it gets tracked in the store, which the serializer takes into account. It could become problematic when models are reused, but the backing store has a Clear method to overcome that. We lose the client-side validation, but I suppose that's not a big deal. Developers can still defensively code against that by inspecting the OAS or SwaggerUI, which tells when properties are required.

About the nullability: it goes both ways. Making everything nullable results in many noisy squigles:
image

In my opinion, NSwag handles this better. By default, it follows the required/nullable from the OAS, but doesn't emit nullable reference types. So everyone on recent .NET versions adds the /GenerateNullableReferenceTypes:true switch. Once activated, an optional second switch (/GenerateOptionalPropertiesAsNullable:true) makes all non-required properties nullable, which results in similar warnings, except when properties are required (so it's still a bit better).

With our OAS, NSwag with /GenerateNullableReferenceTypes:true looks like:
image

And with /GenerateNullableReferenceTypes:true /GenerateOptionalPropertiesAsNullable:true, it becomes:
image

As can be seen above, only .Attributes becomes nullable. This is because the entire object is omitted from the response when no attributes are returned (instead of an empty object). This occurs when a GET request fetches only relationships without attributes.

It would be nice if Kiota could behave similarly:

  • value might be null: then it's nullable
  • value might not be selected on a GET scenario: then nullability depends on the switch
  • value might be optional on a POST/PATCH/PUT scenario: then nullability depends on the switch

And it doesn't account for readonly properties, init only properties, etc... So rather than picking a design that would lock a lot of scenarios out, we picked one that was a bit broader, even though it might come at a cost of not being specific enough in some cases. As the standard evolves, we'll review those decisions.

readonly and init-only are C# concepts. Do you think it makes sense for them to be added to the OAS standard?

@baywet
Copy link
Member

baywet commented Dec 7, 2023

Thank you for the additional information.

readonly and init-only are C# concepts. Do you think it makes sense for them to be added to the OAS standard?

Yes, and @darrelmiller, architect for kiota and long time core member of the OpenAPI technical steering committee is working on OpenAPI v4 (with others) with those considerations in mind.

adding a switch

We understand it's a bit inconvenient at the moment, but adding a switch would go against the pillar principal of keeping kiota simple. NRT is a feature very specific to dotnet (arguably Go and TypeScript could have a similar behavior), and we don't want to add switches that are specific to languages. This is because ultimately it complexifies the CLI experience, makes the maintenance matrix a whole lot more difficult, etc... Our strategy over time is to take advantage of the more precise capabilities of OpenAPI v4. When we get there, we might also be able to drop support for netstandard2.0/net framework, which means we'll be able to modernize the dotnet we're generating a great deal.

@bkoelman
Copy link
Author

bkoelman commented Dec 8, 2023

readonly and init-only are C# concepts. Do you think it makes sense for them to be added to the OAS standard?

Yes, and @darrelmiller, architect for kiota and long time core member of the OpenAPI technical steering committee is working on OpenAPI v4 (with others) with those considerations in mind.

I fail to understand why it would be appropriate for a server to express these. Read-only and init-only are client-side and language-specific concerns that don't describe the expected shape of exchanged data. Can you elaborate on that?

adding a switch

We understand it's a bit inconvenient at the moment, but adding a switch would go against the pillar principal of keeping kiota simple. NRT is a feature very specific to dotnet (arguably Go and TypeScript could have a similar behavior), and we don't want to add switches that are specific to languages. This is because ultimately it complexifies the CLI experience, makes the maintenance matrix a whole lot more difficult, etc... Our strategy over time is to take advantage of the more precise capabilities of OpenAPI v4. When we get there, we might also be able to drop support for netstandard2.0/net framework, which means we'll be able to modernize the dotnet we're generating a great deal.

Fair enough. In that case, I suppose it makes sense for a C# client to infer "optional non-nullable properties are expressed as nullable", although this is opinionated. And that optional nullable properties are inferred as nullable. And that required nullable properties are inferred as nullable. What I think is incorrect is that required non-nullable properties are inferred as nullable. That's like going back to the time before NRT was introduced: you'll never know whether something is functionally required. I'd say it's up to servers to decide how models (components) are shared. In our JSON:API implementation, we chose to define distinct models for GET, POST and PATCH payloads. An alternate design would be to reuse models, but then we'd define every property as optional because of partial POST/PATCH: any omitted attributes are considered unchanged (PATCH) or use a server-defined default (POST), which would result in Kiota generating nullable properties, which works well. While there a pros and cons to both component designs, it's ultimately the server making the decision and the client should just follow along to its best abilities. A client-generator that contradicts the expressed "required non-nullable" sounds like a bug to me. Would you like me to open a separate issue for that?

@baywet
Copy link
Member

baywet commented Dec 8, 2023

"required non-nullable" sounds like a bug to me. Would you like me to open a separate issue for that?

Not at the moment, because again, kiota has this opiniated choice (ultimately wrong, I agree) to account for shortcomings of the standard and mis-use from a majority of the services out there. Yes ideally all services should deeply think about client experience, and if they expect a property to be required in a POST scenario, but optional in a GET scenario, use two different schemas. But the vast majority will use a single one today for simplicity, which leads to locking people out of some scenarios if you follow the spec and the description to the letter. Our hope is that with the v4 which has more specific mechanism, it'd enable people to describe things with re-usable schemas, but apply contextual "constraints", getting the best of both worlds. (and in that context, kiota'd have to project multiple types for the "same" schema)

Can you elaborate on that?

There are additional scenarios that I failed to mention like idempotency for instance.
The service would have interest to describe all those aspects to provide more clarity to the client, and avoid unnecessary information in the payload:

  • writeonly: client can write this thing, but keep a local copy in sync and don't expect to see it in a GET operation.
  • readonly: client can read it, but POST/PATCH operations will fail if included in the payload.
  • init: same as readonly, relaxed on the creation scenario.
  • idempotenty: client can init the property. client is also allowed to send it along on POST/PATCH operations, but it'll be a NOOP. client can request it in a GET operation, but if it was set by the client originally, it'll forcibly be the same value, if the client already has a copy of the value, it can save bytes over the wire.

@bkoelman
Copy link
Author

"required non-nullable" sounds like a bug to me. Would you like me to open a separate issue for that?

Not at the moment, because again, kiota has this opiniated choice (ultimately wrong, I agree) ...

Well, it's not a hard blocker, so I can live with it, for now at least, though I expect questions and/or bug reports from our users regarding this if we adopt Kiota. Yet I sense some frustration in your response, wondering why. So I searched existing issues for "nullable" and noticed this question came up several times before, where you basically explained the same things as here and closed the issue. It may help to keep an open issue around so people won't keep asking about this. Even if low priority, or a strategic decision to postpone fixing, it would enable others to upvote so the team can reconsider this design choice in the future. Likewise, I saw initial pushback on implementing NRT support in a Kiota issue, which happened eventually.

We're not on a tight deadline, but OpenAPI 4 sounds like years away before largely adopted in the ecosystem. Swashbuckle doesn't even support 3.1 because they're still waiting on Microsoft OpenAPI.NET to implement it. As far as I'm aware, no OpenAPI 3.1 server libraries exist for .NET today.

These new v4 features seem to address higher-level architectural patterns, that may not compose well with existing standards, which makes universal adoption take a very long time, if at all. For example, JSON:API addresses these concerns differently and its authors are unlikely to break with earlier versions of their spec, just because of new OpenAPI features. They consider OpenAPI and similar schema languages an implementation detail; it was discussed years ago without reaching consensus.

@baywet
Copy link
Member

baywet commented Dec 12, 2023

It may help to keep an open issue around so people won't keep asking about this

Please go ahead and open an issue dedicated to that, I'll add it to the v3 milestone and we'll take it from there.

I saw initial pushback on implementing NRT support in a Kiota issue, which happened eventually

The initial pushback was due to the lack of support for this feature in netstandard2.0/netfx. We eventually figured out a solution with conditional compilation, arguably it's a bit of a noisy solution.

waiting on Microsoft OpenAPI.NET to implement it

My team is ultimately also responsible for this library. Progress has been slow because we took this opportunity to:

  1. Get rid of a third party YAML parsing library, and implement everything using the STJ primitives.
  2. Remove our custom code for JSON schemas, and rely on a library for that instead.
    I'm hopeful that we should see early previews for 3.1 support in the next semester.

@bkoelman
Copy link
Author

My team is ultimately also responsible for this library.

Great to hear this is getting attention.

I've created #3911 to track required/nullable, so I think this issue can be closed now.

@baywet baywet closed this as not planned Won't fix, can't repro, duplicate, stale Dec 14, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Kiota Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code
Projects
Archived in project
Development

No branches or pull requests

2 participants