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

Update json_parse_node.py #427

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Update json_parse_node.py #427

merged 2 commits into from
Dec 17, 2024

Conversation

Kobusvdwalt
Copy link
Contributor

Fixes numerical strings accidentally getting parsed to date

Overview

There seems to be a problem in deserialization of numerical strings in the ms-graph-sdk, which in turn uses this SDK.
It was pointed out that the old serialization library had a patch applied
https://github.com/microsoft/kiota-serialization-json-python/pull/358/files
Which resolved the issue, but the ms-graph-sdk seems to now be using this repo, where the patch does not exist.

Related Issue

microsoftgraph/msgraph-sdk-python#1026

Fixes # (issue)
microsoftgraph/msgraph-sdk-python#1026

Fixes numerical strings accidentally getting parsed to date
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 also add a unit test for the scenario? This way we prevent any future regression.

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

@christianmartinroffey
Copy link

@baywet just wondering here as @Kobusvdwalt was proactive enough to provide a fix, do you not have a unit test you can repurpose internally for this?

This issue is flagged here microsoftgraph/msgraph-sdk-python#1026 and had previously been fixed according to the comments from another user

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!

@christianmartinroffey I was about to provide more context for this question when I saw the push coming through!

@baywet
Copy link
Member

baywet commented Dec 17, 2024

@Kobusvdwalt can you make sure you sign the CLA please so we can merge this? (see the bot comment above)

@baywet baywet enabled auto-merge (squash) December 17, 2024 13:28
@Kobusvdwalt
Copy link
Contributor Author

@microsoft-github-policy-service agree company="RoyalAholdDelhaize"

@baywet baywet merged commit ab1a6c1 into microsoft:main Dec 17, 2024
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done ✔️
Development

Successfully merging this pull request may close these issues.

3 participants