LGouellec / kafka-streams-dotnet

.NET Stream Processing Library for Apache Kafka 🚀
https://lgouellec.github.io/kafka-streams-dotnet/
MIT License
452 stars 73 forks source link

Record.Create factory function does not initialise Headers - Results in Null reference exception if trying to Access Headers #316

Closed dave-signify closed 1 week ago

dave-signify commented 3 months ago

Description

Hello!

First off, thanks for making this super lib @LGouellec and I hope you had a nice weekend! I love to see succesful .NET ports flourishing, and I'm enjoying using it 💯 :-).

Please forgive me if I'm going about this the wrong way. However, I believe I may have found an issue with the Create factory function in the Record class. I discovered this when trying to write a unit test for the function I am passing into the .Transform function as part of the stream topology creation. I understand this may be a purposeful design choice to not initialise the headers when calling Create via the Record public API, so if there's another way of going about mocking or stubbing out a Record class including setting the Headers from within a unit test process, that would be ideal.

How to reproduce

Simply call the Create factory function on the Record class as shown below:

var aRecord = Record<string,string>.Create("some-key", "some-value");

Now try to call the Headers.Add function:

aRecord.Headers.Add(new Header("some-header-key", Encoding.ASCII.GetBytes("some-header-value")));

The above call to aRecord.Headers.Add will throw an unhandled NullReferenceException. This is preventing me from writing my unit test where I would like to create my own Record with some headers in my test.

I believe the cause of the issue is here https://github.com/LGouellec/kafka-streams-dotnet/blob/981e9d4b8447712ceb03adab6760202f21150f80/core/Processors/Public/Record.cs#L47 where the public factory function calls this private constructor, but the constructor doesn't initialise the Headers property.

I suppose the question is, is this a bug? Or is this a design choice? If it's a design choice is there another way I should be creating instances of Records in my unit tests?

If this is a bug @LGouellec I'd be happy to submit a Pull request for your review! :-)

Thank you! :)

Streamiz nuget version: 1.5.1 Mac OS Sanomoa 14.3.1 Kafka v: latest docker image (local testing) Not a critical issue - testability issue more so

LGouellec commented 2 months ago

Hey @dave-signify ,

Sorry for the late reply, I confirm it's a bug/lack. There is a workaround :

        private class MyTransformer : ITransformer<string, string, string, string>
        {
            private ProcessorContext<string,string> context;

            public MyTransformer()
            {
            }

            public void Init(ProcessorContext<string, string> context)
            {
                this.context = context;
            }

            public Record<string, string> Process(Record<string, string> record)
            {
                var newRecord = Record<string, string>.Create(record.Key, record.Value.ToUpper());
                var newHeaders = new Headers();
                newHeaders.Add("newHeader", Encoding.UTF8.GetBytes("value"));
                context.SetHeaders(newHeaders);
                return newRecord;
            }

            public void Close()
            {

            }
        }

But feel to submit a PR to support this feature at the Record class level