You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/08/12 02:49:16 UTC
[GitHub] [avro] KhrystynaPopadyuk opened a new pull request, #1823: Avro 3603 dotnet reflect refactoring 1
KhrystynaPopadyuk opened a new pull request, #1823:
URL: https://github.com/apache/avro/pull/1823
**Jira**
My PR addresses the following [Avro Jira](https://issues.apache.org/jira/browse/AVRO-3603) issues and references them in the PR title.
**Tests**
TBD
**Commits**
My commits all reference Jira issues in their subject lines.
**Documentation**
TBU
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #1823: Avro 3603 dotnet reflect refactoring 1
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #1823:
URL: https://github.com/apache/avro/pull/1823#discussion_r944142972
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ ConcurrentDictionary<string, Schema> previousFields = new ConcurrentDictionary<string, Schema>();
Review Comment:
## Container contents are never accessed
The contents of this container are never accessed.
[Show more details](https://github.com/apache/avro/security/code-scanning/2892)
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ ConcurrentDictionary<string, Schema> previousFields = new ConcurrentDictionary<string, Schema>();
Review Comment:
## Useless assignment to local variable
This assignment to [previousFields](1) is useless, since its value is never read.
[Show more details](https://github.com/apache/avro/security/code-scanning/2889)
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ ConcurrentDictionary<string, Schema> previousFields = new ConcurrentDictionary<string, Schema>();
+
+ switch (s)
+ {
+ case RecordSchema rs:
+ if (!objType.IsClass)
+ {
+ throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
+ }
+
+ if (typeof(byte[]).IsAssignableFrom(objType)
+ || typeof(string).IsAssignableFrom(objType)
+ || typeof(IEnumerable).IsAssignableFrom(objType)
+ || typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
+ }
+
+ AddClassNameMapItem(rs, objType);
+ var c = _refletCache.GetClass(rs.Fullname);
Review Comment:
## Useless assignment to local variable
This assignment to [c](1) is useless, since its value is never read.
[Show more details](https://github.com/apache/avro/security/code-scanning/2890)
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ ConcurrentDictionary<string, Schema> previousFields = new ConcurrentDictionary<string, Schema>();
+
+ switch (s)
+ {
+ case RecordSchema rs:
+ if (!objType.IsClass)
+ {
+ throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
+ }
+
+ if (typeof(byte[]).IsAssignableFrom(objType)
+ || typeof(string).IsAssignableFrom(objType)
+ || typeof(IEnumerable).IsAssignableFrom(objType)
+ || typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
+ }
+
+ AddClassNameMapItem(rs, objType);
+ var c = _refletCache.GetClass(rs.Fullname);
+ foreach (var f in rs.Fields)
Review Comment:
## Useless assignment to local variable
This assignment to [f](1) is useless, since its value is never read.
[Show more details](https://github.com/apache/avro/security/code-scanning/2891)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #1823: Avro 3603 dotnet reflect refactoring 1
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #1823:
URL: https://github.com/apache/avro/pull/1823#discussion_r944161766
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -179,126 +182,108 @@
/// <returns></returns>
public DotnetClass GetClass(RecordSchema schema)
{
- DotnetClass c;
- if (!_nameClassMap.TryGetValue(schema.Fullname, out c))
- {
- return null;
- }
-
- return c;
+ return (DotnetClass)GetClass(schema.Fullname);
}
/// <summary>
- /// Add an entry to the class cache.
+ /// Add an entry to the class cache.
/// </summary>
/// <param name="objType">Type of the C# class</param>
/// <param name="s">Schema</param>
+ [Obsolete()]
public void LoadClassCache(Type objType, Schema s)
{
- switch (s)
- {
- case RecordSchema rs:
- if (!objType.IsClass)
- {
- throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
- }
+ _reflectFactory.LoadClassCache(objType, s);
+ }
- if (typeof(byte[]).IsAssignableFrom(objType)
- || typeof(string).IsAssignableFrom(objType)
- || typeof(IEnumerable).IsAssignableFrom(objType)
- || typeof(IDictionary).IsAssignableFrom(objType))
- {
- throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
- }
+ // IReflectCache and IConverterService are implemented for backward compatibility
+ #region IReflectCahce
- AddClassNameMapItem(rs, objType);
- var c = GetClass(rs);
- foreach (var f in rs.Fields)
- {
- /*
- //.StackOverflowException
- var t = c.GetPropertyType(f);
- LoadClassCache(t, f.Schema);
- */
- if (_previousFields.TryAdd(f.Name, f.Schema))
- {
- var t = c.GetPropertyType(f);
- LoadClassCache(t, f.Schema);
- }
- }
+ /// <summary>
+ /// Find a class that matches the schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <returns></returns>
+ [Obsolete()]
+ public IDotnetClass GetClass(string schemaFullName)
+ {
+ IDotnetClass c;
+ if (!_nameClassMap.TryGetValue(schemaFullName, out c))
+ {
+ return null;
+ }
- break;
- case ArraySchema ars:
- if (!typeof(IEnumerable).IsAssignableFrom(objType))
- {
- throw new AvroException($"Cant map type {objType.Name} to array {ars.Name}");
- }
+ return c;
+ }
- if (!objType.IsGenericType)
- {
- throw new AvroException($"{objType.Name} needs to be a generic type");
- }
+ /// <summary>
+ /// Add a class that for schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <param name="dotnetClass"></param>
+ [Obsolete()]
+ public void AddClass(string schemaFullName, IDotnetClass dotnetClass)
+ {
+ _nameClassMap.TryAdd(schemaFullName, dotnetClass);
+ }
- LoadClassCache(objType.GenericTypeArguments[0], ars.ItemSchema);
- break;
- case MapSchema ms:
- if (!typeof(IDictionary).IsAssignableFrom(objType))
- {
- throw new AvroException($"Cant map type {objType.Name} to map {ms.Name}");
- }
+ /// <summary>
+ /// Find a enum type that matches the schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <returns></returns>
+ [Obsolete()]
+ public Type GetEnum(string schemaFullName)
+ {
+ return EnumCache.GetEnumeration(schemaFullName);
+ }
- if (!objType.IsGenericType)
- {
- throw new AvroException($"Cant map non-generic type {objType.Name} to map {ms.Name}");
- }
+ /// <summary>
+ /// Add a class that for schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <param name="enumType"></param>
+ [Obsolete()]
+ public void AddEnum(string schemaFullName, Type enumType)
+ {
+ EnumCache.AddEnumNameMapItem(schemaFullName, enumType);
+ }
- if (!typeof(string).IsAssignableFrom(objType.GenericTypeArguments[0]))
- {
- throw new AvroException($"First type parameter of {objType.Name} must be assignable to string");
- }
+ /// <summary>
+ /// Add an array helper type. Array helpers are used for collections that are not generic lists.
+ /// </summary>
+ /// <param name="arrayHelperName">Name of the helper. Corresponds to metadata "helper" field in the schema.</param>
+ public void AddArrayHelperType<T>(string arrayHelperName) where T : IArrayHelper
+ {
+ _nameArrayMap.TryAdd(arrayHelperName, typeof(T));
+ }
- LoadClassCache(objType.GenericTypeArguments[1], ms.ValueSchema);
- break;
- case NamedSchema ns:
- EnumCache.AddEnumNameMapItem(ns, objType);
- break;
- case UnionSchema us:
- if (us.Schemas.Count == 2 && (us.Schemas[0].Tag == Schema.Type.Null || us.Schemas[1].Tag == Schema.Type.Null))
- {
- // in this case objType will match the non null type in the union
- foreach (var o in us.Schemas)
- {
- if (o.Tag == Schema.Type.Null)
- {
- continue;
- }
+ /// <summary>
+ /// Find an array helper type for an array schema node.
+ /// </summary>
+ /// <param name="arrayHelperName">Schema</param>
+ /// <param name="arrayHelperType">Schema</param>
+ /// <returns></returns>
+ public bool TryGetArrayHelperType(string arrayHelperName, out Type arrayHelperType)
+ {
+ return _nameArrayMap.TryGetValue(arrayHelperName, out arrayHelperType);
+ }
- if (objType.IsClass)
- {
- LoadClassCache(objType, o);
- }
+ #endregion
- var innerType = Nullable.GetUnderlyingType(objType);
- if (innerType != null && innerType.IsEnum)
- {
- LoadClassCache(innerType, o);
- }
- }
- }
- else
- {
- // check the schema types are registered
- foreach (var o in us.Schemas)
- {
- if (o.Tag == Schema.Type.Record && GetClass(o as RecordSchema) == null)
- {
- throw new AvroException($"Class for union record type {o.Fullname} is not registered. Create a ClassCache object and call LoadClassCache");
- }
- }
- }
+ #region IConverterService
- break;
- }
+ /// <summary>
+ /// Get converter
+ /// </summary>
+ /// <param name="schema"></param>
+ /// <param name="property"></param>
+ /// <returns></returns>
+ public IAvroFieldConverter GetConverter(Schema schema, PropertyInfo property)
+ {
+ return _converterService.GetConverter(schema, property) ?? GetDefaultConverter(schema.Tag, property.PropertyType);
Review Comment:
## Call to obsolete method
Call to obsolete method [GetDefaultConverter](1).
[Show more details](https://github.com/apache/avro/security/code-scanning/2894)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #1823: Avro 3603 dotnet reflect refactoring 1
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #1823:
URL: https://github.com/apache/avro/pull/1823#discussion_r944079023
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -46,13 +64,14 @@
throw new AvroException($"Type {dotnetClass.Name} is not a class");
}
- _nameClassMap.TryAdd(schema.Fullname, new DotnetClass(dotnetClass, schema, this));
+ AddClass(schema.Fullname, new DotnetClass(dotnetClass, schema, this));
Review Comment:
## Call to obsolete method
Call to obsolete method [AddClass](1).
[Show more details](https://github.com/apache/avro/security/code-scanning/2858)
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -46,13 +64,14 @@
throw new AvroException($"Type {dotnetClass.Name} is not a class");
}
- _nameClassMap.TryAdd(schema.Fullname, new DotnetClass(dotnetClass, schema, this));
+ AddClass(schema.Fullname, new DotnetClass(dotnetClass, schema, this));
Review Comment:
## Dereferenced variable may be null
Variable [schema](1) may be null here as suggested by [this](2) null check.
[Show more details](https://github.com/apache/avro/security/code-scanning/2864)
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -179,20 +190,15 @@
/// <returns></returns>
public DotnetClass GetClass(RecordSchema schema)
{
- DotnetClass c;
- if (!_nameClassMap.TryGetValue(schema.Fullname, out c))
- {
- return null;
- }
-
- return c;
+ return GetClass(schema.Fullname);
Review Comment:
## Dereferenced variable may be null
Variable [schema](1) may be null here because of [this](2) potential null argument.
[Show more details](https://github.com/apache/avro/security/code-scanning/2865)
##########
lang/csharp/src/apache/main/Reflect/Service/ArrayService.cs:
##########
@@ -0,0 +1,63 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Array;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Additional functionality to serialize and deserialize arrays.
+ /// Works with array helpert
+ /// </summary>
+ public class ArrayService : IArrayService
+ {
+ private readonly IReflectCache _reflectCache;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ /// <param name="reflectCache"></param>
+ public ArrayService(IReflectCache reflectCache)
+ {
+ _reflectCache = reflectCache;
+ }
+
+ /// <summary>
+ /// Find an array helper for an array schema node.
+ /// </summary>
+ /// <param name="schema">Schema</param>
+ /// <param name="enumerable">The array object. If it is null then Add(), Count() and Clear methods will throw exceptions.</param>
+ /// <returns></returns>
+ public IArrayHelper GetArrayHelper(ArraySchema schema, IEnumerable enumerable)
+ {
+ string s = GetHelperName(schema);
+
+ if (s != null && _reflectCache.TryGetArrayHelperType(s, out Type arrayHelperType))
+ {
+ return (IArrayHelper)Activator.CreateInstance(arrayHelperType, enumerable);
+ }
+
+ return (IArrayHelper)Activator.CreateInstance(typeof(ArrayHelper), enumerable);
+ }
+
+ internal string GetHelperName(ArraySchema ars)
+ {
+ // ArraySchema is unnamed schema and doesn't have a FulllName, use "helper" metadata.
+ // Metadata is json string, strip quotes
+
+ string s = null;
+ s = ars.GetProperty("helper");
+ if (s != null && s.Length > 2)
+ {
+ s = s.Substring(1, s.Length - 2);
+ }
+ else
+ {
+ s = null;
+ }
Review Comment:
## Missed ternary opportunity
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
[Show more details](https://github.com/apache/avro/security/code-scanning/2874)
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -179,20 +190,15 @@
/// <returns></returns>
public DotnetClass GetClass(RecordSchema schema)
{
- DotnetClass c;
- if (!_nameClassMap.TryGetValue(schema.Fullname, out c))
- {
- return null;
- }
-
- return c;
+ return GetClass(schema.Fullname);
Review Comment:
## Call to obsolete method
Call to obsolete method [GetClass](1).
[Show more details](https://github.com/apache/avro/security/code-scanning/2859)
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +185,9 @@
return obj is string;
case Schema.Type.Error:
case Schema.Type.Record:
- return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+ return _reflectCache.GetClass(sc.Fullname).GetClassType() == obj.GetType();
Review Comment:
## Dereferenced variable may be null
Variable [obj](1) may be null here as suggested by [this](2) null check.
Variable [obj](1) may be null here as suggested by [this](3) null check.
[Show more details](https://github.com/apache/avro/security/code-scanning/2867)
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +185,9 @@
return obj is string;
case Schema.Type.Error:
case Schema.Type.Record:
- return _classCache.GetClass(sc as RecordSchema).GetClassType() == obj.GetType();
+ return _reflectCache.GetClass(sc.Fullname).GetClassType() == obj.GetType();
case Schema.Type.Enumeration:
- return EnumCache.GetEnumeration(sc as EnumSchema) == obj.GetType();
+ return _reflectCache.GetEnum(sc.Fullname) == obj.GetType();
Review Comment:
## Dereferenced variable may be null
Variable [obj](1) may be null here as suggested by [this](2) null check.
Variable [obj](1) may be null here as suggested by [this](3) null check.
[Show more details](https://github.com/apache/avro/security/code-scanning/2868)
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs:
##########
@@ -383,7 +381,7 @@
var i = d.ReadEnum();
var symbol = writerSchema[i];
var es = readerSchema as EnumSchema;
- var enumType = EnumCache.GetEnumeration(es);
+ var enumType = _reflectCache.GetEnum(es.Fullname);
Review Comment:
## Dereferenced variable may be null
Variable [es](1) may be null here because of [this](2) assignment.
[Show more details](https://github.com/apache/avro/security/code-scanning/2866)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #1823: Avro 3603 dotnet reflect refactoring 1
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #1823:
URL: https://github.com/apache/avro/pull/1823#discussion_r944128110
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -179,126 +179,108 @@
/// <returns></returns>
public DotnetClass GetClass(RecordSchema schema)
{
- DotnetClass c;
- if (!_nameClassMap.TryGetValue(schema.Fullname, out c))
- {
- return null;
- }
-
- return c;
+ return (DotnetClass)GetClass(schema.Fullname);
}
/// <summary>
- /// Add an entry to the class cache.
+ /// Add an entry to the class cache.
/// </summary>
/// <param name="objType">Type of the C# class</param>
/// <param name="s">Schema</param>
+ [Obsolete()]
public void LoadClassCache(Type objType, Schema s)
{
- switch (s)
- {
- case RecordSchema rs:
- if (!objType.IsClass)
- {
- throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
- }
+ _reflectFactory.LoadClassCache(objType, s);
+ }
- if (typeof(byte[]).IsAssignableFrom(objType)
- || typeof(string).IsAssignableFrom(objType)
- || typeof(IEnumerable).IsAssignableFrom(objType)
- || typeof(IDictionary).IsAssignableFrom(objType))
- {
- throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
- }
+ // IReflectCache is implemented for backward compatibility
+ #region IReflectCahce
- AddClassNameMapItem(rs, objType);
- var c = GetClass(rs);
- foreach (var f in rs.Fields)
- {
- /*
- //.StackOverflowException
- var t = c.GetPropertyType(f);
- LoadClassCache(t, f.Schema);
- */
- if (_previousFields.TryAdd(f.Name, f.Schema))
- {
- var t = c.GetPropertyType(f);
- LoadClassCache(t, f.Schema);
- }
- }
+ /// <summary>
+ /// Find a class that matches the schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <returns></returns>
+ [Obsolete()]
+ public IDotnetClass GetClass(string schemaFullName)
+ {
+ IDotnetClass c;
+ if (!_nameClassMap.TryGetValue(schemaFullName, out c))
+ {
+ return null;
+ }
- break;
- case ArraySchema ars:
- if (!typeof(IEnumerable).IsAssignableFrom(objType))
- {
- throw new AvroException($"Cant map type {objType.Name} to array {ars.Name}");
- }
+ return c;
+ }
- if (!objType.IsGenericType)
- {
- throw new AvroException($"{objType.Name} needs to be a generic type");
- }
+ /// <summary>
+ /// Add a class that for schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <param name="dotnetClass"></param>
+ [Obsolete()]
+ public void AddClass(string schemaFullName, IDotnetClass dotnetClass)
+ {
+ _nameClassMap.TryAdd(schemaFullName, dotnetClass);
+ }
- LoadClassCache(objType.GenericTypeArguments[0], ars.ItemSchema);
- break;
- case MapSchema ms:
- if (!typeof(IDictionary).IsAssignableFrom(objType))
- {
- throw new AvroException($"Cant map type {objType.Name} to map {ms.Name}");
- }
+ /// <summary>
+ /// Find a enum type that matches the schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <returns></returns>
+ [Obsolete()]
+ public Type GetEnum(string schemaFullName)
+ {
+ return EnumCache.GetEnumeration(schemaFullName);
+ }
- if (!objType.IsGenericType)
- {
- throw new AvroException($"Cant map non-generic type {objType.Name} to map {ms.Name}");
- }
+ /// <summary>
+ /// Add a class that for schema full name.
+ /// </summary>
+ /// <param name="schemaFullName"></param>
+ /// <param name="enumType"></param>
+ [Obsolete()]
+ public void AddEnum(string schemaFullName, Type enumType)
+ {
+ EnumCache.AddEnumNameMapItem(schemaFullName, enumType);
+ }
- if (!typeof(string).IsAssignableFrom(objType.GenericTypeArguments[0]))
- {
- throw new AvroException($"First type parameter of {objType.Name} must be assignable to string");
- }
+ /// <summary>
+ /// Add an array helper type. Array helpers are used for collections that are not generic lists.
+ /// </summary>
+ /// <param name="arrayHelperName">Name of the helper. Corresponds to metadata "helper" field in the schema.</param>
+ public void AddArrayHelperType<T>(string arrayHelperName) where T : IArrayHelper
+ {
+ _nameArrayMap.TryAdd(arrayHelperName, typeof(T));
+ }
- LoadClassCache(objType.GenericTypeArguments[1], ms.ValueSchema);
- break;
- case NamedSchema ns:
- EnumCache.AddEnumNameMapItem(ns, objType);
- break;
- case UnionSchema us:
- if (us.Schemas.Count == 2 && (us.Schemas[0].Tag == Schema.Type.Null || us.Schemas[1].Tag == Schema.Type.Null))
- {
- // in this case objType will match the non null type in the union
- foreach (var o in us.Schemas)
- {
- if (o.Tag == Schema.Type.Null)
- {
- continue;
- }
+ /// <summary>
+ /// Find an array helper type for an array schema node.
+ /// </summary>
+ /// <param name="arrayHelperName">Schema</param>
+ /// <param name="arrayHelperType">Schema</param>
+ /// <returns></returns>
+ public bool TryGetArrayHelperType(string arrayHelperName, out Type arrayHelperType)
+ {
+ return _nameArrayMap.TryGetValue(arrayHelperName, out arrayHelperType);
+ }
- if (objType.IsClass)
- {
- LoadClassCache(objType, o);
- }
+ #endregion
- var innerType = Nullable.GetUnderlyingType(objType);
- if (innerType != null && innerType.IsEnum)
- {
- LoadClassCache(innerType, o);
- }
- }
- }
- else
- {
- // check the schema types are registered
- foreach (var o in us.Schemas)
- {
- if (o.Tag == Schema.Type.Record && GetClass(o as RecordSchema) == null)
- {
- throw new AvroException($"Class for union record type {o.Fullname} is not registered. Create a ClassCache object and call LoadClassCache");
- }
- }
- }
+ #region IConverterService
- break;
- }
+ /// <summary>
+ /// Get converter
+ /// </summary>
+ /// <param name="schema"></param>
+ /// <param name="property"></param>
+ /// <returns></returns>
+ public IAvroFieldConverter GetConverter(Schema schema, PropertyInfo property)
+ {
+ return GetDefaultConverter(schema.Tag, property.PropertyType);
Review Comment:
## Call to obsolete method
Call to obsolete method [GetDefaultConverter](1).
[Show more details](https://github.com/apache/avro/security/code-scanning/2879)
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ Dictionary<string, Schema> previousFields = new Dictionary<string, Schema>();
+
+ switch (s)
+ {
+ case RecordSchema rs:
+ if (!objType.IsClass)
+ {
+ throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
+ }
+
+ if (typeof(byte[]).IsAssignableFrom(objType)
+ || typeof(string).IsAssignableFrom(objType)
+ || typeof(IEnumerable).IsAssignableFrom(objType)
+ || typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
+ }
+
+ AddClassNameMapItem(rs, objType);
+ var c = _refletCache.GetClass(rs.Fullname);
+ foreach (var f in rs.Fields)
+ {
+ /*
+ //.StackOverflowException
+ var t = c.GetPropertyType(f);
+ LoadClassCache(t, f.Schema);
+ */
+ if (!previousFields.ContainsKey(f.Name))
+ {
+ previousFields.Add(f.Name, f.Schema);
+ var t = c.GetPropertyType(f);
+ LoadClassCache(t, f.Schema);
+ }
+ }
Review Comment:
## Missed opportunity to use Where
This foreach loop implicitly filters its target sequence [here](1) - consider filtering the sequence explicitly using '.Where(...)'.
[Show more details](https://github.com/apache/avro/security/code-scanning/2887)
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -179,126 +179,108 @@
/// <returns></returns>
public DotnetClass GetClass(RecordSchema schema)
{
- DotnetClass c;
- if (!_nameClassMap.TryGetValue(schema.Fullname, out c))
- {
- return null;
- }
-
- return c;
+ return (DotnetClass)GetClass(schema.Fullname);
Review Comment:
## Call to obsolete method
Call to obsolete method [GetClass](1).
[Show more details](https://github.com/apache/avro/security/code-scanning/2878)
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ Dictionary<string, Schema> previousFields = new Dictionary<string, Schema>();
+
+ switch (s)
+ {
+ case RecordSchema rs:
+ if (!objType.IsClass)
+ {
+ throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
+ }
+
+ if (typeof(byte[]).IsAssignableFrom(objType)
+ || typeof(string).IsAssignableFrom(objType)
+ || typeof(IEnumerable).IsAssignableFrom(objType)
+ || typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
+ }
+
+ AddClassNameMapItem(rs, objType);
+ var c = _refletCache.GetClass(rs.Fullname);
+ foreach (var f in rs.Fields)
+ {
+ /*
+ //.StackOverflowException
+ var t = c.GetPropertyType(f);
+ LoadClassCache(t, f.Schema);
+ */
+ if (!previousFields.ContainsKey(f.Name))
+ {
+ previousFields.Add(f.Name, f.Schema);
+ var t = c.GetPropertyType(f);
+ LoadClassCache(t, f.Schema);
+ }
+ }
+
+ break;
+ case ArraySchema ars:
+ if (!typeof(IEnumerable).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to array {ars.Name}");
+ }
+
+ if (!objType.IsGenericType)
+ {
+ throw new AvroException($"{objType.Name} needs to be a generic type");
+ }
+
+ LoadClassCache(objType.GenericTypeArguments[0], ars.ItemSchema);
+ break;
+ case MapSchema ms:
+ if (!typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to map {ms.Name}");
+ }
+
+ if (!objType.IsGenericType)
+ {
+ throw new AvroException($"Cant map non-generic type {objType.Name} to map {ms.Name}");
+ }
+
+ if (!typeof(string).IsAssignableFrom(objType.GenericTypeArguments[0]))
+ {
+ throw new AvroException($"First type parameter of {objType.Name} must be assignable to string");
+ }
+
+ LoadClassCache(objType.GenericTypeArguments[1], ms.ValueSchema);
+ break;
+ case NamedSchema ns:
+ _refletCache.AddEnum(ns.Fullname, objType);
+ break;
+ case UnionSchema us:
+ if (us.Schemas.Count == 2 && (us.Schemas[0].Tag == Schema.Type.Null || us.Schemas[1].Tag == Schema.Type.Null))
+ {
+ // in this case objType will match the non null type in the union
+ foreach (var o in us.Schemas)
+ {
+ if (o.Tag == Schema.Type.Null)
+ {
+ continue;
+ }
+
+ if (objType.IsClass)
+ {
+ LoadClassCache(objType, o);
+ }
+
+ var innerType = Nullable.GetUnderlyingType(objType);
+ if (innerType != null && innerType.IsEnum)
+ {
+ LoadClassCache(innerType, o);
+ }
+ }
+ }
+ else
+ {
+ // check the schema types are registered
+ foreach (var o in us.Schemas)
+ {
+ if (o.Tag == Schema.Type.Record && _refletCache.GetClass(o.Fullname) == null)
+ {
+ throw new AvroException($"Class for union record type {o.Fullname} is not registered. Create a ClassCache object and call LoadClassCache");
+ }
+ }
+ }
+
+ break;
+ }
+ }
+
+ private void AddClassNameMapItem(RecordSchema schema, Type dotnetClass)
+ {
+ if (schema != null && _refletCache.GetClass(schema.Fullname) != null)
+ {
+ return;
+ }
+
+ if (!dotnetClass.IsClass)
+ {
+ throw new AvroException($"Type {dotnetClass.Name} is not a class");
+ }
+
+ _refletCache.AddClass(schema.Fullname, _dotnetclassFactory.CreateDotnetClass(schema, dotnetClass));
Review Comment:
## Dereferenced variable may be null
Variable [schema](1) may be null here as suggested by [this](2) null check.
[Show more details](https://github.com/apache/avro/security/code-scanning/2880)
##########
lang/csharp/src/apache/main/Reflect/Service/ConverterService.cs:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.Linq;
+using System.Collections.Generic;
+using System.Reflection;
+using System.Text;
+using Avro.Reflect.Converter;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Service to work with default IAvroFieldConverter
+ /// </summary>
+ public class ConverterService : IConverterService
+ {
+ private readonly IEnumerable<IAvroFieldConverter> _convertors;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ConverterService(IEnumerable<IAvroFieldConverter> convertors)
+ {
+ _convertors = convertors;
+ }
+
+ /// <summary>
+ /// Find a converter
+ /// </summary>
+ /// <param name="schema"></param>
+ /// <param name="property"></param>
+ /// <returns>The first matching converter - null if there isn't one</returns>
+ public IAvroFieldConverter GetConverter(Schema schema, PropertyInfo property)
+ {
+ return GetFieldConverter(schema, property) ?? GetRegisteredConverter(schema, property);
+ }
+
+ internal IAvroFieldConverter GetFieldConverter(Schema schema, PropertyInfo property)
+ {
+ var avroFieldConverters = property
+ .GetCustomAttributes(typeof(AvroFieldAttribute), true)
+ .Select(attr => ((AvroFieldAttribute)attr).Converter)
+ .Where(converter => converter != null)
+ .ToArray();
+
+ if (avroFieldConverters.Length > 1)
+ throw new AvroException($"More them one converter is defined by AvroFieldAttribute for '{property.Name}'.");
+
+ return avroFieldConverters.Length == 1 ? avroFieldConverters[0] : null;
+ }
+
+ internal IAvroFieldConverter GetRegisteredConverter(Schema schema, PropertyInfo property)
+ {
+ Type avroType;
+ switch (schema.Tag)
+ {
+ case Avro.Schema.Type.Null:
+ return null;
+ case Avro.Schema.Type.Boolean:
+ avroType = typeof(bool);
+ break;
+ case Avro.Schema.Type.Int:
+ avroType = typeof(int);
+ break;
+ case Avro.Schema.Type.Long:
+ avroType = typeof(long);
+ break;
+ case Avro.Schema.Type.Float:
+ avroType = typeof(float);
+ break;
+ case Avro.Schema.Type.Double:
+ avroType = typeof(double);
+ break;
+ case Avro.Schema.Type.Bytes:
+ avroType = typeof(byte[]);
+ break;
+ case Avro.Schema.Type.String:
+ avroType = typeof(string);
+ break;
+ case Avro.Schema.Type.Record:
+ return null;
+ case Avro.Schema.Type.Enumeration:
+ return null;
+ case Avro.Schema.Type.Array:
+ return null;
+ case Avro.Schema.Type.Map:
+ return null;
+ case Avro.Schema.Type.Union:
+ return null;
+ case Avro.Schema.Type.Fixed:
+ avroType = typeof(byte[]);
+ break;
+ case Avro.Schema.Type.Error:
+ return null;
+ default:
+ return null;
+ }
+
+ foreach (var c in _convertors)
+ {
+ if (c.GetAvroType() == avroType && c.GetPropertyType() == property.PropertyType)
+ {
+ return c;
+ }
+ }
Review Comment:
## Missed opportunity to use Where
This foreach loop implicitly filters its target sequence [here](1) - consider filtering the sequence explicitly using '.Where(...)'.
[Show more details](https://github.com/apache/avro/security/code-scanning/2886)
##########
lang/csharp/src/apache/main/Reflect/Service/DotnetclassFactory.cs:
##########
@@ -17,30 +17,92 @@
*/
using System;
-using System.Reflection;
using System.Collections;
+using System.Collections.Generic;
+using System.Reflection;
+using System.Text;
+using Avro.Reflect.Converter;
+using Avro.Reflect.Model;
-namespace Avro.Reflect
+namespace Avro.Reflect.Service
{
- internal class DotnetProperty
+ /// <summary>
+ /// Factory to create DotnetClass and related objects
+ /// </summary>
+ public class DotnetclassFactory : IDotnetclassFactory
{
- private PropertyInfo _property;
+ private readonly IConverterService _converterService;
- public IAvroFieldConverter Converter { get; set; }
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ /// <param name="converterService"></param>
+ public DotnetclassFactory(IConverterService converterService)
+ {
+ _converterService = converterService;
+ }
- private bool IsPropertyCompatible(Avro.Schema schema)
+ /// <summary>
+ /// Create IDotnetClass object
+ /// </summary>
+ /// <param name="schema"></param>
+ /// <param name="type"></param>
+ /// <returns></returns>
+ public IDotnetClass CreateDotnetClass(RecordSchema schema, Type type)
{
- Type propType;
- var schemaTag = schema.Tag;
+ Dictionary<string, IDotnetProperty> properties = new Dictionary<string, IDotnetProperty>();
- if (Converter == null)
+ foreach (var field in schema.Fields)
{
- propType = _property.PropertyType;
+ PropertyInfo property = GetPropertyInfo(field, type);
+ properties.Add(field.Name, CreateDotnetProperty(field.Schema, property));
}
- else
+
+ return new DotnetClass(type, properties);
+ }
+
+ /// <summary>
+ /// Create DotnetProperty object
+ /// </summary>
+ /// <param name="schema"></param>
+ /// <param name="property"></param>
+ /// <returns></returns>
+ private IDotnetProperty CreateDotnetProperty(Schema schema, PropertyInfo property)
+ {
+ var converter = _converterService.GetConverter(schema, property);
+ var type = converter?.GetPropertyType() ?? property.PropertyType;
+
+ if (IsTypeCompatibleWithSchema(schema, type))
+ return new DotnetProperty(property, converter);
+
+ throw new AvroException($"Property {property.Name} in object {property.DeclaringType} isn't compatible with Avro schema type {schema.Tag}");
+ }
+
+ private PropertyInfo GetPropertyInfo(Field field, Type type)
+ {
+ var prop = type.GetProperty(field.Name);
+ if (prop != null)
{
- propType = Converter.GetAvroType();
+ return prop;
}
+ foreach (var p in type.GetProperties())
+ {
+ foreach (var attr in p.GetCustomAttributes(true))
+ {
+ var avroAttr = attr as AvroFieldAttribute;
+ if (avroAttr != null && avroAttr.FieldName != null && avroAttr.FieldName == field.Name)
+ {
+ return p;
+ }
+ }
Review Comment:
## Missed opportunity to use OfType
This foreach loop immediately uses 'as' to coerce its iteration variable to another type [here](1) - consider using '.OfType(...)' instead.
[Show more details](https://github.com/apache/avro/security/code-scanning/2885)
##########
lang/csharp/src/apache/main/Reflect/Model/DotnetClass.cs:
##########
@@ -18,72 +18,28 @@
using System;
using System.Reflection;
-using System.Collections.Concurrent;
using Avro;
+using System.Collections.Generic;
-namespace Avro.Reflect
+namespace Avro.Reflect.Model
{
/// <summary>
/// Collection of DotNetProperty objects to repre
/// </summary>
- public class DotnetClass
+ public class DotnetClass : IDotnetClass
{
- private ConcurrentDictionary<string, DotnetProperty> _propertyMap = new ConcurrentDictionary<string, DotnetProperty>();
-
+ private Dictionary<string, IDotnetProperty> _propertyMap;
Review Comment:
## Missed 'readonly' opportunity
Field '_propertyMap' can be 'readonly'.
[Show more details](https://github.com/apache/avro/security/code-scanning/2882)
##########
lang/csharp/src/apache/main/Reflect/ClassCache.cs:
##########
@@ -19,40 +19,48 @@
using System;
using System.Collections;
using System.Collections.Concurrent;
+using System.Reflection;
+using Avro.Reflect.Array;
+using Avro.Reflect.Converter;
+using Avro.Reflect.Model;
+using Avro.Reflect.Service;
namespace Avro.Reflect
{
/// <summary>
/// Class holds a cache of C# classes and their properties. The key for the cache is the schema full name.
/// </summary>
- public class ClassCache
+ [Obsolete()]
+ public class ClassCache : IReflectCache, IConverterService
{
private static ConcurrentBag<IAvroFieldConverter> _defaultConverters = new ConcurrentBag<IAvroFieldConverter>();
- private ConcurrentDictionary<string, DotnetClass> _nameClassMap = new ConcurrentDictionary<string, DotnetClass>();
+ private ConcurrentDictionary<string, IDotnetClass> _nameClassMap = new ConcurrentDictionary<string, IDotnetClass>();
Review Comment:
## Missed 'readonly' opportunity
Field '_nameClassMap' can be 'readonly'.
[Show more details](https://github.com/apache/avro/security/code-scanning/2881)
##########
lang/csharp/src/apache/main/Reflect/Service/ReflectFactory.cs:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Model;
+
+namespace Avro.Reflect.Service
+{
+ /// <summary>
+ /// Factory to create objects for reflest serialixation and deserialization
+ /// </summary>
+ public class ReflectFactory : IReflectFactory
+ {
+ private readonly IReflectCache _refletCache;
+ private readonly IArrayService _arrayService;
+ private readonly IDotnetclassFactory _dotnetclassFactory;
+
+ /// <summary>
+ /// Public constructor
+ /// </summary>
+ public ReflectFactory(
+ IReflectCache refletCache,
+ IArrayService arrayService,
+ IDotnetclassFactory dotnetclassFactory)
+ {
+ _refletCache = refletCache;
+ _arrayService = arrayService;
+ _dotnetclassFactory = dotnetclassFactory;
+ }
+
+ /// <summary>
+ /// Create reflect reader
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <param name="readerSchema"></param>
+ /// <returns></returns>
+ public ReflectReader<T> CreateReader<T>(Schema writerSchema, Schema readerSchema)
+ {
+ LoadClassCache(typeof(T), readerSchema);
+
+ return new ReflectReader<T>(writerSchema, readerSchema, _refletCache, _arrayService);
+ }
+
+ /// <summary>
+ /// Create reflect writer
+ /// </summary>
+ /// <typeparam name="T"></typeparam>
+ /// <param name="writerSchema"></param>
+ /// <returns></returns>
+ public ReflectWriter<T> CreateWriter<T>(Schema writerSchema)
+ {
+ LoadClassCache(typeof(T), writerSchema);
+
+ return new ReflectWriter<T>(writerSchema, _refletCache, _arrayService);
+ }
+
+ internal void LoadClassCache(Type objType, Schema s)
+ {
+ Dictionary<string, Schema> previousFields = new Dictionary<string, Schema>();
+
+ switch (s)
+ {
+ case RecordSchema rs:
+ if (!objType.IsClass)
+ {
+ throw new AvroException($"Cant map scalar type {objType.Name} to record {rs.Fullname}");
+ }
+
+ if (typeof(byte[]).IsAssignableFrom(objType)
+ || typeof(string).IsAssignableFrom(objType)
+ || typeof(IEnumerable).IsAssignableFrom(objType)
+ || typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to record {rs.Fullname}");
+ }
+
+ AddClassNameMapItem(rs, objType);
+ var c = _refletCache.GetClass(rs.Fullname);
+ foreach (var f in rs.Fields)
+ {
+ /*
+ //.StackOverflowException
+ var t = c.GetPropertyType(f);
+ LoadClassCache(t, f.Schema);
+ */
+ if (!previousFields.ContainsKey(f.Name))
+ {
+ previousFields.Add(f.Name, f.Schema);
+ var t = c.GetPropertyType(f);
+ LoadClassCache(t, f.Schema);
+ }
+ }
+
+ break;
+ case ArraySchema ars:
+ if (!typeof(IEnumerable).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to array {ars.Name}");
+ }
+
+ if (!objType.IsGenericType)
+ {
+ throw new AvroException($"{objType.Name} needs to be a generic type");
+ }
+
+ LoadClassCache(objType.GenericTypeArguments[0], ars.ItemSchema);
+ break;
+ case MapSchema ms:
+ if (!typeof(IDictionary).IsAssignableFrom(objType))
+ {
+ throw new AvroException($"Cant map type {objType.Name} to map {ms.Name}");
+ }
+
+ if (!objType.IsGenericType)
+ {
+ throw new AvroException($"Cant map non-generic type {objType.Name} to map {ms.Name}");
+ }
+
+ if (!typeof(string).IsAssignableFrom(objType.GenericTypeArguments[0]))
+ {
+ throw new AvroException($"First type parameter of {objType.Name} must be assignable to string");
+ }
+
+ LoadClassCache(objType.GenericTypeArguments[1], ms.ValueSchema);
+ break;
+ case NamedSchema ns:
+ _refletCache.AddEnum(ns.Fullname, objType);
+ break;
+ case UnionSchema us:
+ if (us.Schemas.Count == 2 && (us.Schemas[0].Tag == Schema.Type.Null || us.Schemas[1].Tag == Schema.Type.Null))
+ {
+ // in this case objType will match the non null type in the union
+ foreach (var o in us.Schemas)
+ {
+ if (o.Tag == Schema.Type.Null)
+ {
+ continue;
+ }
+
+ if (objType.IsClass)
+ {
+ LoadClassCache(objType, o);
+ }
+
+ var innerType = Nullable.GetUnderlyingType(objType);
+ if (innerType != null && innerType.IsEnum)
+ {
+ LoadClassCache(innerType, o);
+ }
+ }
+ }
+ else
+ {
+ // check the schema types are registered
+ foreach (var o in us.Schemas)
+ {
+ if (o.Tag == Schema.Type.Record && _refletCache.GetClass(o.Fullname) == null)
+ {
+ throw new AvroException($"Class for union record type {o.Fullname} is not registered. Create a ClassCache object and call LoadClassCache");
+ }
+ }
Review Comment:
## Missed opportunity to use Where
This foreach loop implicitly filters its target sequence [here](1) - consider filtering the sequence explicitly using '.Where(...)'.
[Show more details](https://github.com/apache/avro/security/code-scanning/2888)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KhrystynaPopadyuk closed pull request #1823: Avro 3603 dotnet reflect refactoring 1
Posted by GitBox <gi...@apache.org>.
KhrystynaPopadyuk closed pull request #1823: Avro 3603 dotnet reflect refactoring 1
URL: https://github.com/apache/avro/pull/1823
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org