REF: Ignore non existing fields #789

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@jdillon
Contributor

jdillon commented Mar 26, 2013

Some details about how we might be able to change xstream to handle missing fields.

@@ -33,7 +34,21 @@
*/
public XStream createForXml()
{
- final XStream xstream = new XStream( new LookAheadXppDriver() );
+ final XStream xstream = new XStream(new LookAheadXppDriver())

This comment has been minimized.

@cstamas

cstamas Mar 27, 2013

Contributor

Unsure we want to go this path. This snippet flows on the net for very long time, and is part of XStream acceptance tests, but it has drawbacks too.

Joerg describes on XStream user list the drawbacks on this approach (makes impossible use of implicit collections with XStream for example).
See related issues:
http://jira.codehaus.org/browse/XSTR-30
http://jira.codehaus.org/browse/XSTR-691

I'm rather to go with "compatible changes" on DTOs, as described in XStream FAQ. If not possible, introduce new resource with new DTOs instead of reworking old ones (especially if DTO use is shared across multiple resources).

@cstamas

cstamas Mar 27, 2013

Contributor

Unsure we want to go this path. This snippet flows on the net for very long time, and is part of XStream acceptance tests, but it has drawbacks too.

Joerg describes on XStream user list the drawbacks on this approach (makes impossible use of implicit collections with XStream for example).
See related issues:
http://jira.codehaus.org/browse/XSTR-30
http://jira.codehaus.org/browse/XSTR-691

I'm rather to go with "compatible changes" on DTOs, as described in XStream FAQ. If not possible, introduce new resource with new DTOs instead of reworking old ones (especially if DTO use is shared across multiple resources).

This comment has been minimized.

@jdillon

jdillon Mar 27, 2013

Contributor

/me shrugs

We need to do something... we can't simply collect transient deprecated fields for years, nor can we create new resources each time a feature change requires model changes.

@jdillon

jdillon Mar 27, 2013

Contributor

/me shrugs

We need to do something... we can't simply collect transient deprecated fields for years, nor can we create new resources each time a feature change requires model changes.

@jdillon

This comment has been minimized.

Show comment
Hide comment
@jdillon

jdillon Mar 29, 2013

Contributor

I spent a little time today and looked into this more. Its really too bad xstream is implemented this way, as it makes it very difficult to configure this behavior w/o changing the source.

I pulled down the latest trunk (1.4.5-SNAPSHOT) and with this change:

Index: src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java
===================================================================
--- src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (revision 2058)
+++ src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (working copy)
@@ -455,7 +455,10 @@
                 }
             }
         }
-        throw new UnknownFieldException(resultType.getName(), fieldName);
+
+        // HACK: Always ignore unknown fields
+        System.out.println("Ingoing unknown field: " + resultType.getName() + " " + fieldName);
+        // throw new UnknownFieldException(resultType.getName(), fieldName);
     }

     private Map writeValueToImplicitCollection(UnmarshallingContext context, Object value,

With simple nested group of users:

    @XStreamAlias("user")
    public static class User
    {
        String id;

        String name;

        @Override
        public String toString() {
            return getClass().getSimpleName() + "{" +
                "id='" + id + '\'' +
                ", name='" + name + '\'' +
                '}';
        }
    }

    @XStreamAlias("group")
    public static class Group
    {
        String id;

        @XStreamImplicit
        List<User> users = new ArrayList<User>();

        @Override
        public String toString() {
            return getClass().getSimpleName() + "{" +
                "id='" + id + '\'' +
                ", users=" + users +
                '}';
        }
    }

I can unmarshal this data (which as an invalid foo element):

<group>
  <id>minions</id>
  <user>
    <id>joe</id>
    <name>Joe Blow</name>
  </user>
  <foo>bar</foo>
  <user>
    <id>steve</id>
    <name>Stevie Wunderbar</name>
  </user>
</group>

which produces:

Ingoing unknown field: org.sonatype.nexus.sandbox.XStreamTrial$Group foo
Group{id='minions', users=[User{id='joe', name='Joe Blow'}, User{id='steve', name='Stevie Wunderbar'}]}

The new handleUnknownField() is new, not sure when it was added but its not in 1.4.3 (or 1.4.4) so this must be new work they have done on trunk. Perhaps this means they are actually looking to fix this in a proper release and allow this behavior to be changed w/o hacking the codebase.

To patch 1.4.3 (or 1.4.4) we'd have to have a few more lines around the problematic exception throwing code to eat the exception and then move to the next element.

I am not sure what other side-effects this change has however.

Contributor

jdillon commented Mar 29, 2013

I spent a little time today and looked into this more. Its really too bad xstream is implemented this way, as it makes it very difficult to configure this behavior w/o changing the source.

I pulled down the latest trunk (1.4.5-SNAPSHOT) and with this change:

Index: src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java
===================================================================
--- src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (revision 2058)
+++ src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (working copy)
@@ -455,7 +455,10 @@
                 }
             }
         }
