[US642] JSON Patch Query preparations - use JsonPath instead of JsonPointer#1
Conversation
| DocumentContext nodeContext = JsonPath.parse(node.deepCopy()); | ||
|
|
||
| final JsonNode parentNode = nodeContext.read(jsonPath); | ||
| if (parentNode == null) |
There was a problem hiding this comment.
I would put { } around, same applies to other one-liner ifs
There was a problem hiding this comment.
That should be done for all one liner ifs.
| throw new JsonPatchException(BUNDLE.getMessage( | ||
| "jsonPatch.noSuchPath")); | ||
| public JsonNode apply(final JsonNode node) throws JsonPatchException { | ||
| String jsonPath = JsonPathParser.tmfStringToJsonPath(from); |
There was a problem hiding this comment.
Here is no final keyword, line below (and other places) it is. Is there a specific reason/convention for that? I can live with no final in front of local vars.
There was a problem hiding this comment.
No, it was just me being a bit frivolous with final, fixed
| if (parentNode.isMissingNode()) | ||
| throw new JsonPatchException(BUNDLE.getMessage( | ||
| "jsonPatch.noSuchParent")); | ||
| final int lastSlashIndex = path.lastIndexOf('/'); |
There was a problem hiding this comment.
Can we have a test with paths like
/this/is/my/path
and
/this/is/my/path/
Will both work?
There was a problem hiding this comment.
First path obviously works.
Technically the second path leads to an element with name equal to empty string, inside a node with name "path", this is currently not working in json path anyway, I filed a ticket for it and it's on our channel.
Our implementation might work with it, but tests are commented out due to that bug.
|
|
||
| import java.io.IOException; | ||
|
|
||
| public class AddQueryOperationTest extends Object { |
There was a problem hiding this comment.
extends Object isn't it by default?
There was a problem hiding this comment.
This is a bit ugly, because I couldn't ignore a test in testng. It's a special case, because annotation is in a superclass, so two standard methods I used didn't work (or actually worked intermittently).
I added a comment:
// TODO extend with JsonPatchOperationTest and uncomment constructor when this test needs to be active, couldn't ignore it otherway
| public class AddQueryOperationTest extends Object { | ||
|
|
||
| public AddQueryOperationTest() throws IOException { | ||
| //super("query/add"); |
There was a problem hiding this comment.
do we need it? (similar in other *OperationTest classes)
| return "$"; | ||
| } | ||
| final String jsonPath = "$" + path.replace('/', '.') | ||
| .replaceAll("\\.(\\d+)\\.", ".[$1].") |
There was a problem hiding this comment.
Can we move those regexps to final statics fields and give them meaningful description?
piotrdytkowski
changed the title
[US642] Update project settings (#3)
3 participants
No description provided.