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

Cannot write null to a nullable value #1188

Closed
mbursill opened this issue May 15, 2024 · 22 comments · Fixed by #1248 or microsoft/kiota#5106
Closed

Cannot write null to a nullable value #1188

mbursill opened this issue May 15, 2024 · 22 comments · Fixed by #1248 or microsoft/kiota#5106
Labels
bug Something isn't working type:bug A broken experience WIP
Milestone

Comments

@mbursill
Copy link

Our API depends on null's being allowed as part of the request:

const updateWidgetRequest: UpdateWidgetRequest = {
  name: 'Sproket',
  size: 5,
  inspectionDetails: null,
};

This is different than sending undefined for inspectionDetails. Our API treats undefined on patch as no change, but null as a means to clear the value.

While the swagger file defines the type as nullable, the generated code only allows InspectionDetails | undefined

We've tried to hack a value by doing:

inspectionDetails: <InspectionDetails>(<unknown>null)

This is awful and still doesn't work. This call to writeObjectValue strips it.

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota May 15, 2024
@baywet
Copy link
Member

baywet commented May 16, 2024

Hi @mbursill
Thanks for using kiota and for reaching out.
QQ: are you enabling the backing store when you generate your client?

@baywet baywet added question status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 16, 2024
@baywet baywet moved this from Needs Triage 🔍 to In Progress 🚧 in Kiota May 16, 2024
@baywet baywet added this to the Backlog milestone May 16, 2024
@mbursill
Copy link
Author

Tested with both backing store and without. The generated model is:

export interface UpdateWidgetRequest extends BackedModel, Parsable {
    /**
     * Stores model information.
     */
    backingStoreEnabled?: boolean;
    /**
     * The inspectionDetails property
     */
    inspectionDetails?: InspectionDetails;
    /**
     * The mass property
     */
    mass?: number;
    /**
     * The name property
     */
    name?: string;
    /**
     * The size property
     */
    size?: number;
}

The fields are all optional, meaning they're union with undefined, but assigning null is not possible.

@baywet
Copy link
Member

baywet commented May 17, 2024

Thanks for the additional information. Is your goal of passing null to have null in the JSON payload to reset a field of an object for example?
The serialization writer already contains a method to write a null value but I don't think it was thought about end to end: the prototype of the methods would need to be updated, and we'd need all the implementations to test for the null value.

@mbursill
Copy link
Author

Is your goal of passing null to have null in the JSON payload to reset a field of an object for example?

Yes, that's exactly what we're wanting it for. Undefined doesn't work for us in this case. Our API uses undefined to mean a change to the field was not made, and not to alter the value. To clear/reset the value we need null.

@baywet
Copy link
Member

baywet commented May 20, 2024

Thanks for the additional context.
Yes, this is probably a limitation of the design and code generation today we should work to fix.
The first step is most likely to update all the interfaces (ParseNode, SerializationWriter, BackingStore,...) so parameters or return types also allow null as a value.
Is this something you'd be interested in submitting a pull request for?

@baywet baywet added bug Something isn't working type:bug A broken experience and removed Needs: Attention 👋 question status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 22, 2024
@baywet baywet moved this from In Progress 🚧 to Proposed 💡 in Kiota May 22, 2024
@rners01
Copy link

rners01 commented Jun 14, 2024

any updates on this?

@rners01
Copy link

rners01 commented Jun 18, 2024

Hey @baywet, I want to contribute.
How I can generate kiota.exe build after adding changes to packages?

@baywet
Copy link
Member

baywet commented Jun 19, 2024

@rners01 you can run it straight in process if you want with a single command

dotnet run --project ./src/kiota/kiota.csproj -- arguments you usually pass to kiota

or you can tweak the .vscode/launch.json if you are using vs code.

Make sure you have dotnet sdk 8 installed first

Thanks for the time and help!

@rners01
Copy link

rners01 commented Jun 21, 2024

@baywet but I don't see no ./src/kiota/kiota.csproj in kiota-typescript repo

image

@baywet
Copy link
Member

baywet commented Jun 21, 2024

sorry, I meant in the main repo

@rners01
Copy link

rners01 commented Jun 25, 2024

but then I don't understand, I have changes in kiota-typescript and seems like kiota is totally different project
how can I generate the .exe with kiota-typescript changes?

@baywet
Copy link
Member

baywet commented Jun 25, 2024

the kiota typescript repository only contains the typescript libraries (abstractions, serialization, http, authentication,...) the generation happens in the main kiota repository.

@rners01
Copy link

rners01 commented Jun 25, 2024

do you have any high-level instruction what changes should I apply to process null along with generation of .exe?

@baywet
Copy link
Member

baywet commented Jun 25, 2024

let's start with the TypeScript changes if you don't mind? Can you put a pull request together to make the following changes please? We'll then proceed to the generation changes as you won't be able to test them until the libraries here are updated.

@Kindest13
Copy link
Contributor

Hey @baywet, are there any changes needed for this to work?

@baywet
Copy link
Member

baywet commented Aug 14, 2024

besides updating the dependencies and refreshing the client? or am I misunderstanding your question?

@Kindest13
Copy link
Contributor

besides updating the dependencies and refreshing the client? or am I misunderstanding your question?

yes

@baywet
Copy link
Member

baywet commented Aug 14, 2024

turns out I found a missed update in my tests earlier. Once this is merged we should be good to go.
microsoft/kiota#5163

@Kindest13
Copy link
Contributor

Kindest13 commented Sep 2, 2024

Hey @baywet, after generating client from latest release build

there are a lot of errors on new null type, I guess we should've extended our methods with null type

image
image

@baywet
Copy link
Member

baywet commented Sep 2, 2024

@Kindest13 you need to update the kiota packages (abstractions,...)

@Kindest13
Copy link
Contributor

Kindest13 commented Sep 6, 2024

@baywet Yes, you'r right.
I have a question around id in ResponseRecord though, should we keep it as just string without additional null type as it's usually required?

export interface ResponseRecord extends Parsable {
    /**
     * The unique identifier.
     */
    id?: string | null;
}

@baywet
Copy link
Member

baywet commented Sep 6, 2024

@Kindest13 there's a long discussion around required/nullability here microsoft/kiota#3911
TL;DR: today the way kiota generates code is not designed to handle required/non-nullable properties, this is because when we started out, most descriptions out there did not handle the information properly. And some contextualization is missing in the OAS standard itself to carry some distinctions.

So for now the goal with TypeScript is to align it with the other languages, and eventually make it stable, before we look at changing those things across languages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working type:bug A broken experience WIP
Projects
Archived in project
4 participants