Skip to content
This repository has been archived by the owner on Oct 14, 2024. It is now read-only.

fix serialization of pendulum.DateTime objects in JsonSerializationWriter #369

Merged

Conversation

eran-av
Copy link
Contributor

@eran-av eran-av commented Sep 24, 2024

Environment

  • Python Version: 3.11.7
  • kiota-serialization-json version: 1.3.2
  • OS: MacOS

Describe the bug
The write_any_value method in JsonSerializationWriter fails to properly handle pendulum.DateTime objects. This is due to insufficient type checking, which causes pendulum.DateTime objects to be treated as non-primitive types instead of as datetime objects. Specifically, the issue stems from not using isinstance() to check for datetime types, which prevents pendulum.DateTime from being recognized as a subclass of the primitive datetime.datetime.

To Reproduce

  1. Create a JsonSerializationWriter instance
  2. Attempt to serialize an object containing a pendulum.DateTime field (specifically as an additional_data dict)
  3. Observe that the pendulum.DateTime object is not serialized correctly

Expected behavior
pendulum.DateTime objects should be treated as datetime objects and serialized correctly, just like datetime.datetime objects. The serialization process should recognize pendulum.DateTime as a subclass of datetime.datetime and handle it accordingly.

Code to reproduce

from kiota_serialization_json import JsonSerializationWriter
import pendulum

writer = JsonSerializationWriter()
writer.write_additional_data_value({
    "created_at": pendulum.DateTime(2024, 9, 24)
})
content = writer.get_serialized_content()
print(content.decode('utf-8'))

@eran-av eran-av requested a review from a team as a code owner September 24, 2024 15:16
@eran-av eran-av marked this pull request as draft September 24, 2024 15:19
@eran-av eran-av force-pushed the bugfix/pendulum-datetime-serialization branch from 5438a74 to 3278fe8 Compare September 24, 2024 16:01
@eran-av
Copy link
Contributor Author

eran-av commented Sep 24, 2024

@eran-av please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@eran-av eran-av marked this pull request as ready for review September 24, 2024 16:41
@eran-av
Copy link
Contributor Author

eran-av commented Sep 29, 2024

@baywet, @andrueastman Can you please take a look?

baywet
baywet previously approved these changes Oct 3, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!
Can you please also bump the patch version in the version file and add a changelog entry? (today's date, changed) to ensure a swift release.

Also, sorry about the delay in reviewing, I missed this one in my many, many notifications...

@baywet
Copy link
Member

baywet commented Oct 3, 2024

Additionally, the code needs to be formatted (see the build logs)

baywet
baywet previously approved these changes Oct 3, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes!

baywet
baywet previously approved these changes Oct 3, 2024
baywet
baywet previously approved these changes Oct 3, 2024
Signed-off-by: Vincent Biret <[email protected]>
baywet
baywet previously approved these changes Oct 3, 2024
@baywet
Copy link
Member

baywet commented Oct 3, 2024

@eran-av it seems some unit tests are not passing, can you have a look into it please?

Copy link

sonarqubecloud bot commented Oct 3, 2024

@eran-av
Copy link
Contributor Author

eran-av commented Oct 3, 2024

@eran-av it seems some unit tests are not passing, can you have a look into it please?

Fixed it,
Also - sorry about the linting issues.
Couldn't quite figure which linter + conf you're using.

@eran-av eran-av requested a review from baywet October 3, 2024 16:21
@baywet
Copy link
Member

baywet commented Oct 3, 2024

@eran-av it seems some unit tests are not passing, can you have a look into it please?

Fixed it, Also - sorry about the linting issues. Couldn't quite figure which linter + conf you're using.

yeah this is a bit confusing: pylint + yapf

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants