-
Notifications
You must be signed in to change notification settings - Fork 83
Fix Go example generator emitting invalid Go for RFC7231 datetime values #4984
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
Open
Copilot
wants to merge
9
commits into
main
Choose a base branch
from
copilot/fix-invalid-go-rfc7231-datetime
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
778f5dc
Initial plan
Copilot 39c538b
Fix Go example generator for RFC7231 datetime values
Copilot 4c6f4f9
Add chronus changelog for RFC7231 example fix
Copilot a87ab32
Add scenario test, chronus change via pnpm change add, bump version, …
Copilot 4922663
Merge branch 'main' into copilot/fix-invalid-go-rfc7231-datetime
JiaqiZhang-Dev 8937e4a
update changelog
JiaqiZhang-Dev af66c33
Merge remote-tracking branch 'origin/main' into copilot/fix-invalid-g…
Copilot bd781da
Merge branch 'main' into copilot/fix-invalid-go-rfc7231-datetime
tadelesh 3dc1ab9
Revert typespec-go version bump and remove version-bump doc step
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
.chronus/changes/copilot-fix-invalid-go-rfc7231-datetime-2026-6-21-5-57-23.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: fix | ||
| packages: | ||
| - "@azure-tools/typespec-go" | ||
| --- | ||
|
|
||
| Fix the example generator emitting incorrect code for `rfc7231` datatime. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 changes: 5 additions & 0 deletions
5
packages/typespec-go/test/unittest/scenario-suites/rfc7231-datetime-sample.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| // Generated by `pnpm gen:scenario-suites`. Do not edit by hand. | ||
| import { resolvePath } from "@typespec/compiler"; | ||
| import { describeScenarioFile } from "../scenario-runner.js"; | ||
|
|
||
| describeScenarioFile(resolvePath(import.meta.dirname, "../scenarios/rfc7231-datetime-sample.md")); |
136 changes: 136 additions & 0 deletions
136
packages/typespec-go/test/unittest/scenarios/rfc7231-datetime-sample.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| # A `utcDateTime` property encoded as `rfc7231` generates a sample that parses the value with `time.RFC1123` | ||
|
|
||
| ```yaml | ||
| generate-samples: true | ||
| ``` | ||
|
|
||
| ## TypeSpec | ||
|
|
||
| ```tsp | ||
| @armProviderNamespace | ||
| namespace Microsoft.Test; | ||
|
|
||
| union ProvisioningState { | ||
| string, | ||
| Succeeded: "Succeeded", | ||
| Failed: "Failed", | ||
| Canceled: "Canceled", | ||
| } | ||
|
|
||
| model WidgetProperties { | ||
| description?: string; | ||
|
|
||
| @encode("rfc7231") | ||
| lastExecuted?: utcDateTime; | ||
|
|
||
| @visibility(Lifecycle.Read) | ||
| provisioningState?: ProvisioningState; | ||
| } | ||
|
|
||
| model Widget is TrackedResource<WidgetProperties> { | ||
| @path | ||
| @key("widgetName") | ||
| @segment("widgets") | ||
| name: string; | ||
| } | ||
|
|
||
| @armResourceOperations | ||
| interface Widgets { | ||
| put is ArmResourceCreateOrReplaceSync<Widget>; | ||
| } | ||
| ``` | ||
|
|
||
| ## The example inputs | ||
|
|
||
| ```json examples/Widgets_put.json | ||
| { | ||
| "operationId": "Widgets_put", | ||
| "title": "Widgets_put", | ||
| "parameters": { | ||
| "api-version": "2025-01-01", | ||
| "subscriptionId": "00000000-0000-0000-0000-000000000000", | ||
| "resourceGroupName": "myResourceGroup", | ||
| "widgetName": "myWidget", | ||
| "resource": { | ||
| "location": "eastus", | ||
| "properties": { | ||
| "description": "my widget", | ||
| "lastExecuted": "05 May 2021 12:00:23 GMT" | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { | ||
| "body": { | ||
| "id": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroup/providers/Microsoft.Test/widgets/myWidget", | ||
| "name": "myWidget", | ||
| "type": "Microsoft.Test/widgets", | ||
| "location": "eastus", | ||
| "properties": { | ||
| "description": "my widget", | ||
| "lastExecuted": "05 May 2021 12:00:23 GMT", | ||
| "provisioningState": "Succeeded" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## The generated sample parses the `rfc7231` datetime with `time.RFC1123` | ||
|
|
||
| ```go widgets_client_example_test | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
| // Code generated by Microsoft (R) Go Code Generator. DO NOT EDIT. | ||
|
|
||
| package testmodule_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" | ||
| "github.com/Azure/azure-sdk-for-go/sdk/azidentity" | ||
| "log" | ||
| "testmodule" | ||
| "time" | ||
| ) | ||
|
|
||
| // Generated from example definition: Widgets_put.json | ||
| func ExampleWidgetsClient_Put() { | ||
| cred, err := azidentity.NewDefaultAzureCredential(nil) | ||
| if err != nil { | ||
| log.Fatalf("failed to obtain a credential: %v", err) | ||
| } | ||
| ctx := context.Background() | ||
| clientFactory, err := testmodule.NewClientFactory("00000000-0000-0000-0000-000000000000", cred, nil) | ||
| if err != nil { | ||
| log.Fatalf("failed to create client: %v", err) | ||
| } | ||
| res, err := clientFactory.NewWidgetsClient().Put(ctx, "2025-01-01", "myResourceGroup", "myWidget", testmodule.Widget{ | ||
| Location: to.Ptr("eastus"), | ||
| Properties: &testmodule.WidgetProperties{ | ||
| Description: to.Ptr("my widget"), | ||
| LastExecuted: to.Ptr(func() time.Time { t, _ := time.Parse(time.RFC1123, "05 May 2021 12:00:23 GMT"); return t }()), | ||
| }, | ||
| }, nil) | ||
| if err != nil { | ||
| log.Fatalf("failed to finish the request: %v", err) | ||
| } | ||
| // You could use response here. We use blank identifier for just demo purposes. | ||
| _ = res | ||
| // If the HTTP response code is 200 as defined in example definition, your response structure would look as follows. Please pay attention that all the values in the output are fake values for just demo purposes. | ||
| // res = testmodule.WidgetsClientPutResponse{ | ||
| // Widget: testmodule.Widget{ | ||
| // ID: to.Ptr("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myResourceGroup/providers/Microsoft.Test/widgets/myWidget"), | ||
| // Name: to.Ptr("myWidget"), | ||
| // Type: to.Ptr("Microsoft.Test/widgets"), | ||
| // Location: to.Ptr("eastus"), | ||
| // Properties: &testmodule.WidgetProperties{ | ||
| // Description: to.Ptr("my widget"), | ||
| // LastExecuted: to.Ptr(func() time.Time { t, _ := time.Parse(time.RFC1123, "05 May 2021 12:00:23 GMT"); return t}()), | ||
| // ProvisioningState: to.Ptr(testmodule.ProvisioningStateSucceeded), | ||
| // }, | ||
| // }, | ||
| // } | ||
| } | ||
| ``` | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Callers should never need to manually parse
time.Timeinto the wire format (we go to great lengths to not leak that implementation detail). Why do we need to do this here?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.
Since the example value is in wire format:
05 May 2021 12:00:23 GMT, we need to manually parse it intotime.Time. For sdk customers, they don't need this logic, just give thetime.Timeis ok.I'm not sure if you mean that we should analyze the wire format in the code, and then generate unified code like
to.Ptr(time.Date(2021, time.May, 5, 12, 0, 23, 0, time.UTC))Uh oh!
There was an error while loading. Please reload this page.
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.
Your example is correct.
The emitted code handles conversion to the correct wire format. All we should need to know is if we need to emit
time.UTCortime.Local(the former should be the vast majority of cases.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.
FYI I'm going to fix this as part of #4956 which is in progress.