azure-sdk-for-net icon indicating copy to clipboard operation
azure-sdk-for-net copied to clipboard

[FEATURE REQ] CreateQueryFilter API that doesn't assume column names are hard coded

Open lailabougria opened this issue 3 years ago • 1 comments

Library name

Azure.Data.Tables

Please describe the feature.

We run into scenarios where we can't access the concrete type of record we want to query, which means we're using reflection to build up certain queries.

That means that when filtering, we'd like to do something as follows:

TableClient.CreateQueryFilter($"{propertyInfo.Name} eq {propertyValue}");

This doesn't currently work because the TableOdataFilter will escape all arguments passed into the FormattableString.

Note that we're using the select options in the GetEntityAsync API only to retrieve the properties we know, so we're not materializing the whole record.

That means there's currently an assumption in the SDK when creating filters that the concrete type is available and well-known. Having an API available that allows differentiating between the arguments that represent column names (and therefore don't require escaping) and argument values would solve this problem.

Currently, we're setting up the filter in 2 phases to ensure column names are not escaped.

lailabougria avatar Nov 10 '22 17:11 lailabougria

Thank you for your feedback. Tagging and routing to the team member best able to assist.

jsquire avatar Nov 10 '22 20:11 jsquire

I was discussing this with @lailabougria recently. Unfortunately I wasn't aware there is already a similar issue https://github.com/Azure/azure-sdk-for-net/issues/32347.

The feedback is still valid though because it shows we as users make assumptions based on the method name sbout the use case a method provides

danielmarbach avatar Nov 11 '22 21:11 danielmarbach

I agree that for the dynamic property name scenario, the current behavior and naming is misleading. I think we could add some better documentation on this method or create a new version that is more intuitively named to indicate that all interpolated values are escaped.

@lailabougria or @danielmarbach - do you have any thoughts on how the ideal API might look?

christothes avatar Nov 11 '22 21:11 christothes

I almost managed to type in "fluent API" but then I got shivers and reminded myself how much I don't like them ;)

When I look how the API is intended to be used I almost think regardless whether it is used with predefined column names or not, it assumes anything formattable in there is related to column values. So maybe it should have been called TableClient.EscapeQueryFilterValue. But I'm not sure this is good input for you since you need to continue support what you have...

I also had some other wild thoughts that I'm happy to share, but I'm also unsure if they are helpful.

In theory, since the TableOdataFilter logic already deals with "type conversion" it would be possible to introduce a Column struct that looks something like

public readonly struct FilterColumn
{
   public FilterColumn(string columnName) => ColumnName = columnName;
   public string ColumnName { get; }
}

then extend the text logic to do something like

FilterColumn x => $"{x.ColumnName}",

then someone could use the filter API like

TableClient.CreateQueryFilter($"{new FilterColumn(propertyInfo.Name)} eq {propertyValue}");

danielmarbach avatar Nov 15 '22 21:11 danielmarbach

Now that I think about this pattern you could even extend this to something like

TableClient.CreateQueryFilter($"{new Column(propertyInfo.Name)} eq {new ColumnValue(propertyValue)}");

or (I know almost fluent, :grin: )

TableClient.CreateQueryFilter($"{propertyInfo.Name.Column()} eq {propertyValue.Value()}");

danielmarbach avatar Nov 15 '22 21:11 danielmarbach

It's an interesting idea. Something like the FilterColumn example would do the trick.

My only concern is that it introduces a new type and this special type might be difficult to discover.

Another idea I had was if we utilized a custom format provider and allowed something like this:

TableClient.CreateQueryFilter($"{propertyInfo.Name:PropertyName} eq {propertyValue}");

That seems a bit magical, but would work without augmenting the API surface.

christothes avatar Nov 15 '22 22:11 christothes

@christothes I like the proposal. Just one question. From a consistency perspective, wouldn't it make sense to have a custom formatter as well for the values?

Of course for backward compatibility you would need to keep around the existing support but maybe with documentation updates you could gently push in the direction of

TableClient.CreateQueryFilter($"{propertyInfo.Name:Column} eq {propertyValue:ColumnValue}");

or something similar.

danielmarbach avatar Nov 17 '22 14:11 danielmarbach

@christothes I like the proposal. Just one question. From a consistency perspective, wouldn't it make sense to have a custom formatter as well for the values?

Of course for backward compatibility you would need to keep around the existing support but maybe with documentation updates you could gently push in the direction of

TableClient.CreateQueryFilter($"{propertyInfo.Name:Column} eq {propertyValue:ColumnValue}");

or something similar.

I'm not sure that we'd want to implement an additional format string that essentially does nothing (as it would continue to do exactly what it does today). Perhaps it would be more clear if the new format string was something like :raw or :unescape.

christothes avatar Nov 17 '22 15:11 christothes

And that is precisely the direction I wanted to lead into with the sample above. I think from a user standpoint (leaving out for a moment the backward compatibility question) having a syntax that has a custom formatter that has a meaning and one part that lacks a formatter (essentially the absence of one has a dedicated meaning) makes it difficult to grasp.

I think what you proposed would eliminate the problem because conceptually it would lift things up into a generic marker that indicates there should be no interpretation of a formattable part as a value. And then having such a marker vs non is no longer an issue because it clearly conveys the intent.

danielmarbach avatar Nov 17 '22 17:11 danielmarbach