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

Overriding collection properties makes enum generic type nullable #796

Closed
heaths opened this issue Jun 2, 2020 · 3 comments · Fixed by #797
Closed

Overriding collection properties makes enum generic type nullable #796

heaths opened this issue Jun 2, 2020 · 3 comments · Fixed by #797
Assignees
Labels
blocking-release Blocks release v3 Version 3 of AutoRest C# generator.

Comments

@heaths
Copy link
Member

heaths commented Jun 2, 2020

For Azure/azure-sdk-for-net#11712 I moved collection properties to a customized class with [CodeGenMember(EmptyAsUndefined = true, Initialize = true)]; however, this now causes the generator to declare the internal serialization constructor as IEnumerable<T?> - nullable, despite these types being non-nullable:

      "properties": {
        "ignoreScripts": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/CjkBigramTokenFilterScripts",
            "x-nullable": false
          },
          "description": "The scripts to ignore."
        },
      }

This ends up looking like:

        internal CjkBigramTokenFilter(string oDataType, string name, IList<CjkBigramTokenFilterScripts?> ignoreScripts, bool? outputUnigrams) : base(oDataType, name)
        {
            IgnoreScripts = ignoreScripts ?? new List<CjkBigramTokenFilterScripts>();
            OutputUnigrams = outputUnigrams;
            ODataType = oDataType ?? "#Microsoft.Azure.Search.CjkBigramTokenFilter";
        }

CjkBigramTokenFilterScripts is an enum with x-nullable as false, and even instantiation of a list gets it right, but the parameter should make the type nullable - the parameter itself could be, but since it's a reference type it naturally is already.

I can work around the problem by changing my declaration, but that is not the shape of the API we want. As such, this is blocking.

@heaths heaths added v3 Version 3 of AutoRest C# generator. blocking-release Blocks release labels Jun 2, 2020
@heaths
Copy link
Member Author

heaths commented Jun 2, 2020

/cc @tg-msft @AlexGhiondea

@heaths
Copy link
Member Author

heaths commented Jun 2, 2020

Note, the de/serialization code (in FileName.Serialization.cs) is also expecting it to be nullable, even though the array value is not nor was it when the generator was generating the properties:

        void IUtf8JsonSerializable.Write(Utf8JsonWriter writer)
        {
            writer.WriteStartObject();
            if (IgnoreScripts != null && IgnoreScripts.Any())
            {
                writer.WritePropertyName("ignoreScripts");
                writer.WriteStartArray();
                foreach (var item in IgnoreScripts)
                {
                    writer.WriteStringValue(item.Value.ToSerialString());
                }
                writer.WriteEndArray();
            }

// ...

       internal static CjkBigramTokenFilter DeserializeCjkBigramTokenFilter(JsonElement element)
        {
            IList<CjkBigramTokenFilterScripts?> ignoreScripts = default;
            bool? outputUnigrams = default;
            string odataType = default;
            string name = default;
            foreach (var property in element.EnumerateObject())
            {
                if (property.NameEquals("ignoreScripts"))
                {
                    if (property.Value.ValueKind == JsonValueKind.Null)
                    {
                        continue;
                    }
                    List<CjkBigramTokenFilterScripts?> array = new List<CjkBigramTokenFilterScripts?>();
                    foreach (var item in property.Value.EnumerateArray())
                    {
                        if (item.ValueKind == JsonValueKind.Null)
                        {
                            array.Add(null);
                        }
                        else
                        {
                            array.Add(item.GetString().ToCjkBigramTokenFilterScripts());
                        }
                    }
                    ignoreScripts = array;
                    continue;
                }

@heaths
Copy link
Member Author

heaths commented Jun 2, 2020

Note also, that if I change my property definition to use T?, the serialization is still wrong (tries creating a new List<T>), and the serializer isn't checking for null:

                    writer.WriteStringValue(item.Value.ToString());

If I remove my property declaration entirely and let the generator do it all, the code is correct throughout, indicating this isn't a regression, but some corner case bug with customized properties:

        // generated property:
        public IList<TokenFilterName> TokenFilters { get; set; }

        // constructor
        internal AnalyzeRequest(string text, LexicalAnalyzerName? analyzer, LexicalTokenizerName? tokenizer, IList<TokenFilterName> tokenFilters, IList<string> charFilters)
        {
            Text = text;
            Analyzer = analyzer;
            Tokenizer = tokenizer;
            TokenFilters = tokenFilters;
            CharFilters = charFilters ?? new List<string>();
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-release Blocks release v3 Version 3 of AutoRest C# generator.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants