Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Issue #497 and add a testcase #699

Merged
merged 4 commits into from Jun 2, 2021
Merged

Fix Issue #497 and add a testcase #699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
de9413d
The method getPropertyKeys() in class JsonOrgJsonProvider doesn't che…
Pigdrum Apr 25, 2021
1132f1a
Addition testcase
Pigdrum Apr 25, 2021
5bf7257
simplify the implementation. And change method names of testcases to …
Pigdrum May 6, 2021
2e749f5
change names
Pigdrum May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 5 additions & 7 deletions json-path/src/main/java/com/jayway/jsonpath/spi/json/JsonOrgJsonProvider.java
View file
Open in desktop
Expand Up @@ -14,6 +14,7 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;

public class JsonOrgJsonProvider extends AbstractJsonProvider {

Expand Down Expand Up @@ -143,17 +144,14 @@ public boolean isMap(Object obj) {
return (obj instanceof JSONObject);
}

@SuppressWarnings("unchecked")
@Override
public Collection<String> getPropertyKeys(Object obj) {
JSONObject jsonObject = toJsonObject(obj);
Copy link

@lodenrogue lodenrogue May 6, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably simplify this method with streams (Sorry for any syntax errors. I'm writing this in the browser):

JSONObject jsonObject  = toJsonObject(obj);
try {
    if(Objects.isNull(jsonObject.names()) return new ArrayList<>();
    
    return jsonObject.names().toList().stream()
        .map(Object::toString)
        .collect(Collectors.toList());

} catch(JSONException e) {
// Rest of the code
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thanks! I have seen all the suggestions you mentioned, and I will change it as soon as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that the return type of jsonObject.names() is JsonArray, which doesn't have method like toList. But the jsonObject has another method keySet which return the set of key that is needed.
(PS: the implementation of jsonObject.names() is that it iteratively adds the value in keySet into JsonArray

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stleary.github.io/JSON-java/org/json/JSONObject.html#names--
https://stleary.github.io/JSON-java/org/json/JSONArray.html#toList--

JSONObject::names should return a JSONArray
JSONArray should have a method called toList() according to their documentation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I updated the code snippet. Instead of casting (String) name you can do Object::toString

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I find that the version of org.Json in JsonPath is 20140107, where JSONArray has no method toList.toList is introduced in version 20160807. What could I do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my latest two commits, thanks!
The code of method .name() is as follows, I think it is not necessary to use .name(). Just using .keySet() will be fine.

public Iterator keys() {
        return this.keySet().iterator();
    }

public JSONArray names() {
        JSONArray ja = new JSONArray();
        Iterator keys = this.keys();
        while (keys.hasNext()) {
            ja.put(keys.next());
        }
        return ja.length() == 0 ? null : ja;
    }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is my first time to contribute to the open source community, sincerely thanks for your suggestions and approval !

List<String> keys = new ArrayList<String>();
try {
for (int i = 0; i < jsonObject.names().length(); i++) {
String key = (String) jsonObject.names().get(i);
keys.add(key);

}
return keys;
if(Objects.isNull(jsonObject.names()))
return new ArrayList<>();
return jsonObject.keySet();
} catch (JSONException e) {
throw new JsonPathException(e);
}
Expand Down
21 changes: 21 additions & 0 deletions json-path/src/test/java/com/jayway/jsonpath/JsonOrgJsonProviderTest.java
View file
Open in desktop
@@ -1,5 +1,7 @@
package com.jayway.jsonpath;

import com.jayway.jsonpath.spi.json.JsonOrgJsonProvider;
import com.jayway.jsonpath.spi.mapper.JsonOrgMappingProvider;
import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.Test;
Expand Down Expand Up @@ -76,4 +78,23 @@ public void read_book_length() {
assertThat(result).isEqualTo(4);
}

@Test
public void test_getPropertyKeys_empty_object() {
String json = "{\"foo\": \"bar\", \"emptyObject\": {},\"emptyList\":[]}";
Configuration config = Configuration.defaultConfiguration()
.jsonProvider(new JsonOrgJsonProvider())
.mappingProvider(new JsonOrgMappingProvider());
Object result = JsonPath.using(config).parse(json).read("$..foo");
assertThat(result.toString()).isEqualTo("[\"bar\"]");
}

@Test
public void test_getPropertyKeys_empty_nest_object() {
String json = "{\"foo\": \"bar\", \"emptyObject\": {\"emptyList\":[]},\"emptyList\":[]}";
Configuration config = Configuration.defaultConfiguration()
.jsonProvider(new JsonOrgJsonProvider())
.mappingProvider(new JsonOrgMappingProvider());
Object result = JsonPath.using(config).parse(json).read("$..foo");
assertThat(result.toString()).isEqualTo("[\"bar\"]");
}
}