Allow for strongly-typed query properties

[fredvollmer]

Hopefully there wasn't a simpler way of resolving this.

Note: this PR includes a breaking change.

Currently, it isn't possible to render a non-string property into the query sent to kSSQL--the JSON serializer will wrap all string values in quotes (as expected). Thus, when setting a Boolean property such as ksql.query.pull.table.scan.enabled, the query string includes the parameter with a stringified value (ksql.query.pull.table.scan.enabled = "true"). This causes kSQL to throw an error, since it is expecting a Boolean value for this parameter.

To work around this, we altered the Properties dictionary to hold System.Text.Json.JsonValue's instead of strings, and added Get<T> and Set<T> methods to allow for setting properties of any type. When the properties are serialized, the Serializer will respect the underlying type: strings will be quoted, Booleans will be unquoted, etc.

As part of this change, we also marked the ProcessingGuarantee and AutoOffsetReset enums to be serialized using the built-in snake case JSON enum serializer. This meant the helper methods for manually marshaling between the enum and their corresponding string representation were no longer needed.

Before:

{
  "sql": "SELECT * FROM MY_TABLE;",
  "properties": {
    "ksql.query.pull.table.scan.enabled": "true"
  }
}

After:

{
  "sql": "SELECT * FROM MY_TABLE;",
  "properties": {
    "ksql.query.pull.table.scan.enabled": true
  }
}

AI-generated summary of changes

This pull request includes several changes to improve the handling of QueryStreamParameters and QueryParameters in the test files. The most important changes include replacing dictionary-style access with the Set and Get methods for better type safety and readability, and adding a new test for the JsonSnakeCaseStringEnumConverterAttribute.

Improvements to Parameter Handling:

• Tests/ksqlDB.RestApi.Client.IntegrationTests/KSql/Linq/ComplexTypesTests.cs: Replaced dictionary-style access with the Set method for QueryStreamParameters to set AutoOffsetResetPropertyName to AutoOffsetReset.Earliest. [1] [2]
• Tests/ksqlDB.RestApi.Client.IntegrationTests/KSql/Linq/QbservableExtensionsTests.cs: Updated multiple test methods to use the Set method for QueryStreamParameters to set AutoOffsetResetPropertyName to AutoOffsetReset.Earliest. [1] [2] [3] [4] [5]

Enhancements to Test Assertions:

• Tests/ksqlDB.RestApi.Client.Tests/DependencyInjection/KSqlDbServiceCollectionExtensionsTests.cs: Changed the assertion to use the Get method for QueryStreamParameters to check ProcessingGuarantee.
• Tests/ksqlDB.RestApi.Client.Tests/KSql/Query/Context/KSqlDBContextOptionsTests.cs: Modified multiple test methods to use the Get method for QueryStreamParameters to verify ProcessingGuarantee and AutoOffsetReset. [1] [2] [3] [4] [5]

New Test Addition:

• Tests/ksqlDB.RestApi.Client.Tests/Infrastructure/Attributes/JsonCamelCaseStringEnumConverterAttributeTests.cs: Added a new test to verify the creation of the JsonSnakeCaseStringEnumConverterAttribute.

Context Options Handling:

• Tests/ksqlDB.RestApi.Client.Tests/KSql/Query/Context/KSqlDBContextTests.cs: Updated multiple test methods to use the Set and Get methods for QueryStreamParameters to set and verify AutoOffsetReset and ProcessingGuarantee. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Options Builder Enhancements:

• Tests/ksqlDB.RestApi.Client.Tests/KSql/Query/Context/Options/KSqlDbContextOptionsBuilderTests.cs: Modified test methods to use the Get method for QueryStreamParameters to verify ProcessingGuarantee and AutoOffsetReset. [1] [2] [3] [4] [5]

[tomasfabian]

Hi @fredvollmer,
thank you for your PR so far. Could you please evaluate my suggestions on the breaking changes before I proceed with a full review?

[tomasfabian]

Would it help avoid a breaking change to keep the dictionary but change the value type to an object?

public interface IQueryOptions
{
  Dictionary<string, object> Properties { get; }
}

Have you encountered any obstacles with this approach?

[fredvollmer]

I believe it would still be breaking, as the dictionary is a public field. I've encountered a lot of code which directly references the dictionary and expects the value to be a string. Because of this, I don't believe an object would be any better than JsonValue as far as breaking changes are concerned. Am I missing something?

[tomasfabian]

For string values, the API remains the same:

["auto.offset.reset"] = "latest"

opt["auto.offset.reset"] = "latest"

Existing code would still compile as before:

opt[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] = "true";

For booleans and similar types, only the assigned value changes if someone consciously decides to fix it:

