AVRO-3603: .NET Reflect Reader and Writer - add interfaces

[KhrystynaPopadyuk]

AVRO-3603

This pull request add interfaces for reflect reader and writer that will help end users override default behavior due to their needs.

This change is refactoring only. Mostly covered by existing tests but require small updates to them.
WriteAndReadObjectsWithLogicalSchemaFields_WithNullValues
WriteAndReadObjectsWithLogicalSchemaFields_WithoutNullValues.

• Does this pull request introduce a new feature? NO
• If yes, how is the feature documented? Not Applicable

[KhrystynaPopadyuk]

This is existing code. Should I change logic if I want rename variable?

[KhrystynaPopadyuk]

Hi @martin-g , @KyleSchoonover , @RyanSkraba ,
I tried introduce interfaces into for reflect reader and writer. They will make code more flexible.
I tried make this changes not breaking. Also they relate only to reflect specific functionality (class and property parsing).

Please review it.

[martin-g]

I tried make this changes not breaking

Changing the method's return type (from a class to an interface) is binary incompatible change.

[KhrystynaPopadyuk]

Hi @martin-g ,

Thank you for review.
I have pushed the changes. It is ready for next review.

[martin-g]

LGTM!
It would be good if an experienced .NET developer reviews this PR too!

[KhrystynaPopadyuk]

Hi @KalleOlaviNiemitalo, please review this PR. It will help inject custom logic and fixes.

[KalleOlaviNiemitalo]

It's morning here, I won't be able to review before evening.

[KhrystynaPopadyuk]

Hi @KalleOlaviNiemitalo, That's OK. Thanks

[KalleOlaviNiemitalo]

(Setting up a new computer took more time than expected.)

This pull request makes existing classes implement new interfaces, and then uses those interfaces instead of the classes:

• public class ClassCache : public interface ICacheService, public interface IArrayService
• public class DotnetClass : public interface IDotnetClass
• internal class DotnetProperty : public interface IDotnetProperty

I worry that these changes will make it more difficult to avoid breaking changes in the future if members are added. Currently, it is possible to add new virtual methods to ClassCache and DotnetClass and provide default implementations. Doing the same in the interfaces would require either using default interface methods, which .NET Framework does not support, or adding more interfaces (e.g. ICacheService2) and checking at run time whether the object implements those.

Does IDotnetProperty need to be public? It is not implemented by any public type, nor used as a parameter or return value of any public method.

[KhrystynaPopadyuk]

Hi @KalleOlaviNiemitalo ,

Thank you for review.

The main goal of this PR is add interfaces to be able inject custom implementation (including breaking changes) and fix bugs quickly.
For example:

• ClassCache at this moment is used to: cache objects, parse types, instantiate objects... Serializer and deserializer do not use most of behavior. Having interface I would be able write own implementation. I would be able use IMemoryChace for caching and have separate service to parse types where I would be able fix bugs much quicker. Also I would be able to update converters part to use DI instead of add them to static list. A lot of other updates would be able to introduce without breaking/any changes to existing Apache.Avro package. Adding this interface open way to add new nuget package for .NET Core that use current Apache.Avro for main functionality but also add usage of DI, ILogger, IMemorryCache and other features that Microsoft offer for developers.
• DotnetClass has only one constructor that require ClassCache as argument. But does it really need ClassCache or just converters? Also it have hard-coded login about how parse and create properties. Having/using interface I would be able have own implementation with custom login. As example use DI to get registered converters, skip properties by criteria....
• DotnetProperty probably the most obvious. There is AVRO-3434: support logical schemas in reflect reader and writer #1718 that was merged 9 of August 2022 it was almost 4 months ago. And till now it is not released and there is no scheduled time for release. It is critical changes for my company. Having interface I would be able write custom implementation and deploy fix to prod in August.

I understand you concern to avoid breaking changes. But what suppose to do users, as me, who need breaking changes?
These interfaces will help users who need breaking changes or urgent fix to have them without breaking changes to main package.

[KhrystynaPopadyuk]

Hi @KalleOlaviNiemitalo and @martin-g ,

There is my next attempt to add opportunity to override default behaviors - #2009
There is no interfaces and it is not breaking changes.
I would appreciate if you review it.

[vivere-dally]

Can you please merge and release this PR?

[KhrystynaPopadyuk]

Hi @vivere-dally ,
This PR and #2009 are different approaches to resolve the same task. Only one should be merged.

I prefer this one.