Skip to content
This repository was archived by the owner on Oct 11, 2023. It is now read-only.

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

Merged
sarangan12 merged 3 commits into from
Sep 26, 2017
Merged

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6cbb2f
Code changes to remove condition to check for known primary type
sarangan12 Sep 8, 2017
160a707
Modify comment
sarangan12 Sep 8, 2017
fdab66e
Removing dead code
sarangan12 Sep 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/vanilla/ClientModelExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ private static string GetSeparator(this CollectionFormat format)
}

/// <summary>
/// Format the value of a sequence given the modeled element format. Note that only sequences of strings are supported.
/// Format the value of a sequence given the modeled element format.
/// </summary>
/// <param name="parameter">The parameter to format.</param>
/// <returns>A reference to the formatted parameter value.</returns>
Expand All @@ -198,10 +198,10 @@ public static string GetFormattedReferenceValue(this Parameter parameter)
primaryType = New<PrimaryType>(KnownPrimaryType.String);
}

if (primaryType == null || primaryType.KnownPrimaryType != KnownPrimaryType.String)
Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@sarangan12 sarangan12 Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
.....
.....

Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amarzavery could you please review this as well ?

Copy link
Copy Markdown
Contributor

@veronicagg veronicagg Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown

@amarzavery amarzavery Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@amarzavery amarzavery Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@sarangan12 sarangan12 Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (primaryType == null)
Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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}");
Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@sarangan12 sarangan12 Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment

}

return string.Format("{0}.nil? ? nil : {0}.join('{1}')", parameter.Name, parameter.CollectionFormat.GetSeparator());
Expand Down