opt[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] = true;

I couldn't reproduce the runtime error, so I suggested adding an integration test. It's probably me who is missing something. Thank you again!

[tomasfabian]

I'm trying to understand how this issue could have gone unnoticed for so long. You mentioned that you

encountered a lot of code which directly references the dictionary

Was this code also assigning boolean values as strings?
I'm trying to reproduce the issue using KSQLDB_VERSION=0.29.0.

[fredvollmer]

I had the same thought as far as how it could have gone unnoticed. I did add an integration test. Using the latest stable release of this library, we observed the following error on kSQL 0.28.2 when setting ksql.query.pull.table.scan.enabled to true:

java.lang.ClassCastException: class java.lang.String cannot be cast to class java.lang.Boolean (java.lang.String and java.lang.Boolean are in module java.base of loader 'bootstrap')
	at io.confluent.ksql.rest.server.resources.streaming.PullQueryConfigPlannerOptions.getTableScansEnabled(PullQueryConfigPlannerOptions.java:41)

[fredvollmer]

For string values, the API remains the same:

["auto.offset.reset"] = "latest"

opt["auto.offset.reset"] = "latest"

Existing code would still compile as before:

opt[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] = "true";

For booleans and similar types, only the assigned value changes if someone consciously decides to fix it:

opt[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] = true;

I couldn't reproduce the runtime error, so I suggested adding an integration test. It's probably me who is missing something. Thank you again!

Ahh, you're right! I think this is the way to go.

One small thing--this will maintain the same API for writes, but what about reads? This is obviously a much smaller use case, but is it not still a breaking change if opt[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] returns an object rather than a string? I still think object is preferrable over JsonValue if possible, just not sure if it gets us entirely out of the breaking change issue.

[tomasfabian]

I downgraded the ksqldb server to 0.28.2 and reran the integration test you provided, using a stringified boolean, and it passed successfully. I didn’t encounter the ClassCastException, even in the ksqldb server logs, which I would have expected based on the information you shared.

Nevertheless, it’s great that you’re addressing the issue, @fredvollmer. If you don’t mind, I’d also like to get a third opinion on the API changes. @mrt181, do you have any thoughts on this PR?

[tomasfabian]

Oops, sorry—I didn’t notice the new message from you before I published my previous answer.

That’s a great point: receiving values from the dictionary will still result in a breaking change. Please go ahead and increase the package’s major version with a note in the changelog.md, or I can handle it later if needed.
Users will need to downcast to a concrete type, which I think is an acceptable trade-off, as the value will more often be set than read.
I hope the documentation remains intact as well. Or even better, examples like the following could be changed to:

options[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] = true;

[tomasfabian]

Hello @fredvollmer, I have a pending release with a bumped major version. How do we want to proceed with your PR? Do you have time to update the public API as per our discussion, or should I take over the task?

[tomasfabian]

To prevent a breaking change, would it be better to keep the indexer but change its return type to an object?

object this[string key] { get; set; }

[tomasfabian]

I tested changing the string value to an object type, and both test cases below passed. I'd like to proceed with these less disruptive changes unless you identify any critical downsides.

[Test]
public void SetupPullQuery_BooleanOptionWasConfigured()
{
  //Arrange
  var setupParameters = ClassUnderTest.UseKSqlDb(TestParameters.KsqlDbUrl);

  //Act
  var parameters = setupParameters.SetupPullQuery(opt =>
  {
    opt[KSqlDbConfigs.KsqlQueryPullTableScanEnabled] = true;
  }).Options.PullQueryParameters;


  var json = JsonSerializer.Serialize(parameters);

  //Assert
  json.Should().BeEquivalentTo("{\"Sql\":null,\"Properties\":{\"ksql.query.pull.table.scan.enabled\":true}}");
}

[Test]
public void SetupPullQuery_ProcessingGuaranteeEnumOptionWasConfigured()
{
  //Arrange
  var setupParameters = ClassUnderTest.UseKSqlDb(TestParameters.KsqlDbUrl);

  //Act
  var parameters = setupParameters.SetupPushQuery(opt =>
  {
    opt[KSqlDbConfigs.ProcessingGuarantee] = ProcessingGuarantee.AtLeastOnce;
  }).Options.QueryStreamParameters;


  var json = JsonSerializer.Serialize(parameters);

  //Assert
  json.Should().BeEquivalentTo("{\"Sql\":null,\"Properties\":{\"auto.offset.reset\":\"earliest\",\"processing.guarantee\":\"at_least_once\"}}");
}

The JsonSnakeCaseStringEnumConverter has been applied in this case as well.

[mrt181]

I would prefer less churn on the public API if possible and would prefer the change to object