tomasfabian / ksqlDB.RestApi.Client-DotNet

ksqlDb.RestApi.Client is a C# LINQ-enabled client API for issuing and consuming ksqlDB push and pull queries and executing statements.
MIT License
97 stars 26 forks source link

JsonPropertyNames are ignored when using syntax to select individual columns. #61

Closed mrt181 closed 8 months ago

mrt181 commented 8 months ago

Describe the bug JsonPropertyNames are ignored when using syntax to selec individual columns.

To Reproduce

  public class JsonPropertyNameTestData
  {
    [JsonPropertyName("sub")] public SubProperty SubProperty { get; set; }
  }

  [Struct]
  public class SubProperty
  {
    [JsonPropertyName("prop")] public string Property { get; set; }
  }

  [Test]
  public void SelectColumnsUsingPullQueryThatHaveJsonPropertyName()
  {
    //Arrange
    //Act
    var ksql = DbProvider.CreatePullQuery<JsonPropertyNameTestData>()
      .Select(c => new { c.SubProperty })
      .ToQueryString();

    //Assert
    ksql.Should().BeEquivalentTo(
      @$"SELECT sub FROM {nameof(JsonPropertyNameTestData)};".ReplaceLineEndings());
  }

  [Test]
  public void SelectColumnsUsingPullQueryThatHaveJsonPropertyNameWithAlias()
  {
    //Arrange
    //Act
    var ksql = DbProvider.CreatePullQuery<JsonPropertyNameTestData>()
      .Select(c => new { Prop = c.SubProperty })
      .ToQueryString();

    //Assert
    ksql.Should().BeEquivalentTo(
      @$"SELECT sub As Prop FROM {nameof(JsonPropertyNameTestData)};".ReplaceLineEndings());
  }

Expected behavior Both Tests should work but only the second one with the alias does. The first one uses SubProperty as column name.

Environment (please complete the following information):

Additional context This is needed when selecting PseudoColumns like ROWTIME but will fail because a stream / table creates the column names using the JsonPropertyNames

mrt181 commented 8 months ago

These are the places that seem related to this but I have currently no idea how to fix this. The moment I touch it many tests that use column selection syntax break. Screenshot 2024-03-05 113115 Screenshot 2024-03-05 113041

tomasfabian commented 8 months ago

Hello @mrt181, this could be potentially fixed by adding the below else if branch before this line in KSqlVisitor.ProcessVisitNewMember:

    else if (expression is MemberExpression me2 && me2.Member.GetCustomAttribute<JsonPropertyNameAttribute>() != null)
      Append(me2.Member.Format(QueryMetadata.IdentifierEscaping));

I've just quickly prototyped it for now. Please share your thoughts and feedback on it.

tomasfabian commented 8 months ago

Alternative solution:

    else if (expression is MemberExpression me2 && !me2.Member.DeclaringType.IsKsqlGrouping())
      Append(me2.Member.Format(QueryMetadata.IdentifierEscaping));
tomasfabian commented 8 months ago

Do we want to implement the suggested fix mentioned above? If so, I can prepare a pull request (PR), or would you prefer to handle it yourself?

mrt181 commented 8 months ago

I did an additional change to cover pseudocolumns and added some tests. If ok than I will create a PR.

tomasfabian commented 8 months ago

Thank you @mrt181 for the PR! I released a patch:

dotnet add package ksqlDb.RestApi.Client --version 3.6.2