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

RFC 3339 + Python #352

Open
pjmagee opened this issue Sep 11, 2024 · 10 comments
Open

RFC 3339 + Python #352

pjmagee opened this issue Sep 11, 2024 · 10 comments
Assignees
Labels
help wanted Good candidate for a pull request python Pull requests that update Python code status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:bug A broken experience

Comments

@pjmagee
Copy link

pjmagee commented Sep 11, 2024

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Nuget tool

Client library/SDK language

Python

Describe the bug

Python Serialized date: 2024-09-11T03:33:16+00:00Z

Link to Microsoft ISO/RFC 3339 for .NET:

https://learn.microsoft.com/en-us/dotnet/standard/datetime/system-text-json-support#the-extended-iso-8601-12019-profile-in-systemtextjson

When the Python SDK sends a request to an API using .NET System.Text.Json to Parse and validate RFC 3339 from a Kiota Python SDK datetime, it cannot deserialize the datetime.

If the Python code omitted the Z at the end, it would be valid because including the +00:00 timezone already means its UTC.

From the Microsoft page on RFC Parsing (for .NET at least)

4. "'Full date''T''Time hour'':''Minute''Time offset'"
"yyyy'-'MM'-'dd'T'HH':'mmZ"
"yyyy'-'MM'-'dd'T'HH':'mm('+'/'-')HH':'mm"

Check the link on RFC 3339 section 5.6:

https://datatracker.ietf.org/doc/html/rfc3339#section-5.6

It states Z OR time-numoffset

Expected behavior

2024-09-11T03:33:16+00:00Z should be serialised as 2024-09-11T03:33:16+00:00

How to reproduce

Any openapi spec with a datetime sent to a server to parse RFC 3339 datetime.

Using .NET System.Text.Json RFC 3339 Support

Open API description file

No response

Kiota Version

1.18.0

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

No response

Known Workarounds

No response

Configuration

No response

Debug output

Click to expand log ```
</details>


### Other information

_No response_
@pjmagee pjmagee added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels Sep 11, 2024
@msgraph-bot msgraph-bot bot added the python Pull requests that update Python code label Sep 11, 2024
@pjmagee
Copy link
Author

pjmagee commented Sep 11, 2024

>>> datetime.datetime.now().astimezone().isoformat()
'2024-09-11T12:29:05.080734+01:00'

This looks to be a valid RFC outcome, but it needs astimezone(). A Z at the end of this would invalidate RFC Specification

@pjmagee
Copy link
Author

pjmagee commented Sep 11, 2024

I'm going to debug shortly and see if i can find the line of the python lib which may need reviewing. According to the spec timezone+Z is invalid.

@baywet baywet transferred this issue from microsoft/kiota Sep 11, 2024
@baywet
Copy link
Member

baywet commented Sep 11, 2024

Hi @pjmagee
Thank you for using kiota and for reaching out.

Let's use RFC3339 moving forward in this discussion since it's the standard that OAS uses

If I understand your issue properly:

  • When serializing a DateTimeOffset in dotnet which is on the UTC standard you get +00:00 or Z at the end, but not both
  • When serializing a datetime in pythonwhich is on the UTC standard you get +00:00Z at the end, which is incorrect according to RFC3339

Does that reflect the situation properly?

If so, I've transferred this issue to the Python serialization repository, this is most likely caused by this block

Unit tests are implemented here (but only for the string type).

Is this something you'd like to submit a pull request for provided some guidance?

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close help wanted Good candidate for a pull request and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Sep 11, 2024
@pjmagee
Copy link
Author

pjmagee commented Sep 11, 2024

Hi @pjmagee Thank you for using kiota and for reaching out.

Let's use RFC3339 moving forward in this discussion since it's the standard that OAS uses

If I understand your issue properly:

  • When serializing a DateTimeOffset in dotnet which is on the UTC standard you get +00:00 or Z at the end, but not both
  • When serializing a datetime in pythonwhich is on the UTC standard you get +00:00Z at the end, which is incorrect according to RFC3339

Does that reflect the situation properly?

If so, I've transferred this issue to the Python serialization repository, this is most likely caused by this block

Unit tests are implemented here (but only for the string type).

Is this something you'd like to submit a pull request for provided some guidance?

Yes, this seems to be what i find occurring. I also am using System.Text.Json to 'deserialise' RFC 3339 Dates as our server is .NET, but I am using Kiota 'examples' in various languages (go, python, c#) etc.

I am happy to try and raise a PR, similar to the other date issue i fixed a while back.

@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 Sep 11, 2024
@baywet
Copy link
Member

baywet commented Sep 11, 2024

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 Sep 11, 2024
@pjmagee
Copy link
Author

pjmagee commented Sep 11, 2024

I think i found the bug.... It's from stnduritemplate in the __init__.py of the package.

    @staticmethod
    def __convert_native_types(value: Any) -> str:
        if isinstance(value, (str)):
            return value
        if isinstance(value, (bool)):
            return str(value).lower()
        elif isinstance(value, (int, float)):
            return str(value)
        elif isinstance(value, datetime):
            return value.isoformat("T") + "Z"

This Z is forced, this is not RFC 3339 Compliant.

I think the fix is:

if value.tzinfo is None:
    return value.isoformat("T") + "Z"
else:
   return value.isoformat("T")

@baywet
Copy link
Member

baywet commented Sep 12, 2024

Thank you for the additional information.
I've seen multiple notifications come in, this is the first one I'm looking at, this answer might be a bit behind.

There are effectively multiple places where the format of a date time matters:

  • URIs (path and query parameters)
  • serialization writers (and parse node) text/json/multipart/form

As a general rule of thumb: kiota client should try to normalize URI parameters as much as it can before sending it down to std uri template. This is because we have more context and dependencies than this lib has and can handle more data types it can.

We should also strive to have the same implementation for date time across abstractions and serialization implementations to achieve maximal consistency.

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Sep 12, 2024
@baywet baywet removed Status: No Recent Activity status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Sep 17, 2024
@baywet baywet transferred this issue from microsoft/kiota-serialization-json-python Oct 14, 2024
@andrueastman
Copy link
Member

@pjmagee Any chance you can confirm if the issue is resolved with the latest release at #405?

If I understand correctly, the issue here is with the serialization in the URI which added the Z due to the workings of the std-uritemplate library. Which should be resolved with changes made in #404

@andrueastman andrueastman added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label Nov 11, 2024
@andrueastman andrueastman moved this from Todo 📃 to Waits for author 🔁 in Kiota Nov 11, 2024
@pjmagee
Copy link
Author

pjmagee commented Nov 12, 2024

Hello, currently overloaded but I will try to find some time end of week to test this :) Thanks for working on it behind the scenes. I know bigger changes were needed to solve this issue

@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 12, 2024
@andrueastman andrueastman added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Nov 12, 2024

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
help wanted Good candidate for a pull request python Pull requests that update Python code status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:bug A broken experience
Projects
Status: Waits for author 🔁
Development

No branches or pull requests

3 participants