microsoft / kiota

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

Nested JSON/additional_data datetimes not round-tripped #5743

Open thelazydogsback opened 1 week ago

thelazydogsback commented 1 week ago

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Windows executable

Client library/SDK language

Python

Describe the bug

I am trying to GET my entity, and without any changes to the object, PUT the object.

Expected behavior

I expect the object to be the same when serialized to the server for the PUT as when it was read with the GET (either exactly, or at least semantically equivalent.)

How to reproduce

request_config = RequestConfiguration[ItemRequestBuilderV2.ItemRequestBuilderGetQueryParameters]()
request_config.query_parameters = ItemRequestBuilderV2.ItemRequestBuilderGetQueryParameters(opt='xxx')
e = asyncio.run( client.v2.Entity.by_id(id).get(request_config))

asyncio.run( client.v2.Entity.put(e))

The entity contains an arbitrarily nested JSON object that should maintain its integrity when reserialized. When the entity is GET on the Python side when deserialized it looks like this: e.Contents.additional_data['installationDate'] = DateTime(2023, 3, 24, 0, 0, 0, tzinfo=TimeZone('UTC')) In this case the JSON value had the string value "2023-03-24".

I expect this value to be serialized back out as either the original format "2023-03-24" by not converting to a date but keeping the original string, or by serializing as "2023-03-24T00:00:00+00:00". (The former is preferred.) (This example is at the top-level, but also happens in deeper nestings.)

However, when the entity is PUT back, what gets serialized is the empty object - inspecting in the debugger in the C# server, the PUT controller method sees this entry in the dictionary - an empty object rather than the date string: installationDate = ValueKind(Object, "{}")

How do I keep the original string format of the dates and round-trip them exactly? And for reference, how to keep the DateTimes and serialize them back out as ISO format strings rather than the empty objects? thanks

Open API description file

Unable to provide

Kiota Version

1.19.1

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

No response

Known Workarounds

No response

Configuration

x64, vscode

Debug output

Click to expand log ``` ```

Other information

No response

andrueastman commented 6 days ago

Thanks for raising this @thelazydogsback

As the item is placed in the additionalData, the culprit would most likely lie in the function here. https://github.com/microsoft/kiota-python/blob/d700d67a2b5da0ab37f2362dcc7504b51a4ef283/packages/serialization/json/kiota_serialization_json/json_serialization_writer.py#L492

Any chance you are able to confirm the version of the python libraries you are using with your generated client? Would you be willing to submit a pull request with a failing test with a date-time object in the additional_data to help us understand the issue here? https://github.com/microsoft/kiota-python/blob/d700d67a2b5da0ab37f2362dcc7504b51a4ef283/packages/serialization/json/tests/unit/test_json_serialization_writer.py#L92

thelazydogsback commented 6 days ago

My microsoft-kiota libs were a bit stale -- but unfortunately updating to latest (1.6.2) didn't change the behavior.

Does this mean I can write a subclass that implements write_datetime and register this somehow when I init my client?

baywet commented 6 days ago

Thank you for the additional information.

To be specific, I don't believe the ask from Andrew was for you to add custom code to your application, but rather he was asking if you'd be willing to work on a pull request to fix the issue for everybody.

I think the first step here would be to add a unit test similar to this one which would instead call "write_any_value" with a datetime value, and see whether we get the desired behaviour.

This way we could determine whether the issue comes from serialization or deserialization.

Let us know if you have any additional comments or questions.

thelazydogsback commented 6 days ago

Yes I understand the request but (not trying to be rude at all) I'll see what I can do, but my first order of business is to get the required functionality working for our users. This is similar to the custom subclasses of AzureIdentity{AccessToken|Auth}Provider I needed to write to allow for auth over HTTP.

Outside of this bug (assuming it is one) I assume that overriding the behavior of dynamic data handling is useful because whatever default assumptions are made are likely not what some clients need in certain scenarios.

thelazydogsback commented 5 days ago

UPDATE: @andrueastman It turns out that updating the kiota libs did solve the issue of the dates being written out as {} -- however, it has brought out other issues. (1) When parsing a string that is date only (such as "2023-03-24"), rather than being read as a date, it is being read as a datetime with no time part set. Thus when serializing back out, it takes the form of "2023-03-24T00:00:00+00:00" rather than "2023-03-24". In this case this causes a BAD REQUEST in my downstream Azure service, because it's expecting an RFC3339 date type, not a datetime.

(2) In order to fix this at serialization time, I'm figured I'd write a custom serializer to handle this. I used the factory pattern and the classes below:

Image

However, there seems to be a bug where the factory is used when the writer is first created, but then later in the code the factory is ignored when a temp writer is created:

Image

Image

This means that by the time we get want to write the date from the parent write_object, then custom writer is no longer being used.

[EDIT] So obviously _create_new_writer is on the same class, so I can override that as well. (Code updated above) So looks like that at least solves the work-around. (Not sure if in future it's better to either invoke the factory or just make a new instance of whatever executing writer class is using self.__class__().)

baywet commented 3 days ago

Thank you for the additional information.

So if I'm understand correctly, the only remaining issue at this point is that when parsing a date only for additional properties, it's being instantiated as a datetime instead of a date only?

If so this section is most likely what's causing the issue here.