[GitHub] [tinkerpop] Haapsaari-Juha opened pull request #1247: Null checks added tp Connection.Parse

["Haapsaari-Juha (GitHub)" <gi...@apache.org>]

Added null check to Gremlin.Net.Driver.Connection.Parse method for receivedMsg variable and responseHandler. My first pull request to public repo, hope I did it correctly

[ Full content available at: https://github.com/apache/tinkerpop/pull/1247 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on issue #1247: TINKERPOP-2192 Null checks added to Connection.Parse

["spmallette (GitHub)" <gi...@apache.org>]

i have no clue why docker is failing on the gremlin-python portion of the build. works fine for me locally - it's just doing `docker/build.sh` with no flags. not sure if anyone else can get it to fail locally...

[ Full content available at: https://github.com/apache/tinkerpop/pull/1247 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1247: TINKERPOP-2192 Null checks added to Connection.Parse

["FlorianHockmann (GitHub)" <gi...@apache.org>]

> Also at same time not sure if it hides bad JSON setting or other json related behind. Should we fail totally in this case?

We would still get an error in that case [from here](https://github.com/apache/tinkerpop/blob/master/gremlin-dotnet/src/Gremlin.Net/Driver/Connection.cs#L136) where the `receivedMsg` is also accessed without a null check. If `receivedMsg` is `null` then `TryParseResponseMessage` will throw a `NullReferenceException` which will be caught by the try/catch in `Parse` and we notify the response handler about the exception.
I think that we should however ensure that we're not getting an exception in a `catch` clause.

[ Full content available at: https://github.com/apache/tinkerpop/pull/1247 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] FlorianHockmann commented on issue #1247: TINKERPOP-2192 Null checks added to Connection.Parse

["FlorianHockmann (GitHub)" <gi...@apache.org>]

> Will not because message is null. There there is silent error eating.

You're right. I should have looked taken a closer look.

> Could we just check that deserialized object is not null and throw exception.

Yes, I think that might be the best option as we expect the message to never be null. So, if for some reason is actually null, then we can throw an exception which will result in a call to `CloseConnectionBecauseOfFailureAsync` which closes the connection completely and notifies all existing response handlers about the exception.

@Haapsaari-Juha: I don't know if you want to incorporate these changes into your PR as you were the first to contribute a fix for this issue or if @dzmitry-lahoda should update his PR accordingly.

> property based testing for any other array passed

Could you explain a bit further what you mean by this? Do you want to add property based testing for the internal classes of the driver?


[ Full content available at: https://github.com/apache/tinkerpop/pull/1247 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] dzmitry-lahoda commented on issue #1247: TINKERPOP-2192 Null checks added to Connection.Parse

["dzmitry-lahoda (GitHub)" <gi...@apache.org>]

> Parse and we notify the response handler about the exception.

Will not because message is null. There there is silent error eating. 

Could we just check that deserialized object is not null and throw exception.

```
       var receivedMsg = _messageSerializer.DeserializeMessage<ResponseMessage<JToken>>(received);
            if (receivedMsg == null)
            {
                throw new Exception("receivedMsg");
            }
```

To check what happens in case of null array passed, in case of empty array passed, and do property based testing for any other array passed. So it will be more explainable failure than just eating null.




[ Full content available at: https://github.com/apache/tinkerpop/pull/1247 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org