[
https://issues.apache.org/jira/browse/AVRO-3603?focusedWorklogId=799886&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-799886
]
ASF GitHub Bot logged work on AVRO-3603:
----------------------------------------
Author: ASF GitHub Bot
Created on: 11/Aug/22 05:37
Start Date: 11/Aug/22 05:37
Worklog Time Spent: 10m
Work Description: github-code-scanning[bot] commented on code in PR #1819:
URL: https://github.com/apache/avro/pull/1819#discussion_r943108992
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,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 as
RecordSchema).GetClassType() == obj.GetType();
case Schema.Type.Enumeration:
- return EnumCache.GetEnumeration(sc as EnumSchema) ==
obj.GetType();
+ return _reflectCache.GetEnumeration(sc as EnumSchema) ==
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/2845)
##########
lang/csharp/src/apache/main/Reflect/Reflection/ReflectCache.cs:
##########
@@ -288,17 +256,53 @@
~ 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");
~ }
~ }
~ }
~
break;
}
}
+
+ /// <summary>
+ /// Lookup an entry in the cache - based on the schema fullname
+ /// </summary>
+ /// <param name="schema"></param>
+ /// <returns></returns>
+ public Type GetEnumeration(NamedSchema schema)
+ {
+ Type t;
+ if (!_nameEnumMap.TryGetValue(schema.Fullname, out t))
+ {
+ throw new AvroException($"Couldn't find enumeration for avro
fullname: {schema.Fullname}");
+ }
+
+ return t;
+ }
+
+ private void AddEnumNameMapItem(NamedSchema schema, Type dotnetEnum)
+ {
+ _nameEnumMap.TryAdd(schema.Fullname, dotnetEnum);
+ }
+
+ private void AddClassNameMapItem(RecordSchema schema, Type dotnetClass)
+ {
+ if (schema != null && GetClass(schema) != null)
+ {
+ return;
+ }
+
+ if (!dotnetClass.IsClass)
+ {
+ throw new AvroException($"Type {dotnetClass.Name} is not a
class");
+ }
+
+ _nameClassMap.TryAdd(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/2847)
##########
lang/csharp/src/apache/main/Reflect/Reflection/ReflectCache.cs:
##########
@@ -19,63 +19,31 @@
using System;
using System.Collections;
using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Text;
+using Avro.Reflect.Converter;
+using Avro.Reflect.Interface;
-namespace Avro.Reflect
+namespace Avro.Reflect.Reflection
{
- /// <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
+ internal class ReflectCache : IReflectCache
{
- private static ConcurrentBag<IAvroFieldConverter> _defaultConverters =
new ConcurrentBag<IAvroFieldConverter>();
-
+ private ConcurrentDictionary<string, Type> _nameEnumMap = new
ConcurrentDictionary<string, Type>();
Review Comment:
## Missed 'readonly' opportunity
Field '_nameEnumMap' can be 'readonly'.
[Show more
details](https://github.com/apache/avro/security/code-scanning/2850)
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -188,9 +181,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 as
RecordSchema).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/2844)
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultWriter.cs:
##########
@@ -28,28 +30,19 @@
/// </summary>
public class ReflectDefaultWriter : SpecificDefaultWriter
{
- private ClassCache _classCache = new ClassCache();
-
- /// <summary>
- /// Class cache
- /// </summary>
- public ClassCache ClassCache { get => _classCache; }
+ private IReflectCache _reflectCache;
Review Comment:
## Missed 'readonly' opportunity
Field '_reflectCache' can be 'readonly'.
[Show more
details](https://github.com/apache/avro/security/code-scanning/2849)
##########
lang/csharp/src/apache/main/Reflect/ReflectDefaultReader.cs:
##########
@@ -37,12 +39,7 @@
/// </summary>
public Type MapType { get => _mapType; set => _mapType = value; }
- private ClassCache _classCache = new ClassCache();
-
- /// <summary>
- /// Class cache
- /// </summary>
- public ClassCache ClassCache { get => _classCache; }
+ private IReflectCache _reflectCache;
Review Comment:
## Missed 'readonly' opportunity
Field '_reflectCache' can be 'readonly'.
[Show more
details](https://github.com/apache/avro/security/code-scanning/2848)
Issue Time Tracking
-------------------
Worklog Id: (was: 799886)
Time Spent: 20m (was: 10m)
> .NET/#C: Refactor ReflectReader/Writer: rename and refactor caches, add DI
> --------------------------------------------------------------------------
>
> Key: AVRO-3603
> URL: https://issues.apache.org/jira/browse/AVRO-3603
> Project: Apache Avro
> Issue Type: Improvement
> Reporter: Khrystyna Popadyuk
> Assignee: Khrystyna Popadyuk
> Priority: Major
> Labels: pull-request-available
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Current ReflectReader/Writer use a lot of static entities (classes, methods,
> fields). It is good to refactor them with interfaces and DI approach.
> If do such update at once it requires massive changes and can we difficult to
> review and test.
> This story is created as first step for such refactoring. It will include:
> - group classes by folders
> - add DI
> - rename and refactor ClassCash and EnumCache (avoid method refactoring in
> scope of this story)
> This story do not update method implementation.
> This is breaking changes.
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
