NoahStolk commented 8 months ago

Description

Our team is currently running into a problem where the current version of NJsonSchema referenced from the Confluent.SchemaRegistry.Serdes.Json library (10.6.3) is not binary compatible with the resolved version from our binaries (11.0.0). We require NJsonSchema version 11.0.0 due to having a dependency on NSwag.AspNetCore version 14.0.0, and we require NSwag.AspNetCore to be on version 14.0.0 because that is the only version compatible with .NET 8.

NSwag.AspNetCore 14.0.0 is requesting NJsonSchema 11.0.0 from NuGet, which is the highest version and therefore ends up being resolved. However, since Confluent.SchemaRegistry.Serdes.Json is requesting NJsonSchema 10.6.3, we are getting runtime errors such as:

System.MissingMethodException: Method not found: 'Newtonsoft.Json.JsonSerializerSettings NJsonSchema.Generation.JsonSchemaGeneratorSettings.get_ActualSerializerSettings()'.
   at Confluent.SchemaRegistry.Serdes.JsonSerializer`1.SerializeAsync(T value, SerializationContext context)

It seems that in NJsonSchema 11.0.0 the JsonSchemaGeneratorSettings class in particular has had some major (both source and binary) breaking changes which currently affect the code in the Confluent.SchemaRegistry.Serdes.Json library:

The ActualSerializerSettings and SerializerSettings properties no longer exist.
The class is now abstract and cannot be instantiated.

.NET 7 will reach end of support on May 14th this year, and besides that we'd very much like to migrate to .NET 8 for various reasons.

What is the recommended approach here? Would it be possible for Confluent.SchemaRegistry.Serdes.Json to migrate to NJsonSchema version 11.0.0? I'd be more than happy to open a PR regarding this issue, but thought it'd be a good idea to discuss here first.

How to reproduce

Install both Confluent.SchemaRegistry.Serdes.Json 2.3.0 and NJsonSchema 11.0.0.
Produce a JSON message to Kafka.

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Confluent.SchemaRegistry.Serdes.Json" Version="2.3.0" />
        <PackageReference Include="NJsonSchema" Version="11.0.0" />
    </ItemGroup>

</Project>

using Confluent.Kafka;
using Confluent.SchemaRegistry;
using Confluent.SchemaRegistry.Serdes;

SchemaRegistryConfig schemaRegistryConfig = new() { Url = "test" };
ISchemaRegistryClient schemaRegistryClient = new CachedSchemaRegistryClient(schemaRegistryConfig);

ProducerConfig producerConfig = new() { BootstrapServers = "localhost:9092" };
IProducer<Null, TestJsonMessage> producer = new ProducerBuilder<Null, TestJsonMessage>(producerConfig)
    .SetValueSerializer(new JsonSerializer<TestJsonMessage>(schemaRegistryClient))
    .Build();

await producer.ProduceAsync("test", new() { Value = new("test") });

public record TestJsonMessage(string Id);

The program crashes with the following exception:

Unhandled exception. Confluent.Kafka.ProduceException`2[Confluent.Kafka.Null,TestJsonMessage]: Local: Value serialization error                                                   
 ---> System.MissingMethodException: Method not found: 'Newtonsoft.Json.JsonSerializerSettings NJsonSchema.Generation.JsonSchemaGeneratorSettings.get_ActualSerializerSettings()'.
   at Confluent.SchemaRegistry.Serdes.JsonSerializer`1.SerializeAsync(T value, SerializationContext context)                                                                      
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)                                                                    
   at Confluent.SchemaRegistry.Serdes.JsonSerializer`1.SerializeAsync(T value, SerializationContext context)                                                                      
   at Confluent.Kafka.Producer`2.ProduceAsync(TopicPartition topicPartition, Message`2 message, CancellationToken cancellationToken)                                              
   --- End of inner exception stack trace ---

Checklist

Please provide the following information:

[x] A complete (i.e. we can run it), minimal program demonstrating the problem. No need to supply a project file.
[x] Confluent.Kafka nuget version.
[ ] Apache Kafka version.
[ ] Client configuration.
[ ] Operating system.
[ ] Provide logs (with "debug" : "..." as necessary in configuration).
[ ] Provide broker log excerpts.
[ ] Critical issue.

KB-RBullock commented 8 months ago

We are experiencing this issue when attempting to update to .NET 8 also

SukharevAndrey commented 8 months ago

@NoahStolk Confluent will do nothing. They do not actively support most of their open-source libraries. Just look at elasticsearch and jdbc connectors for instance: they completely ignore issues and PRs. I suggest you making a copy of NJsonSchema related library code and fix it yourself.

NoahStolk commented 8 months ago

@NoahStolk Confluent will do nothing. They do not actively support most of their open-source libraries. Just look at elasticsearch and jdbc connectors for instance: they completely ignore issues and PRs. I suggest you making a copy of NJsonSchema related library code and fix it yourself.

That's a shame, and I have already done that and upgraded to .NET 8 successfully with minimal changes. I would however prefer to keep this issue open for anyone else who might run into the same problem.

