Catch run-time and compile-time server exceptions #49
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DvirDukhan
requested changes
Sep 11, 2019
| contextedRedisGraph.setRedisGraphCaches(caches); | ||
| return contextedRedisGraph.sendQuery(graphId, preparedQuery); | ||
| } | ||
| catch (Exception e) { |
Contributor
There was a problem hiding this comment.
remove this try-catch clause please, as it does nothing
| this.cache = cache; | ||
|
|
||
| // If a run-time error occured, the last member of the rawResponse will be a JedisDataException. | ||
| if (rawResponse.get(rawResponse.size()-1) instanceof JedisDataException) { |
Contributor
There was a problem hiding this comment.
few questions:
- Isn't a JedisDataException thrown, and not returned?
- If the exception is returned, don't you think it is better to define our own exception class such as JRedisGraphExecption? This is for making a difference between general Jedis/Redis exceptions, and JRedisGraph/RedisGraph exceptions.
| } | ||
|
|
||
| @Test | ||
| public void testErrorReporting() { |
Contributor
There was a problem hiding this comment.
Please follow the following example for Juint 4 framework exception assertion:
- first define a rule
@Rule
public ExpectedException exceptionRule = ExpectedException.none();
Then, in the test itself, set the rule expected exception type and message:
@Test
public void testRuntimeException() {
api.query("social", "create ()");
exceptionRule.expect(JedisDataException.class);
exceptionRule.expectMessage("Type mismatch: expected Integer but was String");
api.query("social", "RETURN toUpper(5)");
}
I know it is a bit awkward, once we'll migrate to Juint 5 it will be easier
DvirDukhan
requested changes
Sep 11, 2019
| return new ResultSetImpl(rawResponse, this, caches.getGraphCache(graphId)); | ||
| } | ||
| catch (JedisDataException j) { | ||
| throw new JRedisGraphCompileTimeError(j); |
Contributor
There was a problem hiding this comment.
- please close the connection
conn.close(); - since
JRedisGraphRunTimeErrorextendsJedisDataExceptionthe catch clause will catch it. write additional catch for this type, and just throw. Write it before theJedisDataExceptioncatch, since it inherits from it.
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
- Coverage 76.15% 75.86% -0.29%
==========================================
Files 20 22 +2
Lines 478 493 +15
Branches 74 75 +1
==========================================
+ Hits 364 374 +10
- Misses 85 90 +5
Partials 29 29
Continue to review full report at Codecov.
|
gkorland
reviewed
Sep 12, 2019
| import redis.clients.jedis.exceptions.JedisDataException; | ||
|
|
||
|
|
||
| public class JRedisGraphCompileTimeError extends JedisDataException { |
| import redis.clients.jedis.exceptions.JedisDataException; | ||
|
|
||
|
|
||
| public class JRedisGraphRunTimeError extends JedisDataException { |
| return new ResultSetImpl(rawResponse, this, caches.getGraphCache(graphId)); | ||
| } | ||
| catch (JedisDataException j) { | ||
| throw new JRedisGraphCompileTimeError(j); |
DvirDukhan
force-pushed
the
runtime-error-handling
branch
from
e3ee2ab to
c61a5eb
Compare
DvirDukhan
approved these changes
Sep 12, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This or similar will be necessary following the merge of RedisGraph/RedisGraph#613
Compile-time errors are received by the client
sendQuerylogic asJedisDataExceptions, while for run-time errors, the last top-level member of the response will be aJedisDataException.I believe there may be an issue with premature thread closures here:
https://github.com/RedisGraph/JRedisGraph/blob/master/src/main/java/com/redislabs/redisgraph/impl/api/ContextedRedisGraph.java#L54
I think it would be preferable to not close the connection, at least for
JedisDataExceptionfailures.Let me know your thoughts!