Duplicate property check does not take MapperFeature.USE_WRAPPER_NAME_AS_PROPERTY_NAME into account

Hi,

I'm trying to upgrade to Jackson 2 using jackson-databind 2.3.0 and discovered the following incompatible change. I configured

mapper.configure(MapperFeature.USE_WRAPPER_NAME_AS_PROPERTY_NAME, true);

And I have a class (generated from XML schema) with these attributes:

    @XmlElementWrapper(name = "head")
    @XmlElement(name = "column")
    protected List<ColumnMeta> head = new ArrayList<ColumnMeta>();
    @XmlElementWrapper(name = "table")
    @XmlElement(name = "column")
    protected List<ColumnMeta> table = new ArrayList<ColumnMeta>();

The check for duplicate fields does not use the renamed properties and an error occurs during serialization: Multiple fields representing property "column": ...

In fact the check takes place during the renaming in

com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector._renameWithWrappers()

when

POJOPropertyBuilder.getPrimaryMember()

is called (line 773)

I think this should be file under XML module (https://github.com/FasterXML/jackson-dataformat-xml/issues), unless it can be reproduced with databind. This is a known issue, although I wasn't aware that it was working with 1.9.

I agree in that this is an unfortunate and nasty problem and should be fixed.

Could you re-create it at jackson-dataformat-xml and close this one? I wish github had a way to transfer issues, but don't think there is one.

Since I'm not using jackson-dataformat-xml and I'm serializing to JSON format the issue seems to be in the correct place here, isn't it? The only jackson JARs my dependency management pulls in are:

jackson-annotations-2.3.0.jar jackson-core-2.3.0.jar jackson-databind-2.3.0.jar jackson-jaxrs-base-2.3.0.jar jackson-jaxrs-json-provider-2.3.0.jar jackson-module-jaxb-annotations-2.3.0.jar

Hmmh. Good point, you are right -- I forgot that although wrappers are only added for xml output, using wrapper names as property names does affect all output.

So never mind wrt move. And thank you again for reporting this.

This issue really inhibits upgrade to Jackson 2. Just a side note - it was first reported 7 months ago in https://github.com/FasterXML/jackson-module-jaxb-annotations/issues/22 somewhat hidden in comments. The workaround suggested there does not work since it is not always possible to change annotations on old POJOs for compatibility reasons.

Someone needs to work on it, and it is not a trivial thing to fix. I hope someone has time & itch to work on this: Jackson is a pure OSS project with no paid contributors.

FWIW it should be possible to use mix-in annotations for unmodifiable types.

@cowtowncoder I had to apply the https://github.com/FasterXML/jackson-module-jaxb-annotations/issues/22 patch because I have to deal with POJOs which can't have direct dependencies to Jackson annotations.

What if original methods of AnnotationIntrospector were kept (possibly marked as deprecated) and new ones in the patch would merely delegate to them? BasicBeanDeserializer and JacksonAnnotationIntrospector (plus TestJaxbAnnotationIntrospector) could still refer to the original methods thus only classes directly involved would be re-factored.

I wasn't 100% certain what you meant by design constraints because other methods there already utilize the MapperConfig but this could be the way you were looking for if you merely emphasized that backwards compatibility must be preserved so that any derivative work doesn't need to be recompiled due to the fix.

@cowtowncoder Instead what I proposed above, I'm now trying to solve the issues by neither using USE_WRAPPER_NAME_AS_PROPERTY_NAME nor the patch mentioned above, but instead simply renaming the wrapped properties using a custom PropertyNamingStrategy.

I stumbled upon #428 fix and had to make such patch that the logic is done in the strategy itself rather than in POJOPropertiesCollector.
What I then realized was that there was some preliminary renaming so that had to be patched as well to be handed over to the naming strategy.
Original POJOPropertyBuilder did property grouping as a side effect I patched that to be optionally avoided by focusing the grouping to be done only to the given explicit names.
Sorting in POJOPropertiesCollector loses duplicate explicit names so I had to patch that also by sorting according to what was returned from strategy.


PropertyNamingStrategy

    public Map<BeanPropertyDefinition, String> namesForProperty(MapperConfig<?> config, BeanPropertyDefinition prop,
            boolean forSerialization)
    {
        Map<BeanPropertyDefinition, String> renames;
        Set<PropertyName> explicitNames = prop.findExplicitNames();
        // As per [#428](https://github.com/FasterXML/jackson-databind/issues/428) need
        // to skip renaming if property has explicitly defined name
        if (explicitNames.isEmpty()) {
            PropertyName fullName = prop.getFullName();
            renames = Collections.singletonMap(prop, fullName.getSimpleName());
        } else {
            Collection<? extends BeanPropertyDefinition> explicitProps = explicitNames.size() == 1 ? Collections.singleton(prop): prop.explode(explicitNames);
            renames = new LinkedHashMap<BeanPropertyDefinition, String>(explicitProps.size());
            for (BeanPropertyDefinition explicitProp : explicitProps)
            {
                PropertyName fullName = explicitProp.getFullName();
                String rename = null;
                if (forSerialization) {
                    if (explicitProp.hasGetter()) {
                        rename = nameForGetterMethod(config, explicitProp.getGetter(), fullName.getSimpleName());
                    } else if (explicitProp.hasField()) {
                        rename = nameForField(config, explicitProp.getField(), fullName.getSimpleName());
                    }
                } else {
                    if (explicitProp.hasSetter()) {
                        rename = nameForSetterMethod(config, explicitProp.getSetter(), fullName.getSimpleName());
                    } else if (explicitProp.hasConstructorParameter()) {
                        rename = nameForConstructorParameter(config, explicitProp.getConstructorParameter(), fullName.getSimpleName());
                    } else if (explicitProp.hasField()) {
                        rename = nameForField(config, explicitProp.getField(), fullName.getSimpleName());
                    } else if (explicitProp.hasGetter()) {
                        /* Plus, when getter-as-setter is used, need to convert that too..
                         * (should we verify that's enabled? For now, assume it's ok always)
                         */
                        rename = nameForGetterMethod(config, explicitProp.getGetter(), fullName.getSimpleName());
                    }
                }
                if (rename != null && !fullName.hasSimpleName(rename)) {
                    explicitProp = explicitProp.withSimpleName(rename);
                }
                renames.put(explicitProp, rename);
            }
        }
        return renames;
    }

BeanPropertyDefinition

    public Set<PropertyName> findExplicitNames() {
        return this.isExplicitlyNamed() ? Collections.singleton(this.getFullName()) : Collections.<PropertyName>emptySet();
    }

    public Collection<? extends BeanPropertyDefinition> explode(Collection<PropertyName> explicitNames) {
        return Collections.singleton(this);
    }

POJOPropertiesCollector

    protected void collectAll()
    {
        ...
        // And use custom naming strategy, if applicable...
        PropertyNamingStrategy naming = _findNamingStrategy();
        if (naming != null) {
            _renameUsing(naming);
        } else {
            // Rename remaining properties
            _renameProperties();
        }
        ...
    }

    protected void _renameUsing(PropertyNamingStrategy naming)
    {
        POJOPropertyBuilder[] props = _properties.values().toArray(new POJOPropertyBuilder[_properties.size()]);
        _properties.clear();
        for (POJOPropertyBuilder prop : props) {
            Map<BeanPropertyDefinition, String> renames = naming.namesForProperty(_config, prop, _forSerialization);
            for (Map.Entry<BeanPropertyDefinition, String> entry : renames.entrySet())
            {
                POJOPropertyBuilder newProp = (POJOPropertyBuilder) entry.getKey();
                final String simpleName = entry.getValue();
                /* As per [JACKSON-687], need to consider case where there may already be
                 * something in there...
                 */
                POJOPropertyBuilder old = _properties.get(simpleName);
                if (old == null) {
                    _properties.put(simpleName, newProp);
                } else {
                    old.addAll(newProp);
                }
                // replace the creatorProperty too, if there is one
                _updateCreatorProperty(prop, _creatorProperties);
            }
        }
    }

    protected void _sortProperties(Map<String, POJOPropertyBuilder> props)
    {
        // Then how about explicit ordering?
        AnnotationIntrospector intr = _annotationIntrospector;
        boolean sort;
        Boolean alpha = (intr == null) ? null : intr.findSerializationSortAlphabetically((Annotated) _classDef);

        if (alpha == null) {
            sort = _config.shouldSortPropertiesAlphabetically();
        } else {
            sort = alpha.booleanValue();
        }
        String[] propertyOrder = (intr == null) ? null : intr.findSerializationPropertyOrder(_classDef);

        // no sorting? no need to shuffle, then
        if (!sort && (_creatorProperties == null) && (propertyOrder == null)) {
            return;
        }
        int size = props.size();
        Map<String, POJOPropertyBuilder> all;
        // Need to (re)sort alphabetically?
        if (sort) {
            all = new TreeMap<String,POJOPropertyBuilder>(props);
        } else {
            all = props;
        }

        Map<String,POJOPropertyBuilder> ordered = new LinkedHashMap<String,POJOPropertyBuilder>(size);
        // Ok: primarily by explicit order
        if (propertyOrder != null) {
            for (String name : propertyOrder) {
                POJOPropertyBuilder w = all.get(name);
                if (w != null) {
                    ordered.put(name, w);
                }
            }
        }
        // And secondly by sorting Creator properties before other unordered properties
        if (_creatorProperties != null) {
            /* As per [Issue#311], this is bit delicate; but if alphabetic ordering
             * is mandated, at least ensure creator properties are in alphabetic
             * order. Related question of creator vs non-creator is punted for now,
             * so creator properties still fully predate non-creator ones.
             */
            Collection<POJOPropertyBuilder> cr;
            if (sort) {
                TreeMap<String, POJOPropertyBuilder> sorted =
                        new TreeMap<String,POJOPropertyBuilder>();
                for (POJOPropertyBuilder prop : _creatorProperties) {
                    sorted.put(prop.getName(), prop);
                }
                cr = sorted.values();
            } else {
                cr = _creatorProperties;
            }
            for (POJOPropertyBuilder prop : cr) {
                ordered.put(prop.getName(), prop);
            }
        }
        // And finally whatever is left (trying to put again will not change ordering)
        ordered.putAll(all);

        props.clear();
        props.putAll(ordered);
    }  

POJOPropertyBuilder

    @Override
    public Collection<POJOPropertyBuilder> explode(Collection<PropertyName> explicitNames)
    {
        Map<PropertyName,POJOPropertyBuilder> newNames = new LinkedHashMap<PropertyName,POJOPropertyBuilder>(explicitNames.size());
        Collection<POJOPropertyBuilder> props = explicitNames.isEmpty() ? new LinkedList<POJOPropertyBuilder>() : new ArrayList<POJOPropertyBuilder>(explicitNames.size());
        for (PropertyName name : explicitNames)
        {
            POJOPropertyBuilder prop = new POJOPropertyBuilder(_internalName, name, _annotationIntrospector, _forSerialization);
            props.add(prop);
            newNames.put(name, prop);
        }
        _explode(newNames, props, _fields);
        _explode(newNames, props, _getters);
        _explode(newNames, props, _setters);
        _explode(newNames, props, _ctorParameters);
        return props;
    }

    @SuppressWarnings("unchecked")
    private void _explode(Map<PropertyName, POJOPropertyBuilder> newNames,
            Collection<POJOPropertyBuilder> props,
            Linked<?> accessors)
    {
        final Linked<?> firstAcc = accessors; // clumsy, part 1
        for (Linked<?> node = accessors; node != null; node = node.next) {
            PropertyName name = node.name;
            if (!node.isNameExplicit || name == null) { // no explicit name -- problem!
                throw new IllegalStateException("Conflicting/ambiguous property name definitions (implicit name '"
                        +_name+"'): found multiple explicit names: "
                        +newNames+", but also implicit accessor: "+node);
            }
            POJOPropertyBuilder prop = newNames.get(name);
            if (prop == null) {
                prop = new POJOPropertyBuilder(_internalName, name, _annotationIntrospector, _forSerialization);
                props.add(prop);
            }
            // ultra-clumsy, part 2 -- lambdas would be nice here
            if (firstAcc == _fields) {
                Linked<AnnotatedField> n2 = (Linked<AnnotatedField>) node;
                prop._fields = n2.withNext(prop._fields);
            } else if (firstAcc == _getters) {
                Linked<AnnotatedMethod> n2 = (Linked<AnnotatedMethod>) node;
                prop._getters = n2.withNext(prop._getters);
            } else if (firstAcc == _setters) {
                Linked<AnnotatedMethod> n2 = (Linked<AnnotatedMethod>) node;
                prop._setters = n2.withNext(prop._setters);
            } else if (firstAcc == _ctorParameters) {
                Linked<AnnotatedParameter> n2 = (Linked<AnnotatedParameter>) node;
                prop._ctorParameters = n2.withNext(prop._ctorParameters);
            } else {
                throw new IllegalStateException("Internal error: mismatched accessors, property: "+this);
            }
        }
    }

In the end the whole renaming logic can be transferred over to naming strategy. Mine was simply to group properties using the original property name (a.k.a internal name) which involved skipping a lot of forced renames that I can now avoid as shown above. The approach should be backwards compatible both in run and compile time, but constructor sorting will still revolve around explicit names (#556).

private static class JaxbPropertyNamingStrategy extends PropertyNamingStrategy {

        @Override
        public Map<BeanPropertyDefinition, String> namesForProperty(
            MapperConfig<?> config, BeanPropertyDefinition prop, boolean forSerialization)
        {
            String internalName = prop.getInternalName();
            Map<BeanPropertyDefinition, String> props;
            if (prop.isExplicitlyNamed())
            {
                Collection<? extends BeanPropertyDefinition> explicitProps = prop.explode(Collections.<PropertyName> emptySet());
                props = new LinkedHashMap<BeanPropertyDefinition, String>(
                    explicitProps.size());
                for (BeanPropertyDefinition explicitProp : explicitProps)
                {
                    props.put(explicitProp, internalName);
                }
            }
            else
            {
                props = Collections.singletonMap(prop, internalName);
            }
            return props;
        }

}

@TuomasKiviaho conceptually it could be that renaming via naming strategy (or similar) could work. One concern in using existing mechanism is that only one NamingStrategy may be used, although it could be possible to chain strategies. On the other hand, just using NamingStrategy as API is not problematic, if there was a way to register sort of secondary renaming handler.

I guess the problem however is exactly that renaming should come before uniqueness constraints checks

As to group, naming; this is a tricky area because there are expectations to both group using implicit names (to reduce need for duplication annotations), and occasionally explicit, or combination thereof (especially due to constructor parameter names not necessarily being available). If change passes unit tests, it should be relatively safe, but I am bit worried since it is quite fragile construct all in all.

FWIW, I think that the fix will have to wait until 2.7, since there are RCs for 2.6. While unfortunate I think this is better to try to reduce risk of late changes before release. As soon as release is done we can branch off 2.6 maintenance branch and start working on this; and PRs can of course be created against master even earlier.

@cowtowncoder NP. I just documented as I went along with the NamingStrategy, because it seemed to keep the code changes concentrated inside the collect method, but even then the rabbit hole was to my surprise deep. I also though about nesting, but I guess that in the final form some new strategy that delegates to NamingStrategycould be a cleaner solution.

JAXB members can be sorted using XmlType.propOrder so keying the properties using using implicit names is also vital there. Currently the sorting algorithm seems to be a hacked compromise of the both explicit and implicit names ~~, but it worked for me.~~ and I had to ensure that duplicate explicit names do not cause properties to be neglected. Also I left out patching of _renameWithWrappers, because I wasn't depending on it anymore.

I appreciate your work and help here, esp. given the complexity and compromises of existing code! :) Reordering is indeed a compromise (there was a bug report to support using implicit names, and old code assumed explicit). JAXB usage did trigger quite a few changes, although compatibility is still incomplete.

Explicit naming doesn't take ambiguous choice (neither @XmlElements nor @XmlElementRefs with value.length <> 1) into account when dealing with @XmlElementWrapper. In this scenario the wrapper name is present while the explicit name isn't, but in this case the property name surely isn't implicit name.

JaxbAnnotationIntrospector

    private static PropertyName findJaxbPropertyName(Annotated ae, Class<?> aeType, String defaultName)
    {
        XmlAttribute attribute = ae.getAnnotation(XmlAttribute.class);
        if (attribute != null) {
            return _combineNames(attribute.name(), attribute.namespace(), defaultName);
        }
        XmlElement element = ae.getAnnotation(XmlElement.class);
        XmlElements elements = ae.getAnnotation(XmlElements.class);
        if (element == null && elements != null) {
            XmlElement[] value = elements.value();
            if (value.length == 1) {
                element = value[0];
            }
        }
        if (element != null) {
            return _combineNames(element.name(), element.namespace(), defaultName);
        }
        XmlElementRef elementRef = ae.getAnnotation(XmlElementRef.class);
        XmlElementRefs elementRefs = ae.getAnnotation(XmlElementRefs.class); 
        if (elementRef == null && elementRefs != null) {
            XmlElementRef[] value = elementRefs.value();
            if (value.length == 1) {
                elementRef = value[0];
            }
        }
        boolean hasAName = (elementRef != null);
        if (hasAName) {
            if (!MARKER_FOR_DEFAULT.equals(elementRef.name())) {
                return _combineNames(elementRef.name(), elementRef.namespace(), defaultName);
            }
            if (aeType != null) {
                XmlRootElement rootElement = (XmlRootElement) aeType.getAnnotation(XmlRootElement.class);
                if (rootElement != null) {
                    String name = rootElement.name();
                    if (!MARKER_FOR_DEFAULT.equals(name)) {
                        return _combineNames(name, rootElement.namespace(), defaultName);
                    }
                    // Is there a namespace there to use? Probably not?
                    return new PropertyName(Introspector.decapitalize(aeType.getSimpleName()));
                }
            }
        }
        if (!hasAName) {
            hasAName = ae.hasAnnotation(XmlElementWrapper.class);
            if (hasAName && (elements != null || elementRefs != null)) {
                return PropertyName.NO_NAME;
            }
        }
        // 09-Aug-2014, tatu: Note: prior to 2.4.2, we used to give explicit name "value"
        //   if there was "@XmlValue" annotation; since then, only implicit name.

        // One more thing: 
        return hasAName ? PropertyName.USE_DEFAULT : null;
    }

POJOPropertyBuilder.Linked

       public Linked(T v, Linked<T> n,
                PropertyName name, boolean explName, boolean visible, boolean ignored)
        {
            value = v;
            next = n;
            // ensure that we'll never have missing names
            this.name = (name == null || (name.isEmpty() && PropertyName.NO_NAME != name)) ? null : name;

            if (explName) {
                if (this.name == null) { // sanity check to catch internal problems
                    throw new IllegalArgumentException("Can not pass true for 'explName' if name is null/empty");
                }
                // 03-Apr-2014, tatu: But how about name-space only override?
                //   Probably should not be explicit? Or, need to merge somehow?
                if (!name.hasSimpleName() && PropertyName.NO_NAME != name) {
                    explName = false;
                }
            }

            isNameExplicit = explName;
            isVisible = visible;
            isMarkedIgnored = ignored;
        }

POJOPropertiesCollector
    protected void _addFields(Map<String, POJOPropertyBuilder> props)
    {
            ...
            if (nameExplicit && pn.isEmpty() && PropertyName.NO_NAME != pn) { // empty String meaning "use default name", here just means "same as field name"
                pn = _propNameFromSimple(implName);
                nameExplicit = false;
            }
            ...
    }

    protected void _addGetterMethod(Map<String, POJOPropertyBuilder> props,
            AnnotatedMethod m, AnnotationIntrospector ai)
    {
        ...
            if (pn.isEmpty() && PropertyName.NO_NAME != pn) {
                // !!! TODO: use PropertyName for implicit names too
                pn = _propNameFromSimple(implName);
                nameExplicit = false;
            }
            visible = true;
        }
        ...
    }

The ambiguity indicator is utilized on the XML (and perhaps JSON) PropertyWriter as well so that only wrapper name is written out, because name itself is unknown at this point.

@Override
    public void serializeAsField(Object bean, JsonGenerator gen, SerializerProvider prov) throws Exception
    {
        ...
        final SerializedString name = _name.charLength() > 0 ? _name : null;
        // Null handling is bit different, check that first
        if (value == null) {
            if (_nullSerializer != null) {
                if (_wrapperName != null) {
                    gen.writeFieldName(_wrapperName.getSimpleName());
                    if (name != null) {
                        gen.writeStartObject();
                    }
                }
                if (name != null) {
                    gen.writeFieldName(name);
                }
                _nullSerializer.serialize(null, gen, prov);
                if (_wrapperName != null && name != null) {
                    gen.writeEndObject();
                }
            }
            return;
        }
        ...
        if (_wrapperName != null) {
            gen.writeFieldName(_wrapperName.getSimpleName());
            if (name != null) {
                gen.writeStartObject();
            }
        }
        if (name != null) {
            gen.writeFieldName(_name);
        }
        if (_typeSerializer == null) {
            ser.serialize(value, gen, prov);
        } else {
            ser.serializeWithType(value, gen, prov, _typeSerializer);
        }
        if (_wrapperName != null && name != null) {
            gen.writeEndObject();
        }
    }

I stumbled upon #428 fix and had to make such patch that the logic is done in the strategy itself rather than in POJOPropertiesCollector.

What I then realized was that there was some preliminary renaming so that had to be patched as well to be handed over to the naming strategy.

Original POJOPropertyBuilder did property grouping as a side effect I patched that to be optionally avoided by focusing the grouping to be done only to the given explicit names.

Sorting in POJOPropertiesCollector loses duplicate explicit names so I had to patch that also by sorting according to what was returned from strategy. PropertyNamingStrategy

Upgrading from 2.6.0-rc2 to 2.9.0.pr2 and these ones could still as easily be fixed.

@TuomasKiviaho Is there an existing patch that still works?

One note on XML vs JSON (and others): only XML uses wrapper AND property name: other formats just use a single name, which may be configured by taken from wrapper or not. There is desire to allow use of wrappers in general (reverse of @JsonUnwrapped), but that would likely be added separately, not using existing facilities. So as far as I know, this is primarily XML issue.

FasterXML / jackson-databind

Duplicate property check does not take MapperFeature.USE_WRAPPER_NAME_AS_PROPERTY_NAME into account #360