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

[Bug]Generator using fromUnixTimeSeconds for unixTimestamp with int64 #4189

Closed
yifan-zhou922 opened this issue Feb 4, 2024 · 9 comments
Closed
Assignees
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. DPG v3 Version 3 of AutoRest C# generator.

Comments

@yifan-zhou922
Copy link
Member

Describe the issue or request
When defining the createTime as @encode(DateTimeKnownEncoding.unixTimestamp, int64) in TypeSpec, the generator uses FromUnixTimeSeconds.
However, in the service use case, the createTime is in milliseconds. So an error for out-of-range is raised.
image

Here is the sample for createTime/updateTime.
image

Describe your ideas for solutions
Please describe how you think a change or changes might work. Consider
how these changes might work for multiple services.

Add labels
As appropriate, select a label to describe how hard the issue is to work
around or how hard it would be to do without a particular feature that
could help make it easier. These labels start with "workaround".

Also select a label that describes how many instances of the workaround
you would have to do without the bug being fixed or feature implemented.
These labels start with "instances".

These labels will help priority bug fixes and feature requests.

If this bug or feature request is for older versions of autorest, please
remove the v3 label and add the v2 label as appropriate.

@yifan-zhou922 yifan-zhou922 added the v3 Version 3 of AutoRest C# generator. label Feb 4, 2024
@chunyu3 chunyu3 added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Feb 4, 2024
@ArcturusZhang
Copy link
Member

ArcturusZhang commented Feb 4, 2024

We are calling the wrong method all day along:

? DateTimeOffsetExpression.FromUnixTimeSeconds(element.GetInt64())

But all the information I could find, such as:

The typespec doc here never says it is milliseconds or seconds, it looks like milliseconds

@mikeharder
Copy link
Member

I'm pretty sure it is milliseconds. TypeSpec is heavily influenced by JavaScript, and the JS Date object uses milliseconds since the epoch.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date

@mikeharder
Copy link
Member

@ArcturusZhang: If this is a simple fix to call FromUnixTimeMilliseconds() instead of FromUnixTimeSeconds(), could this be fixed quickly, so the spec doesn't need to apply a workaround?

@johanste
Copy link
Member

johanste commented Feb 5, 2024

@bterlson, this was intended as seconds, not milliseconds, right?

@mikeharder
Copy link
Member

mikeharder commented Feb 5, 2024

@johanste, @bterlson: Best issue for discussion is microsoft/typespec#2887

@mikeharder
Copy link
Member

@ArcturusZhang: Per discussion in microsoft/typespec#2887, it seems that the intention of DateTimeKnownEncoding.unixTimestamp in TypeSpec was seconds since epoch, so autorest.csharp is correct.

@ArcturusZhang
Copy link
Member

@ArcturusZhang: Per discussion in microsoft/typespec#2887, it seems that the intention of DateTimeKnownEncoding.unixTimestamp in TypeSpec was seconds since epoch, so autorest.csharp is correct.

So I bring this up because of being afraid there would be debates like this because it is really unclear on typespec documents, and different person with different background would have different comprehension on the work "unix timestamp". My first thought is that this is "seconds".

Service teams (slash typespec authors) might also have the same confusion depending on their background - therefore this is really ambiguous.
Please make sure we clarify that enough
Currently dotnet generator will not change since it is the correct intention. If we do need a change, it is really simple, just replace the method we are using from DateTimeOffset.FromUnixSeconds to FromUnixMilliseconds.

@ArcturusZhang ArcturusZhang closed this as not planned Won't fix, can't repro, duplicate, stale Feb 6, 2024
@ArcturusZhang
Copy link
Member

Close this because our current behavior is correct

@mikeharder
Copy link
Member

mikeharder commented Feb 6, 2024

@ArcturusZhang: Per discussion in microsoft/typespec#2887, it seems that the intention of DateTimeKnownEncoding.unixTimestamp in TypeSpec was seconds since epoch, so autorest.csharp is correct.

So I bring this up because of being afraid there would be debates like this because it is really unclear on typespec documents, and different person with different background would have different comprehension on the work "unix timestamp". My first thought is that this is "seconds".

Service teams (slash typespec authors) might also have the same confusion depending on their background - therefore this is really ambiguous. Please make sure we clarify that enough Currently dotnet generator will not change since it is the correct intention. If we do need a change, it is really simple, just replace the method we are using from DateTimeOffset.FromUnixSeconds to FromUnixMilliseconds.

@ArcturusZhang: If you have any more feedback not already captured microsoft/typespec#2887, please add it to that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. DPG v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

No branches or pull requests

6 participants