Http and Bolt clients serialize input parameters differently

[andrey-kastryukhin]

Description There is a difference in input parameters serialization between Http and Bolt clients. So, if any existing solution is migrating from HttpClient to BoltClient, some unexpected bugs can arise under certain conditions.

HttpClient has a property Serializer which is intialized as CustomJsonSerializer instance. CustomJsonSerializer uses under the hood JsonSerializer with the setting NullValueHandling.Ignore defined.

In contrast, BoltClient uses Neo4jDriverExtensions.Serialize(). The underlying private methods don't skip the properties with null values.

Versions:

• Neo4jClient 4.1.15
• Bolt AND Http
• .NET Framework 4.8
• DB 3.5

Code snippet

var actor = new
{
    newProperty = "New property we'd like to set2",
    born = (int?)null
};

var query = graphClient
    .Cypher
    .Match("(actor:Person {name: 'Keanu Reeves'})")
    .Set("actor += $pActor")
    .WithParam("pActor", actor);

await query.ExecuteWithoutResultsAsync();

HttpClient will set newProperty to the node and preserves born. BoltClient will set newProperty to the node and set born property to null.

Expected behaviour BoltClient must run queries identically to HttpClient.

Full example Program.zip

[cskardon]

Firstly, I totally agree that they should do the same.

What I'm not sure about is the correct version. The way the Bolt client is doing it is the way the += tends to be used in Cypher, and setting a property to null is a standard way (to the extent it's part of the teaching materials) to delete a property from a Node/Relationship.

By ignoring the null in the Http version - it's in effect choosing to hide that behaviour from the users.

Personally - I prefer the Bolt one - as it matches what I would expect. But I suspect the best approach is to replicate the Http one - with the option to turn it off.

[Clooney24]

Hi Charlotte,

not an easy decision. From our point of view, we used += as a kind of merging of values into a node, where null was not meant as a value, but as a "property that is not yet set". So a non-null value would win against a null value.

I would prefer "to replicate the Http one - with the option to turn it off." Also just discussed that with Andrey (we are working together on the same project). As HttpClient is older, it should be treated as truth. Otherwise Migration from HttpClient to BoltClient could cause a lot of trouble. In our case, we would have to check and test 100+ queries where we used this += operator.

Best, Reiner

[andrey-kastryukhin]

@cskardon , @Clooney24 have we come to any conclusion?

[cskardon]

@andrey-kastryukhin It would be to replicate the HTTP functionality, with a switch to choose how to do the null handling for Bolt.

Easy migration from Http to Bolt is highest priority - and it's not too much effort to switch them to the other way of handling nulls.

[andrey-kastryukhin]

@cskardon Do you have any timeframe for the agreed solution?

[cskardon]

errr no, this is open source, and I don't get paid to do it - so unfortunately - work will always take precedence, then life, then this.

[Clooney24]

Sure. We will try a PR for this.

Can we do anything to get this Neo4jClient an official part of Neo4j so you will get paid for this in future ;-) The official C# driver is not really usable for serious business applications, even with your extensions.

[cskardon]

To be clear - it's not because I don't want to! I would love to spend time on this, but I have SO many other things at the moment, time is tight!

I don't think it will ever be a full part of Neo4j, and there are some big benefits to that, for you to be able to do the PR and me pull it in is relatively simple - and we can discuss how to approach the problem and fix it, and then it's there.

If it were part of Neo4j - I think the process would be significantly longer, and we'd be waiting on release cycles etc -

BUT I am painfully aware of the annoyance in terms of getting things done - relying on either yourself or me to fix it - it's a complicated thing to work with :/

[Clooney24]

We know about your time, and you are right, I have not thought about these benefits until now. Andrey will ask some questions about the technical approach and then we will try to fix it.

[andrey-kastryukhin]

@cskardon Then it would be great to consider the way to fix the issue. As you said, BoltClient should have null-handling configurable. So, BoltClient constructors should be modified. What to pass there? A simple boolean value SerializeNullValues or maybe create a class BoltClientConfiguration having SerializeNullValues as its property.

Do you have any preference?

[cskardon]

I think a class with default values set would be the most extensible, it would allow for anything else like this to happen in an easier way

[andrey-kastryukhin]

@cskardon Hi Charlotte,

Following the constructors of BoltGraphClient I've realized some ExecutionConfiguration class do exist. It combines the connection related data including ExecutionConfiguration.JsonConverters property. Now I think it would be good to keep new expected SerializeNullValues property there as well. How do you think? Basically that means, the class I was going to introduce looks excessive and we can just include a simple serializeNullValues parameters to constructor(s). NOTE: the properties of ExecutionConfiguration class have setters and can be overwritten at any moment. Honestly, I'd like to avoid those setters, but we cannot change this without compatibility issues.

[cskardon]

Hey Andrey,

Adding the serializeNullValues to the constructor of the ExecutionConfiguration class? or the BoltGraphClient?

[cskardon]

Excuse the clumsy 'close/open' - oops

[andrey-kastryukhin]

ExecutionConfiguration class contains almost all parameters came to controller. So yes, to ExecutionConfiguration class

[cskardon]

Then yes, that's fine!

[andrey-kastryukhin]

@cskardon Hi Charlotte,

This is the PR with the fix: https://github.com/DotNet4Neo4j/Neo4jClient/pull/423

[cskardon]

Hey @andrey-kastryukhin,

As you can (hopefully!) see - I added one change request - I think it makes sense, let me know if you have any problems/questions.

[andrey-kastryukhin]

@cskardon The requested changes have been pushed

[cskardon]

This is now in 4.1.18 which is available (or should be) now.

Thank you for doing this!