Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

REF: Ignore non existing fields #789

Closed
wants to merge 2 commits into from

2 participants

@jdillon
Owner

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

@cstamas cstamas commented on the diff
...e/nexus/client/internal/rest/NexusXStreamFactory.java
@@ -33,7 +34,21 @@
*/
public XStream createForXml()
{
- final XStream xstream = new XStream( new LookAheadXppDriver() );
+ final XStream xstream = new XStream(new LookAheadXppDriver())
@cstamas Owner
cstamas added a note

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

@jdillon Owner
jdillon added a note

/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.

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

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
Owner

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
Owner

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
Owner

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
Owner

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
@jdillon jdillon deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
17 nexus-client-core/src/main/java/org/sonatype/nexus/client/internal/rest/NexusXStreamFactory.java
@@ -12,6 +12,7 @@
*/
package org.sonatype.nexus.client.internal.rest;
+import com.thoughtworks.xstream.mapper.MapperWrapper;
import org.sonatype.nexus.client.internal.msg.ErrorMessage;
import org.sonatype.nexus.client.internal.msg.ErrorResponse;
import org.sonatype.nexus.rest.model.XStreamConfiguratorLightweight;
@@ -33,7 +34,21 @@
*/
public XStream createForXml()
{
- final XStream xstream = new XStream( new LookAheadXppDriver() );
+ final XStream xstream = new XStream(new LookAheadXppDriver())
@cstamas Owner
cstamas added a note

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

@jdillon Owner
jdillon added a note

/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ // Ignore fields which are present in the payload but not on target class
+ @Override
+ protected MapperWrapper wrapMapper(final MapperWrapper next) {
+ return new MapperWrapper(next)
+ {
+ @Override
+ public boolean shouldSerializeMember(final Class definedIn, final String fieldName) {
+ return definedIn != Object.class && super.shouldSerializeMember(definedIn, fieldName);
+ }
+ };
+ }
+ };
+
xstream.setMode( XStream.NO_REFERENCES );
xstream.autodetectAnnotations( false );
return xstream;
View
2  nexus-webapp/src/main/webapp/js/Nexus/util/IconContainer.js
@@ -104,7 +104,7 @@ NX.define('Nexus.util.IconContainer', {
* Reference to installed stylesheet.
*
* @private
- * @type {Stylesheet}
+ * @type {CSSStyleSheet}
*/
stylesheet: undefined,
Something went wrong with that request. Please try again.