KyleSchoonover commented on a change in pull request #1597: URL: https://github.com/apache/avro/pull/1597#discussion_r826215496
########## File path: lang/csharp/src/apache/main/Schema/Aliases.cs ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace Avro +{ + internal static class Aliases + { + internal static IList<SchemaName> GetSchemaNames(IEnumerable<string> aliases, string enclosingTypeName, string enclosingTypeNamespace) + { + if (aliases == null) + return null; + + var enclosingSchemaName = new SchemaName(enclosingTypeName, enclosingTypeNamespace, null, null); Review comment: Can you replace var with the actual name of the object? ########## File path: lang/csharp/src/apache/main/Schema/EnumSchema.cs ########## @@ -112,6 +140,21 @@ internal static EnumSchema NewInstance(JToken jtok, PropertyMap props, SchemaNam Default = defaultSymbol; } + private static IDictionary<string, int> CreateSymbolsMap(IEnumerable<string> symbols) + { + IDictionary<string, int> symbolMap = new Dictionary<string, int>(); + int i = 0; + foreach (var symbol in symbols) + { + if (symbolMap.ContainsKey(symbol)) Review comment: Add brackets ########## File path: lang/csharp/src/apache/main/Schema/MapSchema.cs ########## @@ -68,8 +68,9 @@ internal static MapSchema NewInstance(JToken jtok, PropertyMap props, SchemaName /// Constructor for map schema class /// </summary> /// <param name="valueSchema">schema for map values type</param> - /// <param name="props">dictionary that provides access to custom properties</param> - private MapSchema(Schema valueSchema, PropertyMap props) : base(Type.Map, props) + /// <param name="cutsomProperties">dictionary that provides access to custom properties</param> + public MapSchema(Schema valueSchema, PropertyMap cutsomProperties = null) Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/EnumSchema.cs ########## @@ -47,6 +48,33 @@ public class EnumSchema : NamedSchema /// </summary> public int Count { get { return Symbols.Count; } } + /// <summary> + /// Initializes a new instance of the <see cref="EnumSchema"/> class. + /// </summary> + /// <param name="name">name of enum.</param> + /// <param name="space">namespace of enum.</param> + /// <param name="aliases">list of aliases for the name.</param> + /// <param name="symbols">list of enum symbols.</param> + /// <param name="customProperties">custom properties on this schema.</param> + /// <param name="doc">documentation for this named schema.</param> + /// <param name="defaultSymbol"></param> + public EnumSchema(string name, Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/ArraySchema.cs ########## @@ -48,11 +48,12 @@ internal static ArraySchema NewInstance(JToken jtok, PropertyMap props, SchemaNa } /// <summary> - /// Constructor + /// Initializes a new instance of the <see cref="ArraySchema"/> class. /// </summary> - /// <param name="items">schema for the array items type</param> - /// <param name="props">dictionary that provides access to custom properties</param> - private ArraySchema(Schema items, PropertyMap props) : base(Type.Array, props) + /// <param name="items">schema for the array items type.</param> + /// <param name="customAttributes">dictionary that provides access to custom properties.</param> + public ArraySchema(Schema items, PropertyMap customAttributes = null) Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/Aliases.cs ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace Avro +{ + internal static class Aliases + { + internal static IList<SchemaName> GetSchemaNames(IEnumerable<string> aliases, string enclosingTypeName, string enclosingTypeNamespace) + { + if (aliases == null) Review comment: Can you add brackets? ########## File path: lang/csharp/src/apache/main/Schema/Aliases.cs ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +using System; +using System.Collections.Generic; Review comment: Can you remove and sort usings. Assuming you are using VS. ########## File path: lang/csharp/src/apache/main/Schema/Field.cs ########## @@ -105,6 +105,29 @@ public enum SortOrder /// </summary> internal static JTokenEqualityComparer JtokenEqual = new JTokenEqualityComparer(); + /// <summary> + /// Initializes a new instance of the <see cref="Field"/> class. + /// </summary> + /// <param name="schema">schema for the field type.</param> + /// <param name="name">name of the field.</param> + /// <param name="aliases">list of aliases for the name of the field.</param> + /// <param name="pos">position of the field.</param> + /// <param name="doc">documentation for the field.</param> + /// <param name="defaultValue">field's default value if it exists.</param> + /// <param name="sortorder">sort order of the field.</param> + /// <param name="customProperties">dictionary that provides access to custom properties.</param> + public Field(Schema schema, Review comment: Is there a reason you didn't modify the existing constructor? ########## File path: lang/csharp/src/apache/main/Schema/EnumSchema.cs ########## @@ -112,6 +140,21 @@ internal static EnumSchema NewInstance(JToken jtok, PropertyMap props, SchemaNam Default = defaultSymbol; } + private static IDictionary<string, int> CreateSymbolsMap(IEnumerable<string> symbols) Review comment: Document usage and exception case in method summary, params, and exceptions ########## File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs ########## @@ -165,10 +284,19 @@ private static Field createField(JToken jfield, int pos, SchemaNames names, stri private static void addToFieldMap(Dictionary<string, Field> map, string name, Field field) { if (map.ContainsKey(name)) - throw new SchemaParseException("field or alias " + name + " is a duplicate name"); + throw new AvroException("field or alias " + name + " is a duplicate name"); Review comment: Don't change exception type. Potentially breaking. You would want to mark the old method obsolete and recreate with new exception type. ########## File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs ########## @@ -28,10 +29,28 @@ namespace Avro /// </summary> public class RecordSchema : NamedSchema { + private List<Field> fields; Review comment: rename _fields ########## File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs ########## @@ -41,11 +60,111 @@ public class RecordSchema : NamedSchema /// <summary> /// Map of field name and Field object for faster field lookups /// </summary> - private readonly IDictionary<string, Field> fieldLookup; + private IDictionary<string, Field> fieldLookup; - private readonly IDictionary<string, Field> fieldAliasLookup; + private IDictionary<string, Field> fieldAliasLookup; private bool request; + /// <summary> + /// Constructor for the record schema + /// </summary> + /// <param name="name">name of the record schema</param> + /// <param name="fields">list of fields for the record</param> + /// <param name="space">type of record schema, either record or error</param> + /// <param name="aliases">list of aliases for the record name</param> + /// <param name="customProperties">custom properties on this schema</param> + /// <param name="doc">documentation for this named schema</param> + public RecordSchema(string name, Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -161,5 +164,17 @@ public override int GetHashCode() result += getHashCode(Props); return result; } + + private void VerifyChildSchemas(List<Schema> schemas) + { + if (schemas.Any(schema => schema.Tag == Type.Union)) + throw new ArgumentException("Unions may not immediately contain other unions", nameof(schemas)); + + var groupedByFullNames = schemas.GroupBy(schema => schema.Fullname); + IGrouping<string, Schema> duplicateType = groupedByFullNames.FirstOrDefault(x => x.Count() > 1); + + if (duplicateType != null) + throw new ArgumentException($"Duplicate type in union: {duplicateType.Key}"); Review comment: add brackets ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -20,6 +20,7 @@ using System.Text; using Newtonsoft.Json.Linq; using Newtonsoft.Json; +using System.Linq; Review comment: Remove and sort usings ########## File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs ########## @@ -28,10 +29,28 @@ namespace Avro /// </summary> public class RecordSchema : NamedSchema { + private List<Field> fields; + /// <summary> /// List of fields in the record /// </summary> - public List<Field> Fields { get; private set; } + public List<Field> Fields + { + get + { + return fields; + } + + set + { + VerifyFieldsPositions(value); Review comment: We shouldn't throw exceptions in properties ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -71,11 +72,13 @@ internal static UnionSchema NewInstance(JArray jarr, PropertyMap props, SchemaNa /// Contructor for union schema /// </summary> /// <param name="schemas"></param> - /// <param name="props">dictionary that provides access to custom properties</param> - private UnionSchema(List<Schema> schemas, PropertyMap props) : base(Type.Union, props) + /// <param name="customProperties">dictionary that provides access to custom properties</param> + public UnionSchema(List<Schema> schemas, PropertyMap customProperties = null) Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -161,5 +164,17 @@ public override int GetHashCode() result += getHashCode(Props); return result; } + + private void VerifyChildSchemas(List<Schema> schemas) + { + if (schemas.Any(schema => schema.Tag == Type.Union)) + throw new ArgumentException("Unions may not immediately contain other unions", nameof(schemas)); Review comment: Add brackets ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -161,5 +164,17 @@ public override int GetHashCode() result += getHashCode(Props); return result; } + + private void VerifyChildSchemas(List<Schema> schemas) + { + if (schemas.Any(schema => schema.Tag == Type.Union)) + throw new ArgumentException("Unions may not immediately contain other unions", nameof(schemas)); + + var groupedByFullNames = schemas.GroupBy(schema => schema.Fullname); Review comment: replace var with actual type ########## File path: lang/csharp/src/apache/main/Schema/PrimitiveSchema.cs ########## @@ -17,6 +17,8 @@ */ using System; using System.Linq; +using System.Collections.Generic; Review comment: Remove and sort usings ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -161,5 +164,17 @@ public override int GetHashCode() result += getHashCode(Props); return result; } + + private void VerifyChildSchemas(List<Schema> schemas) Review comment: Document parameters and exception cases ########## File path: lang/csharp/src/apache/main/Schema/PrimitiveSchema.cs ########## @@ -30,8 +32,9 @@ public sealed class PrimitiveSchema : UnnamedSchema /// Constructor for primitive schema /// </summary> /// <param name="type"></param> - /// <param name="props">dictionary that provides access to custom properties</param> - private PrimitiveSchema(Type type, PropertyMap props) : base(type, props) + /// <param name="customProperties">dictionary that provides access to custom properties</param> + public PrimitiveSchema(Type type, PropertyMap customProperties = null) Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/FixedSchema.cs ########## @@ -32,6 +32,20 @@ public class FixedSchema : NamedSchema /// </summary> public int Size { get; set; } + /// <summary> + /// Initializes a new instance of the <see cref="FixedSchema"/> class. + /// </summary> + /// <param name="name">name of the fixed schema</param> + /// <param name="aliases">list of aliases for the name</param> + /// <param name="size">fixed size</param> + /// <param name="space">namespace of fixed</param> + /// <param name="customProperties">custom properties on this schema</param> + /// <param name="doc">documentation for this named schema</param> + public FixedSchema(string name, int size, string space = null, IEnumerable<string> aliases = null, PropertyMap customProperties = null, string doc = null) Review comment: Schema classes already use a Factory pattern method "NewInstance". I would recommend following the same pattern than exposing the constructor. ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -161,5 +164,17 @@ public override int GetHashCode() result += getHashCode(Props); return result; } + + private void VerifyChildSchemas(List<Schema> schemas) + { + if (schemas.Any(schema => schema.Tag == Type.Union)) + throw new ArgumentException("Unions may not immediately contain other unions", nameof(schemas)); Review comment: Update wording. May seems optional. "Unions can not..." ########## File path: lang/csharp/src/apache/main/Schema/UnionSchema.cs ########## @@ -161,5 +164,17 @@ public override int GetHashCode() result += getHashCode(Props); return result; } + + private void VerifyChildSchemas(List<Schema> schemas) + { + if (schemas.Any(schema => schema.Tag == Type.Union)) + throw new ArgumentException("Unions may not immediately contain other unions", nameof(schemas)); + + var groupedByFullNames = schemas.GroupBy(schema => schema.Fullname); + IGrouping<string, Schema> duplicateType = groupedByFullNames.FirstOrDefault(x => x.Count() > 1); + + if (duplicateType != null) Review comment: You can move your linq statement into the if. There is no reason to create a variable that is not used in another place. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
