You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by GitBox <gi...@apache.org> on 2020/03/20 10:15:20 UTC
[GitHub] [johnzon] Neoklosch opened a new pull request #62: Added
possibility to use minus as array index for the remove operation.
Neoklosch opened a new pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62
## Problem
Currently it is not allowed to remove an entry by using a `-` as array index.
Since it is allowed for the add operation, it should also be allowed for all the other operations.
Further this is necessary if johnzon is used together with the JSON Patch library.
## Solution
I've added a check for the `-` index in the `getArrayIndex` method to return the index of the last entry.
Additionally I've made the check in the `validateArrayIndex` more explicit.
## Test(s) added
One test that perform a remove with a `-` as array index.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau commented on a change in pull request #62:
Added possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#discussion_r398339315
##########
File path: johnzon-core/src/test/java/org/apache/johnzon/core/JsonPointerTest.java
##########
@@ -456,6 +456,19 @@ public void testRemoveArrayElement() {
assertEquals("[[\"bar\",\"baz\"]]", result.toString()); // [["bar","baz"]]
}
+ @Test
+ public void testRemoveLastArrayElement() {
Review comment:
maybe add a test for "" too since this case is handled differently
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau commented on a change in pull request #62:
Added possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#discussion_r398512320
##########
File path: johnzon-core/src/test/java/org/apache/johnzon/core/JsonPointerTest.java
##########
@@ -456,6 +456,19 @@ public void testRemoveArrayElement() {
assertEquals("[[\"bar\",\"baz\"]]", result.toString()); // [["bar","baz"]]
}
+ @Test
+ public void testRemoveLastArrayElement() {
Review comment:
I was more thinking about the + case (know it is not what you changed but your change moves from (a || b) to (a || (b && c)), I just want to prevent the fact it becomes ((a || b) && c)
- or something else - during another enhancement by a test executed in the build.
Hope it makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau commented on a change in pull request #62:
Added possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#discussion_r398512320
##########
File path: johnzon-core/src/test/java/org/apache/johnzon/core/JsonPointerTest.java
##########
@@ -456,6 +456,19 @@ public void testRemoveArrayElement() {
assertEquals("[[\"bar\",\"baz\"]]", result.toString()); // [["bar","baz"]]
}
+ @Test
+ public void testRemoveLastArrayElement() {
Review comment:
I was more thinking about the + case (know it is not what you changed but your change moves from (a || b) to (a || (b && c)), I just want to prevent the fact it becomes ((a || b) && c)
[or something else] during another enhancement by a test executed in the build.
Hope it makes sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau edited a comment on issue #62: Added
possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau edited a comment on issue #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#issuecomment-604855057
merged since I will start the release and don't want to hold this enhancement for a test, feel free to add the test we spoke about in another PR
in any case, thanks a lot, this is really appreciated!
Edit: "as this" this PR breaks some tests and cases, will fix them now
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau commented on a change in pull request #62:
Added possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#discussion_r398339170
##########
File path: johnzon-core/src/main/java/org/apache/johnzon/core/JsonPointerImpl.java
##########
@@ -339,8 +339,7 @@ private JsonValue getValue(JsonValue jsonValue, String referenceToken, int curre
try {
JsonArray jsonArray = (JsonArray) jsonValue;
- int arrayIndex = Integer.parseInt(referenceToken);
- validateArraySize(jsonArray, arrayIndex, jsonArray.size());
Review comment:
seems we loose that validation for number cases, no?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau merged pull request #62: Added possibility to
use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau merged pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] Neoklosch commented on a change in pull request #62:
Added possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
Neoklosch commented on a change in pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#discussion_r398453456
##########
File path: johnzon-core/src/main/java/org/apache/johnzon/core/JsonPointerImpl.java
##########
@@ -339,8 +339,7 @@ private JsonValue getValue(JsonValue jsonValue, String referenceToken, int curre
try {
JsonArray jsonArray = (JsonArray) jsonValue;
- int arrayIndex = Integer.parseInt(referenceToken);
- validateArraySize(jsonArray, arrayIndex, jsonArray.size());
Review comment:
The array size validation will be executed in the validateArraySize (https://github.com/apache/johnzon/blob/d1c6a2f62ef0bb5a798161799eae4602e20a233e/johnzon-core/src/main/java/org/apache/johnzon/core/JsonPointerImpl.java#L469-L472).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] rmannibucau commented on issue #62: Added possibility to
use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on issue #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#issuecomment-604855057
merged since I will start the release and don't want to hold this enhancement for a test, feel free to add the test we spoke about in another PR
in any case, thanks a lot, this is really appreciated!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [johnzon] Neoklosch commented on a change in pull request #62:
Added possibility to use minus as array index for the remove operation.
Posted by GitBox <gi...@apache.org>.
Neoklosch commented on a change in pull request #62: Added possibility to use minus as array index for the remove operation.
URL: https://github.com/apache/johnzon/pull/62#discussion_r398459224
##########
File path: johnzon-core/src/test/java/org/apache/johnzon/core/JsonPointerTest.java
##########
@@ -456,6 +456,19 @@ public void testRemoveArrayElement() {
assertEquals("[[\"bar\",\"baz\"]]", result.toString()); // [["bar","baz"]]
}
+ @Test
+ public void testRemoveLastArrayElement() {
Review comment:
If I get your question correct, there is already a test for it: https://github.com/apache/johnzon/blob/d1c6a2f62ef0bb5a798161799eae4602e20a233e/johnzon-core/src/test/java/org/apache/johnzon/core/JsonPointerTest.java#L390-L396
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services