-
Notifications
You must be signed in to change notification settings - Fork 206
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
Feature: Add @dateFormat
decorator
#1796
Feature: Add @dateFormat
decorator
#1796
Conversation
Changes in this PR will be published to the following url to try(check status of TypeSpec Pull Request Try It pipeline for publish status): Website: https://cadlwebsite.z1.web.core.windows.net/prs/1796/ |
@@ -337,6 +337,9 @@ extern dec projectedName(target: unknown, targetName: string, projectedName: str | |||
*/ | |||
extern dec discriminator(target: object | Union, propertyName: string); | |||
|
|||
alias KnownDateFormat = "rfc1123" | "rfc7231" | "unixTimeStamp"; | |||
extern dec dateFormat(target: zonedDateTime | ModelProperty, format: KnownDateFormat | string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does @dateFormat
make the most sense as the name and not @dateSerializationFormat
. It is different from @format
on a string which actually dictate how the string is formatted vs here we are telling what serialization pattern to use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like shorter names, but dateSerializationFormat is more accurate
@@ -337,6 +337,9 @@ extern dec projectedName(target: unknown, targetName: string, projectedName: str | |||
*/ | |||
extern dec discriminator(target: object | Union, propertyName: string); | |||
|
|||
alias KnownDateFormat = "rfc1123" | "rfc7231" | "unixTimeStamp"; | |||
extern dec dateFormat(target: zonedDateTime | ModelProperty, format: KnownDateFormat | string); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like shorter names, but dateSerializationFormat is more accurate
deepStrictEqual(res.schemas.MyDate, { type: "string", format: "date-time" }); | ||
}); | ||
|
||
it("set format to 'timestamp' and type to 'ingeger' for 'unixtimestamp' format", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it("set format to 'timestamp' and type to 'ingeger' for 'unixtimestamp' format", async () => { | |
it("set format to 'timestamp' and type to 'integer' for 'unixtimestamp' format", async () => { |
|
||
|
||
```typespec | ||
dec dateFormat(target: zonedDateTime | ModelProperty, format: rfc1123 | rfc7231 | unixTimeStamp | string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values should be rfc3339 and rfc7231. rfc7231 is the replacement for rfc1123
@@ -1307,6 +1308,16 @@ function createOAPIEmitter(program: Program, options: ResolvedOpenAPI3EmitterOpt | |||
newTarget.format = "password"; | |||
} | |||
|
|||
const dateFormat = getDateFormat(program, typespecType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also set the default dateFormat, which requires knowing whether the date is used in a header (rfc7231) or elsewhere (rfc 3339)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’ll in openapi date-time already assume that from what I understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, no, see: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md?plain=1#L170
date-time should be the default for non-header types, but for headers, we need to be explicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, I’ll update
@@ -189,4 +189,56 @@ describe("openapi3: primitives", () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe("date format", () => { | |||
it("set format to 'date-time' by default", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have an alternate where this is ued in a header, and the default format is rfc-7231
Sorry forgot the context, remind me why this isn't just the existing format decorator? Also how does this combine with utcDateTime and offsetDateTime? What are the semantic differences between say a utcDateTime without a format and an offsetDateTime with a unixTimestamp format? |
Co-authored-by: Mark Cowlishaw <markcowl@microsoft.com>
@bterlson Think that design discussion was done when I was one leave so can’t say about what was discussed but from my comment above I think it is different from format as it means serialization format vs format means a know pattern for the string. for the utc and offset date time I am not fully sure either what was the rationale behind that distinction but I guess that would mean you shouldn’t do Unix time stamp for offsetdattime |
I was probably present for the discussion but I've forgotten it. Forgive me! (Also I've spent a bunch of time in Json Schema land and paging back in arcane date knowledge) I worry that this decorator's design isn't fully rationalized with existing/planned features so it might be worth circling back a little? Some thoughts: The existing format decorator isn't just for string types (despite what the docs say, int16 is an example integer format). Json Schema leaves format unbounded and allows it to be used for any type. We should probably too? (Or the json schema emitter will need yet another format decorator?) The Json Schema and OAS include serialization format in its format registry - format: "date-time" requires RFC3339. Seems reasonable to say that we're extending this list with some additional known formats. A somewhat orthogonal axis might make it hard for OAS and JsonSchema emitters to know what combinations are valid/when to emit a registered format vs. fall back to pattern or something? The idea behind utcDateTime and offsetDateTime is to describe the contract the service is exposing with regard to whether it expects/persists/round trips time zones and offsets (zonedDateTime), offsets (offsetDateTime) or just ticks since epoch in UTC (utcDateTime). Format is mostly orthogonal to this notion, except that some formats might imply information loss depending on type it's applied to. I think this should be an error probably? |
My argument for not using format still applies. In json schema we are saying the value is a string with a specific format (date time). Or an int being something more specific maybe. but here we have already that distinction of a type which is where it feels weird to me
maybe this is not the right solution but format to me sounds like the wrong one as well |
Doesn't format: date-time in OpenAPI/Json Schema describe the serialization format moreso than the data type? date-time requires RFC3339, meanwhile format is type-agnostic, the type is described by the |
@bterlson I guess it does technically but to me it still sounds like Like if we look at those format in jsonschema:
Using format on the date times would to me imply similar style of pattern restriction Like, not that it make sense but if you said |
In a world where pattern can apply to any type, isn't serialization format just a pattern restriction? I see your point if we say |
I guess an argument against that decorator is, let’s say we have this new geolocation type, I guess that also needs to specify how to serialize it. Seems overkill to have to have a new decorator per month primitive type that might serialize in many different ways |
|
||
#### Target | ||
|
||
`union zonedDateTime | ModelProperty` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this work for duration as well?
Note that, if we have a core decorator that serves the purpose of
-or - type-specific format decorators // we decided against type-specific decorators
|
|
Replaced by updated |
fix #282