Code changes to remove condition to check for known primary type #7

sarangan12 · 2017-09-08T18:46:28Z

When you execute the following command, you get the error:

C:\workspace\autorest.ruby>autorest https://raw.githubusercontent.com/Azure/azure-rest-api-specs/current/specification/trafficmanager/resource-manager/readme.md --output-folder=C:\workspace\temp --azure-arm=True --namespace=Azure::ARM::TrafficManager --package-name=azure_mgmt_traffic_manager --package-version=0.11.0 --ruby
AutoRest code generation utility.

(C) 2017 Microsoft Corporation.
https://aka.ms/autorest
Loading AutoRest extension '@microsoft.azure/autorest.ruby' (1.9.3)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (1.9.6)
FATAL: System.InvalidOperationException: Cannot generate a formatted sequence from a non-string array parameter AutoRest.Ruby.Model.ParameterRb
   at AutoRest.Ruby.ClientModelExtensions.GetFormattedReferenceValue(Parameter parameter) in C:\work\oneautorest\autorest.ruby\src\vanilla\ClientModelExtensions.cs:line 207
........
........

This error is happening because of a condition that was added (and removed in the current PR). Now, after modification, when you execute the command, the SDK is generated without any issues:

C:\workspace\autorest.ruby>autorest https://raw.githubusercontent.com/Azure/azure-rest-api-specs/current/specification/trafficmanager/resource-manager/readme.md --output-folder=C:\workspace\temp --azure-arm=True --namespace=Azure::ARM::TrafficManager --package-name=azure_mgmt_traffic_manager --package-version=0.11.0 --ruby --use=C:\workspace\autorest.ruby
AutoRest code generation utility.

(C) 2017 Microsoft Corporation.
https://aka.ms/autorest
Loading local AutoRest extension '@microsoft.azure/autorest.ruby' (C:\workspace\autorest.ruby)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (1.9.6)

msftclas · 2017-09-08T18:46:31Z

@sarangan12,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

vishrutshah · 2017-09-08T18:58:32Z

src/vanilla/ClientModelExtensions.cs

                primaryType = New<PrimaryType>(KnownPrimaryType.String);
            }

-            if (primaryType == null || primaryType.KnownPrimaryType != KnownPrimaryType.String)


Can you point the reference to the swagger specification where it says any data type is allowed instead of just string? Just for my reference :)

I see that nodeJs has the same case, in that case are they also broken ? https://github.com/Azure/autorest.nodejs/blob/master/src/vanilla/ClientModelExtensions.cs#L55

Can you point the reference to the swagger specification where it says any data type is allowed instead of just string?
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#itemsObject

I see that nodeJs has the same case, in that case are they also broken?

C:\workspace>autorest https://raw.githubusercontent.com/Azure/azure-rest-api-specs/current/specification/trafficmanager/resource-manager/readme.md --output-folder=C:\workspace\temp --azure-arm=True --namespace=Azure::ARM::TrafficManager --package-name=azure_mgmt_traffic_manager --package-version=0.11.0 --nodejs AutoRest code generation utility. (C) 2017 Microsoft Corporation. https://aka.ms/autorest Installing AutoRest extension '@microsoft.azure/autorest.nodejs' (1.9.3) Loading AutoRest extension '@microsoft.azure/autorest.modeler' (1.9.6) FATAL: System.InvalidOperationException: Cannot generate a formatted sequence from a non-string array parameter AutoRest.NodeJS.Model.ParameterJs at AutoRest.NodeJS.ClientModelExtensions.GetFormattedReferenceValue(Parameter parameter) in D:\sdk\upstream\autorest.nodejs\src\vanilla\ClientModelExtensions.cs:line 61 ..... .....

@amarzavery could you please review this as well ?

