groovy
  1. groovy
  2. GROOVY-5169

JsonOutput.toJson(object) is not returning expected results

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 1.8.4
    • Fix Version/s: None
    • Component/s: JSON
    • Labels:
    • Number of attachments :
      6

      Description

      Consider the attached Groovy script. It defines two classes. Each has public properties explicitly defined. When I send the resulting objects to JsonOutput.toJson(), I expect the public properties of the object to be serialized into the JSON output. This is not working as expected. The only properties that get serialized are those created via "def propName" and those that seem to have getter methods (this in not actually the case).

      Attached is a screenshot of GroovyConsole running the attached script.

      1. 0001-bugfix.patch
        2 kB
        umang bhatt
      2. json_test.groovy
        0.8 kB
        James Sumners
      3. MetaClassImpl.java
        151 kB
        umang bhatt
      4. MetaClassImpl.patch
        2 kB
        blackdrag blackdrag
      1. GroovyConsole_output.png
        275 kB
      2. screenshot.png
        67 kB

        Activity

        Hide
        Roshan Dawrani added a comment -

        Yes. I think JsonOutput.toJson(Object) should be generally in sync with object.dump(), which it is not currently.

        Show
        Roshan Dawrani added a comment - Yes. I think JsonOutput.toJson(Object) should be generally in sync with object.dump(), which it is not currently.
        Hide
        blackdrag blackdrag added a comment -

        @James... if you use a visibility modifier in a declaration for a property, then you actually do not declare a property, but a field only. So if you do "public String bar", then you don't have a bar property, but a bar field. If you do "String bar", then you get a bar property.

        Show
        blackdrag blackdrag added a comment - @James... if you use a visibility modifier in a declaration for a property, then you actually do not declare a property, but a field only. So if you do "public String bar", then you don't have a bar property, but a bar field. If you do "String bar", then you get a bar property.
        Hide
        Roshan Dawrani added a comment -

        And a private getter makes a property?

        Show
        Roshan Dawrani added a comment - And a private getter makes a property?
        Hide
        Roshan Dawrani added a comment -

        Wouldn't it make sense if JsonOutput.toJson(Object) included public fields too?

        Show
        Roshan Dawrani added a comment - Wouldn't it make sense if JsonOutput.toJson(Object) included public fields too?
        Hide
        blackdrag blackdrag added a comment -

        the visibility of the getter is not included in determining if something is a property or not.

        Show
        blackdrag blackdrag added a comment - the visibility of the getter is not included in determining if something is a property or not.
        Hide
        Guillaume Laforge added a comment -

        When this was initially implemented, the idea was to use properties, not public fields or anything else.
        Also, I think only public properties should be used for that JSON serialization.
        So according to that logic and concern, I think with your example that foo should be serialized as {} and bar as {"bar":"foo"} – ie. only public getters used for the serialization.

        Show
        Guillaume Laforge added a comment - When this was initially implemented, the idea was to use properties, not public fields or anything else. Also, I think only public properties should be used for that JSON serialization. So according to that logic and concern, I think with your example that foo should be serialized as {} and bar as {"bar":"foo"} – ie. only public getters used for the serialization.
        Hide
        Roshan Dawrani added a comment -

        >> the visibility of the getter is not included in determining if something is a property or not.

        Didn't mean in the usual "property" sense. I meant outputting a private getter in a JSON looks odd (and public fields are being ignored). As Guillaume seems to agree, only public getters should be used for JSON serialization.

        Show
        Roshan Dawrani added a comment - >> the visibility of the getter is not included in determining if something is a property or not. Didn't mean in the usual "property" sense. I meant outputting a private getter in a JSON looks odd (and public fields are being ignored). As Guillaume seems to agree, only public getters should be used for JSON serialization.
        Hide
        James Sumners added a comment -

        From http://groovy.codehaus.org/Groovy+Beans – "In Groovy, fields and properties have been merged so that they act and look the same."

        Given that statement, I still think these objects should serialize as I outlined.

        Show
        James Sumners added a comment - From http://groovy.codehaus.org/Groovy+Beans – "In Groovy, fields and properties have been merged so that they act and look the same." Given that statement, I still think these objects should serialize as I outlined.
        Hide
        umang bhatt added a comment - - edited

        probably(i am not sure) we need to do changes in MetaClassImpl.java.

        I have made some changes and its working with me(its giving output as Mr. James said).

        what am i supposed to do now? to whom do i need to submit the change for review ?

        how do i run the test cases ? to whom i need to send the patch ?

        Show
        umang bhatt added a comment - - edited probably(i am not sure) we need to do changes in MetaClassImpl.java. I have made some changes and its working with me(its giving output as Mr. James said). what am i supposed to do now? to whom do i need to submit the change for review ? how do i run the test cases ? to whom i need to send the patch ?
        Hide
        blackdrag blackdrag added a comment -

        you can attach the patch here. It would help to check if the test can be applied to the code base without any test failing.

        Show
        blackdrag blackdrag added a comment - you can attach the patch here. It would help to check if the test can be applied to the code base without any test failing.
        Hide
        Pascal Schumacher added a comment -

        @umang bhatt:
        Could you attach your patch?

        Show
        Pascal Schumacher added a comment - @umang bhatt: Could you attach your patch?
        Hide
        umang bhatt added a comment - - edited

        I am a beginner so please review my code. Also i have not run any tests yet.

        I have changed public List<MetaProperty> getProperties() method.

        I have tried fixing the issue. The order is a little different than what is desired.
        I can change the behavior as you say(eg include the value that we get from the getter/accesor method).

        Show
        umang bhatt added a comment - - edited I am a beginner so please review my code. Also i have not run any tests yet. I have changed public List<MetaProperty> getProperties() method. I have tried fixing the issue. The order is a little different than what is desired. I can change the behavior as you say(eg include the value that we get from the getter/accesor method).
        umang bhatt made changes -
        Field Original Value New Value
        Attachment screenshot.png [ 62441 ]
        Hide
        umang bhatt added a comment - - edited

        I have attached the file that contains changes. I have also attached a screenshot.

        Show
        umang bhatt added a comment - - edited I have attached the file that contains changes. I have also attached a screenshot.
        umang bhatt made changes -
        Attachment MetaClassImpl.java [ 62442 ]
        Hide
        blackdrag blackdrag added a comment -

        Please always create a minimal patch file, just attaching the new file does not help much in judging if the change should be done or not. I created the patch for you, plus made some formatting changes to minimize the patch

        Show
        blackdrag blackdrag added a comment - Please always create a minimal patch file, just attaching the new file does not help much in judging if the change should be done or not. I created the patch for you, plus made some formatting changes to minimize the patch
        blackdrag blackdrag made changes -
        Attachment MetaClassImpl.patch [ 62443 ]
        umang bhatt made changes -
        Attachment MetaClassImpl.java [ 62442 ]
        Hide
        umang bhatt added a comment -

        patch

        Show
        umang bhatt added a comment - patch
        umang bhatt made changes -
        Attachment 0001-solving-bug-GROOVY-5169.patch [ 62444 ]
        Attachment MetaClassImpl.java [ 62445 ]
        Hide
        blackdrag blackdrag added a comment -

        Part of the patch is puzzling me before we had (!setter && !getter) to filter out properties that are in fact fields and thus have neither getter or setter. What is kept are read- or write-only properties. With your (!setter || !getter) those are removed as well. I can understand from the JSON perspective, but for getProperties this looks wrong.

        Next is the element.getModifiers() != 1, which seems to be there to filter out all non public fields. Since we filtered out all fields before already, thus is surely ok in principle, but you don't do something like that for MetaBeanProperties. They can be protected or package private as well imho. Why should getProperties show a protected MetaBeanProperty, but not a protected CachedField?

        My Conclusion:
        Changing getProperties to return many more is a potentially breaking change, so it surely wouldn't go into 1.8.x or 2.0.x. I think for 2.1.x it is too late. We have to investigate though how that method is used before we can say that it goes into 2.2, otherwise the next possible version would be 3.0 only. I would suggest finding another way to tackle the problem and keep the changes to the json classes itself if possible.

        Show
        blackdrag blackdrag added a comment - Part of the patch is puzzling me before we had (!setter && !getter) to filter out properties that are in fact fields and thus have neither getter or setter. What is kept are read- or write-only properties. With your (!setter || !getter) those are removed as well. I can understand from the JSON perspective, but for getProperties this looks wrong. Next is the element.getModifiers() != 1, which seems to be there to filter out all non public fields. Since we filtered out all fields before already, thus is surely ok in principle, but you don't do something like that for MetaBeanProperties. They can be protected or package private as well imho. Why should getProperties show a protected MetaBeanProperty, but not a protected CachedField? My Conclusion: Changing getProperties to return many more is a potentially breaking change, so it surely wouldn't go into 1.8.x or 2.0.x. I think for 2.1.x it is too late. We have to investigate though how that method is used before we can say that it goes into 2.2, otherwise the next possible version would be 3.0 only. I would suggest finding another way to tackle the problem and keep the changes to the json classes itself if possible.
        Hide
        umang bhatt added a comment - - edited

        ok.
        1. I would try to make changes in json related class, should it be fine ?
        2. Do i need to change the behavior(output) in any way ? Or this is fine ?

        Show
        umang bhatt added a comment - - edited ok. 1. I would try to make changes in json related class, should it be fine ? 2. Do i need to change the behavior(output) in any way ? Or this is fine ?
        Hide
        Guillaume Laforge added a comment -

        First of all, before doing any further change to your patch, let's agree on what should be serialized or not.

        • public properties: Yes (ie. both final and non final)
        • public fields: Yes (both final and non final)
        • static properties, static fields, whether public, protected or private: No
        Show
        Guillaume Laforge added a comment - First of all, before doing any further change to your patch, let's agree on what should be serialized or not. public properties: Yes (ie. both final and non final) public fields: Yes (both final and non final) static properties, static fields, whether public, protected or private: No
        Hide
        umang bhatt added a comment -

        here is a new patch that does the same thing by adding a method in JsonOutput class

        Show
        umang bhatt added a comment - here is a new patch that does the same thing by adding a method in JsonOutput class
        umang bhatt made changes -
        Attachment 0001-solving-GROOVY-5169.patch [ 62446 ]
        umang bhatt made changes -
        Attachment 0001-solving-bug-GROOVY-5169.patch [ 62444 ]
        umang bhatt made changes -
        Attachment 0001-solving-GROOVY-5169.patch [ 62446 ]
        umang bhatt made changes -
        Attachment 0001-bugfix.patch [ 62447 ]
        Hide
        blackdrag blackdrag added a comment -

        hmm... CachedField is only an internal representation for fields. That means the patch should include only fields that are public. But the usual property is one with getter and setter and private field. In the json_test.groovy attached above this would imho mean that Bar#bar is excluded since it is a property backed by a private field.

        Also there is the question to answer if a property that consists only of a getter or setter and no field should be included. And dynamically added properties are up to discussion as well I think.

        Show
        blackdrag blackdrag added a comment - hmm... CachedField is only an internal representation for fields. That means the patch should include only fields that are public. But the usual property is one with getter and setter and private field. In the json_test.groovy attached above this would imho mean that Bar#bar is excluded since it is a property backed by a private field. Also there is the question to answer if a property that consists only of a getter or setter and no field should be included. And dynamically added properties are up to discussion as well I think.
        Hide
        James Sumners added a comment -

        "Also there is the question to answer if a property that consists only of a getter or setter and no field should be included. And dynamically added properties are up to discussion as well I think."

        When I originally filed the bug, and this still hold true for me, I assumed that the instance of the object would be serialized in such a fashion that it could be recreated from the JSON. That would imply dynamically added properties should be included in the serialization. Otherwise, what are you serializing?

        Show
        James Sumners added a comment - "Also there is the question to answer if a property that consists only of a getter or setter and no field should be included. And dynamically added properties are up to discussion as well I think." When I originally filed the bug, and this still hold true for me, I assumed that the instance of the object would be serialized in such a fashion that it could be recreated from the JSON. That would imply dynamically added properties should be included in the serialization. Otherwise, what are you serializing?
        Hide
        blackdrag blackdrag added a comment -

        but if you want this to recreate the object afterwards, then read-only-properties are going to be a problem for you - plus dynamically added properties will only be available if they have been added on the deserialization side as well.

        Show
        blackdrag blackdrag added a comment - but if you want this to recreate the object afterwards, then read-only-properties are going to be a problem for you - plus dynamically added properties will only be available if they have been added on the deserialization side as well.
        Hide
        James Sumners added a comment -

        Well, since Groovy doesn't deserialize JSON to an object anyway (that I can tell) that's a moot point. It would be left up to the deserializer as to how to handle that.

        Show
        James Sumners added a comment - Well, since Groovy doesn't deserialize JSON to an object anyway (that I can tell) that's a moot point. It would be left up to the deserializer as to how to handle that.
        Hide
        James Sumners added a comment -

        By the way, back when I filed this bug I ended up resorting to some ridiculousness to get around it – http://stackoverflow.com/questions/7853809/iterating-an-objects-own-properties-in-groovy

        Show
        James Sumners added a comment - By the way, back when I filed this bug I ended up resorting to some ridiculousness to get around it – http://stackoverflow.com/questions/7853809/iterating-an-objects-own-properties-in-groovy

          People

          • Assignee:
            Unassigned
            Reporter:
            James Sumners
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated: