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

Nested JSON/additional_data datetimes not round-tripped #5743

Closed
thelazydogsback opened this issue Nov 8, 2024 · 7 comments
Closed

Nested JSON/additional_data datetimes not round-tripped #5743

thelazydogsback opened this issue Nov 8, 2024 · 7 comments
Labels
Python Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:bug A broken experience

Comments

@thelazydogsback
Copy link

thelazydogsback commented Nov 8, 2024

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 ```
</details>


### Other information

_No response_
@thelazydogsback thelazydogsback added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels Nov 8, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Nov 8, 2024
@msgraph-bot msgraph-bot bot added the Python label Nov 8, 2024
@andrueastman
Copy link
Member

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

@andrueastman andrueastman added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Nov 11, 2024
@andrueastman andrueastman moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Nov 11, 2024
@thelazydogsback
Copy link
Author

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?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Nov 11, 2024
@baywet
Copy link
Member

baywet commented Nov 11, 2024

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.

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Nov 11, 2024
@thelazydogsback
Copy link
Author

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Nov 11, 2024
@thelazydogsback
Copy link
Author

thelazydogsback commented Nov 12, 2024

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
Copy link
Member

baywet commented Nov 14, 2024

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.

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Nov 14, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Python Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:bug A broken experience
Projects
Status: Done ✔️
Development

No branches or pull requests

3 participants