[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)