Should we be expanding the condition to include"string", "number", "integer", "boolean", or "array", as those are the only types that seem to be allowed per the spec? (I'm not sure "array" makes sense in this case).

This is what the spec says

type string Required. The type of the parameter. Since the parameter is not located at the request body, it is limited to simple types (that is, not an object). The value MUST be one of "string", "number", "integer", "boolean", "array" or "file". If type is "file", the consumes MUST be either "multipart/form-data", " application/x-www-form-urlencoded" or both and the parameter MUST be in "formData".

So technically you could have an array of array of primary type as well.
C# generator is not checking anything over here. https://github.com/Azure/autorest.csharp/blob/master/src/vanilla/ClientModelExtensions.cs#L204 That check was removed completely. I think it is safe to do wha C# has done.

Here is my PR for nodejs Azure/autorest.nodejs#8

👍 Let's make the identical changes to the .NET and nodejs

NodeJS PR will be handled by amarsavery. For .NET SDK, I have created the issue https://github.com/Azure/autorest/issues/2589

vishrutshah · 2017-09-08T18:59:00Z

src/vanilla/ClientModelExtensions.cs

            {
                throw new InvalidOperationException(
-                    "Cannot generate a formatted sequence from a " + $"non-string array parameter {parameter}");
+                    "Cannot generate a formatted sequence from a " + $"null parameter {parameter}");


please update the comment to the function if any primary type is supported.

Updated the comment

msftclas · 2017-09-08T19:50:49Z

@sarangan12,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

veronicagg

I've added a comment inline.
One more question: is there any type of "deserialization" of the collection format we're doing anywhere, that would be impacted because the elements may not be strings anymore?
From customer usage point of view, is there any expectation or facility we're providing to construct the CSV (is there anything in the SDK that indicates the type of the values, so the service can deserialize them on the other side per its expectation? - because when they are joined together, they are a "string" for sure)

veronicagg · 2017-09-08T23:54:13Z

Referencing the spec and where it's having trouble: https://github.com/Azure/azure-rest-api-specs/blob/ee1746258548ed189a07b6ce86c13fe11a615cea/specification/trafficmanager/resource-manager/Microsoft.Network/2017-09-01-preview/trafficmanager.json#L732

sarangan12 · 2017-09-15T18:03:00Z

is there any type of "deserialization" of the collection format we're doing anywhere, that would be impacted because the elements may not be strings anymore?

Nope

vishrutshah

Also please add the test case so that we don't miss it. Thanks!

vishrutshah · 2017-09-15T18:12:33Z

src/vanilla/ClientModelExtensions.cs

            }

-            if (primaryType == null || primaryType.KnownPrimaryType != KnownPrimaryType.String)
+            if (primaryType == null)


seems like primaryType is not used anywhere. LEt's remove it

olydis · 2017-09-26T22:40:26Z

publish job

success (version: 2.0.15)

…ype (#7)" This reverts commit 5b1323c.

…Resource for azure-arm Ruby generator" (#12) * Revert "Code changes to remove condition to check for known primary type (#7)" This reverts commit 5b1323c. * Revert "Updating the inheritance logic for Resoruce & SubResource for azure-arm Ruby generator (#9)" This reverts commit b14c6ef.

veronicagg · 2017-09-28T22:23:22Z

Fixing Azure/azure-sdk-for-ruby#944

Code changes to remove condition to check for known primary type

b6cbb2f

sarangan12 requested review from devigned, veronicagg and vishrutshah September 8, 2017 18:46

msftclas added the cla-not-required label Sep 8, 2017

vishrutshah suggested changes Sep 8, 2017

View reviewed changes

Modify comment

160a707

veronicagg closed this Sep 8, 2017

veronicagg reopened this Sep 8, 2017

msftclas added the cla-not-required label Sep 8, 2017

veronicagg suggested changes Sep 8, 2017

View reviewed changes

vishrutshah mentioned this pull request Sep 9, 2017

[azure_mgmt_*] Prepare to release azure_mgmt_* version 0.12.0 Azure/azure-sdk-for-ruby#962

Merged

9 tasks

olydis closed this Sep 13, 2017

olydis reopened this Sep 13, 2017

msftclas added the cla-not-required label Sep 13, 2017

Azure deleted a comment from msftclas Sep 13, 2017

sarangan12 mentioned this pull request Sep 13, 2017

Release by 25th Sept with updated specs Azure/azure-sdk-for-ruby#938

Closed

vishrutshah suggested changes Sep 15, 2017

View reviewed changes

Removing dead code

fdab66e

msftgits removed the cla-not-required label Sep 26, 2017

sarangan12 mentioned this pull request Sep 26, 2017

Add test case for known primary type Azure/autorest#2612

Closed

sarangan12 merged commit 5b1323c into Azure:master Sep 26, 2017

vishrutshah added a commit that referenced this pull request Sep 27, 2017

Revert "Code changes to remove condition to check for known primary t…

53bf77a

…ype (#7)" This reverts commit 5b1323c.

sarangan12 mentioned this pull request Nov 7, 2019

Issue when the collectiveformat is csv and primary type is not string Azure/autorest.csharp#126

Closed

Code changes to remove condition to check for known primary type #7

Code changes to remove condition to check for known primary type #7

Uh oh!

Conversation

sarangan12 commented Sep 8, 2017

Uh oh!

msftclas commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msftclas commented Sep 8, 2017

Uh oh!

veronicagg left a comment

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarangan12 commented Sep 15, 2017

Uh oh!

vishrutshah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olydis commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

publish job

success (version: 2.0.15)

Uh oh!

veronicagg commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

veronicagg commented Sep 8, 2017 •

edited

Loading

vishrutshah left a comment •

edited

Loading

olydis commented Sep 26, 2017 •

edited

Loading