The Confluent.SchemaRegistry.Serdes.Json library is not necessarily complex so it's fairly easy to make a copy and make the necessary changes (especially in our case since we were only using about 20% of the library's code). It's not ideal though.

anchitj commented 8 months ago

Thanks for reporting. I'll check before the new version release if we can upgrade the NJsonSchema. Please open up a PR if you can, I'll take a look.

NoahStolk commented 8 months ago

Thanks @anchitj, I'll set up a branch with my current work, and see if I can port the remaining parts of the library to NJsonSchema 11 later this week. 👍

NoahStolk commented 7 months ago

I have completed my PR. Build and unit tests are succeeding. Please let me know if there's anything else I can do to get this merged. It would be ideal if the change could be included in a 3.0.0 release.

NoahStolk commented 7 months ago

Hi @anchitj, are you able to give us an indication of when we can expect a new release? I imagine the PR would not get merged until a new release is scheduled. Is that correct?

KB-RBullock commented 6 months ago

Anything that could be done to expedite this fix/release would be much appreciated by the community, it is a show stopper for .NET8 migration

Thanks in advance

NoahStolk commented 4 months ago

FWIW, .NET 7 has gone out of maintenance earlier this week.

Am I correct in assuming that the library authors cannot release breaking changes due to the version number being in sync with librdkafka? What is the versioning policy here?

Would this mean that the fix can only get released when librdkafka 3.0.0 comes out, if ever?

mkoziel-pfl commented 4 months ago

Any update on when this is getting rolled out? We are moving apps from .NET 6 (EOL in Nov) to .NET 8...this fix is critical for us.

NoahStolk commented 3 months ago

It's been over 4 months since I've opened my PR and I haven't received any replies from Confluent. PR https://github.com/confluentinc/confluent-kafka-dotnet/pull/2226 has recently been merged and now my branch has some merge conflicts. I wouldn't mind fixing those, but since I'm not getting any response I don't think it's worth my time?

I understand this is a difficult problem where a major release is required if we are to follow semantic versioning, but not getting any response at all is a little frustrating, especially given the "help appreciated!" label assigned to this issue. Even just saying "we can't fix this because we cannot release breaking changes at this time" would be fine with me. There are other ways around this problem, but the lack of communication does not help.

rayokota commented 2 months ago

@NoahStolk , sorry for the lack of responsiveness. Yes, we want to take this change and are trying to decide how best to do so, even in a minor release.

One suggestion that came up is to add preprocessor directives of the form

#if NET8_0_OR_GREATER

so that this would only be a breaking change for those using .Net 8.0. Does this work for you?

In any case, if you can address the merge conflicts, we can start work toward merging your PR. Sorry again for not getting back to you earlier.

NoahStolk commented 2 months ago

@NoahStolk , sorry for the lack of responsiveness. Yes, we want to take this change and are trying to decide how best to do so, even in a minor release.

One suggestion that came up is to add preprocessor directives of the form
#if NET8_0_OR_GREATER
so that this would only be a breaking change for those using .Net 8.0. Does this work for you?

In any case, if you can address the merge conflicts, we can start work toward merging your PR. Sorry again for not getting back to you earlier.

Hi @rayokota, thank you for your reply. Yes, that would work in our case, but it would still be a breaking change for those targeting .NET 8 and using NJsonSchema v10. For example, an app that does not use NSwag might target .NET 8 and use NJsonSchema v10 for different purposes.

Your suggestion gave me a different idea -- would it be acceptable to introduce our own constant for this? Instead of relying on the target framework, we could have a constant named CONFLUENT_NJSONSCHEMA_VER11 (or something similar), and let that decide whether the library compiles with NJsonSchema 10 or 11. This would require users to explicitly opt into the new dependency version by defining the constant in their project, solving the problem without introducing any breaking changes for any target framework.

I will be resolving the merge conflicts shortly.

NoahStolk commented 2 months ago

The original PR broke because the fork source was changed to https://github.com/ah-/confluent-kafka-dotnet/, so I've closed that. Couldn't find a reliable way update the fork source on GitHub.

I've opened a new PR: https://github.com/confluentinc/confluent-kafka-dotnet/pull/2263

rayokota commented 2 months ago

@NoahStolk , after talking with a colleague, I think we will want to use #if NET8_0_OR_GREATER instead of CONFLUENT_NJSONSCHEMA_VER11

NathanT02 commented 1 month ago

Blocking issue for me. Is there a temporary workaround?

NoahStolk commented 1 month ago

Blocking issue for me. Is there a temporary workaround?

@NathanT02 The easiest way would be to just copy the Confluent.SchemaRegistry.Serdes.Json project, and then make the same changes as I've done in my PR: https://github.com/confluentinc/confluent-kafka-dotnet/pull/2263/files

You only need to modify the project file so it uses NJsonSchema 11 and change a few lines of code in 3 source files. Alternatively, I think you can just clone my fork and copy the source files directly from my branch.

Then replace all of your references to the Confluent.SchemaRegistry.Serdes.Json NuGet package with the new local copy.

confluentinc / confluent-kafka-dotnet

Binary incompatibility with NJsonSchema version 11.0.0 #2169

Description

How to reproduce

Checklist