[jira] [Commented] (SLING-12269) Changed behaviour in commons json when used as a dependency
[
https://issues.apache.org/jira/browse/SLING-12269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17828626#comment-17828626
]
Remo Liechti commented on SLING-12269:
--
to restore the happy path functionality, it is enough to patch getString of
JSONObject and JSONArray. However, the edge cases now throw different
exceptions (before: ClassCast, NumberFormat. Now: JsonException)
Please see attached patch that would give us the same old behavior, with
slightly adapted unit tests. Marked the two lines changed in JSONObject and
JSONArray as well as added a note to the manual steps in the readme.
[^SLING-12269.patch]
> Changed behaviour in commons json when used as a dependency
> ---
>
> Key: SLING-12269
> URL: https://issues.apache.org/jira/browse/SLING-12269
> Project: Sling
> Issue Type: Bug
> Components: Commons
>Reporter: Remo Liechti
>Priority: Blocker
> Attachments: SLING-12269.patch
>
>
> With the new internal release 2.0.22, there seems to be a different behaviour
> when it comes to get values from JSONObject.
> The new version checks for types and throws an exception, as of the old
> version simply called toString() on any object found.
> *Old:*
> {{public String getString(String key) throws JSONException {}}
> {{ return get(key).toString();}}
> {{}}}
> *New:*
> {{public String getString(String key) throws JSONException {}}
> {{ Objectobject=this.get(key);}}
> {{ if (objectinstanceofString) {}}
> {{ return (String) object;}}
> }
> {{ throwwrongValueFormatException(key, "string", object, null);}}
> {{}}}
>
> Same is true for all other types, such as getInt, getLong etc.
> There might be more such small differences in behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12269) Changed behaviour in commons json when used as a dependency
[
https://issues.apache.org/jira/browse/SLING-12269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17828433#comment-17828433
]
Remo Liechti commented on SLING-12269:
--
not so sure, if somebody was actually having an inputfield on a UI where the
user could enter some numbers, and was checking for NumberFormatException in
order to present a proper error message to the user, this could be broken.
> Changed behaviour in commons json when used as a dependency
> ---
>
> Key: SLING-12269
> URL: https://issues.apache.org/jira/browse/SLING-12269
> Project: Sling
> Issue Type: Bug
> Components: Commons
>Reporter: Remo Liechti
>Priority: Blocker
>
> With the new internal release 2.0.22, there seems to be a different behaviour
> when it comes to get values from JSONObject.
> The new version checks for types and throws an exception, as of the old
> version simply called toString() on any object found.
> *Old:*
> {{public String getString(String key) throws JSONException {}}
> {{ return get(key).toString();}}
> {{}}}
> *New:*
> {{public String getString(String key) throws JSONException {}}
> {{ Objectobject=this.get(key);}}
> {{ if (objectinstanceofString) {}}
> {{ return (String) object;}}
> }
> {{ throwwrongValueFormatException(key, "string", object, null);}}
> {{}}}
>
> Same is true for all other types, such as getInt, getLong etc.
> There might be more such small differences in behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12269) Changed behaviour in commons json when used as a dependency
[
https://issues.apache.org/jira/browse/SLING-12269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17828429#comment-17828429
]
Robert Munteanu commented on SLING-12269:
-
Right, in this case the new version is more lenient and uses {{toString()}}
instead of conditionally downcasting. That is a behaviour change indeed, but I
am personally not very concerned about that as it will not break consumers
relying on the old behaviour.
> Changed behaviour in commons json when used as a dependency
> ---
>
> Key: SLING-12269
> URL: https://issues.apache.org/jira/browse/SLING-12269
> Project: Sling
> Issue Type: Bug
> Components: Commons
>Reporter: Remo Liechti
>Priority: Blocker
>
> With the new internal release 2.0.22, there seems to be a different behaviour
> when it comes to get values from JSONObject.
> The new version checks for types and throws an exception, as of the old
> version simply called toString() on any object found.
> *Old:*
> {{public String getString(String key) throws JSONException {}}
> {{ return get(key).toString();}}
> {{}}}
> *New:*
> {{public String getString(String key) throws JSONException {}}
> {{ Objectobject=this.get(key);}}
> {{ if (objectinstanceofString) {}}
> {{ return (String) object;}}
> }
> {{ throwwrongValueFormatException(key, "string", object, null);}}
> {{}}}
>
> Same is true for all other types, such as getInt, getLong etc.
> There might be more such small differences in behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12269) Changed behaviour in commons json when used as a dependency
[
https://issues.apache.org/jira/browse/SLING-12269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17828428#comment-17828428
]
Remo Liechti commented on SLING-12269:
--
Similar Story for JSONArray and maybe more classes, to be checked.
> Changed behaviour in commons json when used as a dependency
> ---
>
> Key: SLING-12269
> URL: https://issues.apache.org/jira/browse/SLING-12269
> Project: Sling
> Issue Type: Bug
> Components: Commons
>Reporter: Remo Liechti
>Priority: Blocker
>
> With the new internal release 2.0.22, there seems to be a different behaviour
> when it comes to get values from JSONObject.
> The new version checks for types and throws an exception, as of the old
> version simply called toString() on any object found.
> *Old:*
> {{public String getString(String key) throws JSONException {}}
> {{ return get(key).toString();}}
> {{}}}
> *New:*
> {{public String getString(String key) throws JSONException {}}
> {{ Objectobject=this.get(key);}}
> {{ if (objectinstanceofString) {}}
> {{ return (String) object;}}
> }
> {{ throwwrongValueFormatException(key, "string", object, null);}}
> {{}}}
>
> Same is true for all other types, such as getInt, getLong etc.
> There might be more such small differences in behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12269) Changed behaviour in commons json when used as a dependency
[
https://issues.apache.org/jira/browse/SLING-12269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17828427#comment-17828427
]
Remo Liechti commented on SLING-12269:
--
I think the behaviour is changed for all methods.
I.e. for getLong:
*Old:*
public long getLong(String key) throws JSONException {
Object o = get(key);
return o instanceof Number ?
((Number)o).longValue() :
Long.valueOf((String)o).longValue();
}
+Cases+
# Object o is Number: returns long
# Object o is String of format Long: returns long
# Object o is String with invalid format: throws a NumberFormatException
# Object o is no String: throws ClassCastException
*New:*
public long getLong(String key) throws JSONException {
final Object object = this.get(key);
if (object instanceof Number) {
return ((Number) object).longValue();
}
try {
return Long.parseLong(object.toString());
} catch (Exception e) {
throw wrongValueFormatException(key, "long", object, e);
}
}
+Cases+
# Object o is Number: returns long
# Object o is any type, does toString() with valid format: returns long
# Object o is any type, does toString() with invalid format: throws a
JsonException
# throws no more ClassCastException nor NumberFormatException
> Changed behaviour in commons json when used as a dependency
> ---
>
> Key: SLING-12269
> URL: https://issues.apache.org/jira/browse/SLING-12269
> Project: Sling
> Issue Type: Bug
> Components: Commons
>Reporter: Remo Liechti
>Priority: Blocker
>
> With the new internal release 2.0.22, there seems to be a different behaviour
> when it comes to get values from JSONObject.
> The new version checks for types and throws an exception, as of the old
> version simply called toString() on any object found.
> *Old:*
> {{public String getString(String key) throws JSONException {}}
> {{ return get(key).toString();}}
> {{}}}
> *New:*
> {{public String getString(String key) throws JSONException {}}
> {{ Objectobject=this.get(key);}}
> {{ if (objectinstanceofString) {}}
> {{ return (String) object;}}
> }
> {{ throwwrongValueFormatException(key, "string", object, null);}}
> {{}}}
>
> Same is true for all other types, such as getInt, getLong etc.
> There might be more such small differences in behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (SLING-12269) Changed behaviour in commons json when used as a dependency
[
https://issues.apache.org/jira/browse/SLING-12269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17828419#comment-17828419
]
Robert Munteanu commented on SLING-12269:
-
Thanks for the report [~rliechti]. I would like to understand what methods are
affected, so here's a comparison table:
||Method||Version 2.0.20|| Version 2.0.22|| Change? ||
|JSONObject.getString|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L487-L489|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/7a15fe774b6892c770af4d7ad12aa1d1bd7f976c/src/main/java/org/apache/sling/commons/json/JSONObject.java#L913-L919
| (!) Yes, 2.0.22 fails if value is not string |
| JSONObject.getBoolean |
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L375-L388
|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/7a15fe774b6892c770af4d7ad12aa1d1bd7f976c/src/main/java/org/apache/sling/commons/json/JSONObject.java#L663-L675
| No, both versions need either "true" or "false", case insensitive |
| JSonObject.getDouble |
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L398-L408
|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L398-L408
| No, both versions check for {{Number}} and try to parse if needed |
| JSonObject.getInt |
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L420-L424
|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/7a15fe774b6892c770af4d7ad12aa1d1bd7f976c/src/main/java/org/apache/sling/commons/json/JSONObject.java#L795-L805
| No, both versions check for {{Number}} and try to parse if needed |
| JSONObject.getJSONArray |
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L435-L442
|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/7a15fe774b6892c770af4d7ad12aa1d1bd7f976c/src/main/java/org/apache/sling/commons/json/JSONObject.java#L817-L823
| No, both versions throw exceptions if the instanceof check fails |
| JSONObject.getJSONObject |
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L453-L460
|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/7a15fe774b6892c770af4d7ad12aa1d1bd7f976c/src/main/java/org/apache/sling/commons/json/JSONObject.java#L835-L841
| No, both versions throw exceptions if the instanceof check fails |
| JSONObject.getLong |
https://github.com/apache/sling-org-apache-sling-commons-json/blob/43d3030dbb967232d129ed19f79d0b89b82f4965/src/main/java/org/apache/sling/commons/json/JSONObject.java#L472-L477
|
https://github.com/apache/sling-org-apache-sling-commons-json/blob/7a15fe774b6892c770af4d7ad12aa1d1bd7f976c/src/main/java/org/apache/sling/commons/json/JSONObject.java#L854-L864
| No, both versions check for {{Number}} and try to parse if needed |
It looks to me like {{getString}} is the only problematic change. Do you see
anything else?
> Changed behaviour in commons json when used as a dependency
> ---
>
> Key: SLING-12269
> URL: https://issues.apache.org/jira/browse/SLING-12269
> Project: Sling
> Issue Type: Bug
>Reporter: Remo Liechti
>Priority: Blocker
>
> With the new internal release 2.0.22, there seems to be a different behaviour
> when it comes to get values from JSONObject.
> The new version checks for types and throws an exception, as of the old
> version simply called toString() on any object found.
> *Old:*
> {{public String getString(String key) throws JSONException {}}
> {{ return get(key).toString();}}
> {{}}}
> *New:*
> {{public String getString(String key) throws JSONException {}}
> {{ Objectobject=this.get(key);}}
> {{ if (objectinstanceofString) {}}
> {{ return (String) object;}}
> }
> {{ throwwrongValueFormatException(key, "string", object, null);}}
> {{}}}
>
> Same is true for all other types, such as getInt, getLong etc.
> There might be more such small differences in behaviour.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
