Add ToQueryString method to RedisCollection, RedisAggregationSet

[tgmoore]

Exposes ToQueryString from RedisCollection and RedisAggregationSet.

Resolves #486

The following questions came up while putting this together:

1. Is ToQueryString a reasonable name? Is ToCommandString more accurate for Redisearch?
2. These methods duplicate behavior found in AggregationEnumerator.SerializedArgs and RedisCollectionEnumerator.RedisCollectionEnumerator. Is that reasonable, or should there be a common source for that behavior?
3. Should the tests cover less RedisCollection/RedisAggregationSet behavior and just do the least amount necessary to serialize the command to a string?

[slorello89]

Hi @tgmoore, first thanks for opening this! I think we just need to wrap all the arguments and the command names in double quotes - and this should be pretty much good to go. To your questions:

1. ToCommandString is probably the more correct name insofar as defining what it does, however given that EF Core uses ToQueryString, I think it's very probably better to leave it as ToQueryString
2. This is a perfectly reasonable level of duplication
3. You probably don't need to do all the myriad set of different things, but it's not wrong to do it.

[slorello89]

I think we also need to do a select and wrap all the arguments in double quotes " (as well as wrapping the FT.AGGREGATE in quotes)

There's two reasons for this:

1. Syntax / parameterization: the way that Redis protocol works, it requires it's arguments to be parameterized, so all of the serialized arguments above are actually parameterized when they are sent over the wire to Redis (e.g. the protocol tells Redis how many arguments to expect, and then a precise length for each so the query FT.SEARCH people "@name:{Steve} @city:Satellite" LIMIT 0 100 is actually 6 parameters over the wire: FT.SEARCH , people, @name:{Steve} @city:Satellite, LIMIT, 0, 100. Particularly in the case of the query string @name:{Steve} @city:Satellite, it needs to be made very clear to the RESP serializer (e.g. the redis CLI) that that is the entire parameter, which leads to the ergonomics of it.

2. Ergonomics: I think what's likely to be the most common usage of this feature is to dump a string into a log / console for a user to just copy/paste into the redis-cli / Redis insights to see what it pulls back. Surrounding everything with quotes is how the MONITOR works, and this is quite deliberate as it makes it trivial to copy/paste whatever the command was.

[tgmoore]

Will do. Thank you for your clear answers to my questions and additional comprehensive feedback! I really appreciate your time and attention.

[slorello89]

See this is what I was worried about above, this command needs quotes to be properly parameterized (from the CLI's perspective)

127.0.0.1:6379> FT.AGGREGATE person-idx @Salary:[(30.55 inf] LOAD * APPLY @Address_HouseNumber + 4 AS house_num_modified SORTBY 2 @Age DESC LIMIT 0 10 WITHCURSOR COUNT 10000
(error) Unknown argument `inf]` at position 1 for <main>

vs (mind you I've not added any data):

127.0.0.1:6379> FT.AGGREGATE person-idx "@Salary:[(30.55 inf]" LOAD * APPLY "@Address_HouseNumber + 4" AS house_num_modified SORTBY 2 @Age DESC LIMIT 0 10 WITHCURSOR COUNT 10000
1) 1) (integer) 0
2) (integer) 0

IMO the expected output should be:

"FT.AGGREGATE" "person-idx" "@Salary:[(30.55 inf]" "LOAD" "*" "APPLY" "@Address_HouseNumber + 4" "AS" "house_num_modified" "SORTBY" "2" "@Age" "DESC" "LIMIT" "0" "10" "WITHCURSOR" "COUNT" "10000"

[tgmoore]

Makes sense. Thanks for the link to MONITOR. This feedback helped me to understand some of the conversations I've seen on the Discord and in other issues about serialization and escaping special characters.

[slorello89]

same comment regarding quotations surrounding the arguments here.

[tgmoore]

I've made this change as well. Thank you!

[slorello89]

@tgmoore LGTM 👍 - thank you for submitting this!