Gremlin.Net.CosmosDb icon indicating copy to clipboard operation
Gremlin.Net.CosmosDb copied to clipboard

Incorrect query when using P.Within()

Open GerbenGaastra opened this issue 6 years ago • 6 comments

When querying using the method P.Within() there never seem to yield any results. When inspecting the produced query I noticed it has some additional quotes around the array. When removing these quotes I get the expected behavior which are the requested vertexes.

Example code:

var idArray = new int[] {5, 13, 27};
var query = g.V<Vertex>().Has(p => p.IdProperty, P.Within(idArray));


var queryWithTooManyQuotes = query.ToGremlinQuery();
// -> "g.V().hasLabel(\"label"\).has(\"IdProperty\",within(\"[5,13,27]\"))"

var fixedQuery = query
      .ToGremlinQuery()
      .Replace("\"[", "[", StringComparison.InvariantCulture)
      .Replace("]\"", "]", StringComparison.InvariantCulture);
// -> "g.V().hasLabel(\"label"\).has(\"IdProperty\",within([5,13,27]))"

var incorrectlyEmptyResult = await gremlinClient.QueryAsync<Vertex>(queryWithTooManyQuotes);
var correctResult = await gremlinClient.QueryAsync<Vertex>(fixedQuery);

GerbenGaastra avatar Nov 12 '18 15:11 GerbenGaastra

Might be here: https://github.com/evo-terren/Gremlin.Net.CosmosDb/blob/master/src/Gremlin.Net.CosmosDb/Serialization/GremlinQuerySerializer.cs#L184-L202

BenjaBobs avatar Nov 12 '18 15:11 BenjaBobs

Yep. That's definitely a bug in the serialization logic. I'm guessing it's being treated as an object and is therefore getting the quotes. The code linked by @BenjaBobs would be a good place to start on a fix. I won't be able to get to this any time soon due to the season, unfortunately, but would be happy to look at any pull requests you may have for a fix. Otherwise, I will address this when I can. Thank you for your patience.

evo-terren avatar Nov 13 '18 21:11 evo-terren

I investigated a bit, and this turned out to be more complex than I initially thought.

P.Within(params object[]) behaves in the following way:

[Fact]
public void Within_serializes_properly()
{
    var g = new GraphTraversalSource();

    var priceObjectArray = new object[] { 5, 13, 27 };
    var priceIntArray = new int[] { 5, 13, 27 };

    var queryObject = g.V<Product>().Has(p => p.Price, P.Within(priceObjectArray));
    var queryInt = g.V<Product>().Has(p => p.Price, P.Within(priceIntArray));

    var queryObjectString = queryObject.ToGremlinQuery();
    var queryIntString = queryInt.ToGremlinQuery();

    queryObjectString.Should().Be("g.V().hasLabel(\"product\").has(\"Price\",within(5, 13, 27))");
    queryIntString.Should().Be("g.V().hasLabel(\"product\").has(\"Price\",within(\"[5, 13, 27]\"))");
}

Because of the way the type is inferred and the way params object[] works I'm not exactly sure how to fix this and maintain a nice syntax.

BenjaBobs avatar Nov 15 '18 10:11 BenjaBobs

Might be here: https://github.com/evo-terren/Gremlin.Net.CosmosDb/blob/master/src/Gremlin.Net.CosmosDb/Serialization/GremlinQuerySerializer.cs#L184-L202

When I add an additional overload to the class GremlinQuerySerializer you mentioned the query is parsed correctly.

private void Serialize(IEnumerable<int> array)
{
    var serializedObj = JsonConvert.SerializeObject(array, Formatting.None, _serializerSettings);
    _writer.Write(serializedObj);
}

The replace functionality in private void Serialize(object obj) seem to be not needed for a simple IEnumerable<int>, so this just might work.

GerbenGaastra avatar Nov 16 '18 09:11 GerbenGaastra

I'm still not sure this is the best method of solving this, as the underlying data is still formatted wrong and might lead to errors in the future. The problem is that P.Within(priceObjectArray) produces this data structure:

[
	5,
	13,
	27
]

While P.Within(priceIntArray) produces

[
	[
		5,
		13,
		27
	]
]

Due to the signature of P.Within(params object[]), the int array is interpreted as a single object instead of an array of objects. I worry this might lead to unexpected behavior.

BenjaBobs avatar Nov 16 '18 10:11 BenjaBobs

I believe this issue was previously documented as this issue: https://github.com/evo-terren/Gremlin.Net.CosmosDb/issues/32

Soon after, it was addressed by this commit: https://github.com/evo-terren/Gremlin.Net.CosmosDb/commit/ae3b63fa435c87a9b4c21a387b17396840de467a

This test scenario is passing, and it seems to be almost the same as the one from the original post. https://github.com/evo-terren/Gremlin.Net.CosmosDb/blob/94e1fe861aa9d0429b937d41a4a4e0a287d30730/test/Gremlin.Net.CosmosDb.Tests/TraversalSerialization/QuerySerializerTests.cs#L225

SamHard avatar Jun 22 '19 04:06 SamHard