-        throw new UnknownFieldException(resultType.getName(), fieldName);
+
+        // HACK: Always ignore unknown fields
+        System.out.println("Ingoing unknown field: " + resultType.getName() + " " + fieldName);
+        // throw new UnknownFieldException(resultType.getName(), fieldName);
     }

     private Map writeValueToImplicitCollection(UnmarshallingContext context, Object value,

With simple nested group of users:

    @XStreamAlias("user")
    public static class User
    {
        String id;

        String name;

        @Override
        public String toString() {
            return getClass().getSimpleName() + "{" +
                "id='" + id + '\'' +
                ", name='" + name + '\'' +
                '}';
        }
    }

    @XStreamAlias("group")
    public static class Group
    {
        String id;

        @XStreamImplicit
        List<User> users = new ArrayList<User>();

        @Override
        public String toString() {
            return getClass().getSimpleName() + "{" +
                "id='" + id + '\'' +
                ", users=" + users +
                '}';
        }
    }

I can unmarshal this data (which as an invalid foo element):

<group>
  <id>minions</id>
  <user>
    <id>joe</id>
    <name>Joe Blow</name>
  </user>
  <foo>bar</foo>
  <user>
    <id>steve</id>
    <name>Stevie Wunderbar</name>
  </user>
</group>

which produces:

Ingoing unknown field: org.sonatype.nexus.sandbox.XStreamTrial$Group foo
Group{id='minions', users=[User{id='joe', name='Joe Blow'}, User{id='steve', name='Stevie Wunderbar'}]}

The new handleUnknownField() is new, not sure when it was added but its not in 1.4.3 (or 1.4.4) so this must be new work they have done on trunk. Perhaps this means they are actually looking to fix this in a proper release and allow this behavior to be changed w/o hacking the codebase.

To patch 1.4.3 (or 1.4.4) we'd have to have a few more lines around the problematic exception throwing code to eat the exception and then move to the next element.

I am not sure what other side-effects this change has however.

@jdillon

This comment has been minimized.

Show comment
Hide comment
@jdillon

jdillon Mar 29, 2013

Contributor

Diff for 1.4.4 change looks like this:

Index: src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java
===================================================================
--- src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (revision 2058)
+++ src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (working copy)
@@ -291,10 +291,20 @@
                 .getImplicitCollectionDefForFieldName(fieldDeclaringClass, fieldName);
             boolean fieldExistsInClass = implicitCollectionMapping == null
                 && reflectionProvider.fieldDefinedInClass(fieldName, fieldDeclaringClass);
-            Class type = implicitCollectionMapping == null || implicitCollectionMapping.getItemType() == null
-                ? determineType(
-                    reader, fieldExistsInClass, result, fieldName, classDefiningField)
-                : implicitCollectionMapping.getItemType();
+
+            Class type;
+            try {
+                type = implicitCollectionMapping == null || implicitCollectionMapping.getItemType() == null
+                    ? determineType(
+                        reader, fieldExistsInClass, result, fieldName, classDefiningField)
+                    : implicitCollectionMapping.getItemType();
+            }
+            catch (UnknownFieldException e) {
+                System.out.println("Ignoring unknown field: " + fieldDeclaringClass.getName() + " " + fieldName);
+                reader.moveUp();
+                continue;
+            }
+
             final Object value;
             if (fieldExistsInClass) {
                 Field field = reflectionProvider.getField(classDefiningField != null ? classDefiningField : result.getClass(), fieldName);
Contributor

jdillon commented Mar 29, 2013

Diff for 1.4.4 change looks like this:

Index: src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java
===================================================================
--- src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (revision 2058)
+++ src/java/com/thoughtworks/xstream/converters/reflection/AbstractReflectionConverter.java    (working copy)
@@ -291,10 +291,20 @@
                 .getImplicitCollectionDefForFieldName(fieldDeclaringClass, fieldName);
             boolean fieldExistsInClass = implicitCollectionMapping == null
                 && reflectionProvider.fieldDefinedInClass(fieldName, fieldDeclaringClass);
-            Class type = implicitCollectionMapping == null || implicitCollectionMapping.getItemType() == null
-                ? determineType(
-                    reader, fieldExistsInClass, result, fieldName, classDefiningField)
-                : implicitCollectionMapping.getItemType();
+
+            Class type;
+            try {
+                type = implicitCollectionMapping == null || implicitCollectionMapping.getItemType() == null
+                    ? determineType(
+                        reader, fieldExistsInClass, result, fieldName, classDefiningField)
+                    : implicitCollectionMapping.getItemType();
+            }
+            catch (UnknownFieldException e) {
+                System.out.println("Ignoring unknown field: " + fieldDeclaringClass.getName() + " " + fieldName);
+                reader.moveUp();
+                continue;
+            }
+
             final Object value;
             if (fieldExistsInClass) {
                 Field field = reflectionProvider.getField(classDefiningField != null ? classDefiningField : result.getClass(), fieldName);
@jdillon

This comment has been minimized.

Show comment
Hide comment
@jdillon

jdillon Mar 29, 2013

Contributor

Note this doesn't handle missing classes though, which would have to be handled separately, though we might be able to leverage some of the techniques used by hudson here.

For reference:

Contributor

jdillon commented Mar 29, 2013

Note this doesn't handle missing classes though, which would have to be handled separately, though we might be able to leverage some of the techniques used by hudson here.

For reference:

@jdillon

This comment has been minimized.

Show comment
Hide comment
@jdillon

jdillon Mar 29, 2013

Contributor

Doh, I did not even notice this is already fixed:

http://jira.codehaus.org/browse/XSTR-691?focusedCommentId=322111&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-322111

Appears that next release will have support to ignore unknown elements... yay.

Contributor

jdillon commented Mar 29, 2013

Doh, I did not even notice this is already fixed:

http://jira.codehaus.org/browse/XSTR-691?focusedCommentId=322111&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-322111

Appears that next release will have support to ignore unknown elements... yay.

@jdillon

This comment has been minimized.

Show comment
Hide comment
@jdillon

jdillon Apr 5, 2013

Contributor

closing, we'll need/want to consume latest xstream once its released to fix this... hopefully that will be some time soon, else we may be forced to use a custom sonatype version here.

Contributor

jdillon commented Apr 5, 2013

closing, we'll need/want to consume latest xstream once its released to fix this... hopefully that will be some time soon, else we may be forced to use a custom sonatype version here.

@jdillon jdillon closed this Apr 5, 2013

@jdillon jdillon deleted the ignore-non-existing-fields branch Apr 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment