highsource / jaxb2-basics

Useful plugins and tools for JAXB2.
BSD 2-Clause "Simplified" License
108 stars 54 forks source link

mergeFrom appears to be doing a copyTo #127

Closed senderic closed 3 years ago

senderic commented 3 years ago

Issue

.mergeFrom seems to just do the same thing as .copyTo

Description

I have a need to merge two XML (jaxb based) objects and it seems like the .mergeFrom method derived from the JAXBMergeStrategy plugin. I successfully implemented the plugin via maven, however am I finding the usage of .mergeFrom to be odd.

The name of the object is Match so I will refer to the objects as MatchA, MatchB, and MatchBA. My goal is to merge MatchBA with MatchA to attain MatchB.

MatchA:
    <match>
        <ethernet-match>
            <ethernet-type>
                <type>2054</type>
            </ethernet-type>
            <ethernet-source>
                <address>aa:aa:aa:aa:aa:aa</address>
            </ethernet-source>
        </ethernet-match>
        <in-port>8</in-port>
    </match>

MatchBA:

    <match operation="none" orig-value="">
        <ethernet-match operation="none">
            <ethernet-type operation="none">
                <type operation="replace" orig-value="2054">2048</type>
            </ethernet-type>
        </ethernet-match>
        <ip-match operation="create">
            <ip-protocol operation="create">17</ip-protocol>
        </ip-match>
    </match>
Expected MatchB:
    <match operation="none" orig-value="">
        <ethernet-match operation="none">
            <ethernet-type operation="none">
                <type operation="replace" orig-value="2054">2048</type>
            </ethernet-type>
        </ethernet-match>
        <ip-match operation="create">
            <ip-protocol operation="create">17</ip-protocol>
        </ip-match>
        <ethernet-source>
            <address>aa:aa:aa:aa:aa:aa</address>
        </ethernet-source>
       <in-port>8</in-port>
    </match>
Or this could come out too:
    <match operation="none" orig-value="">
        <ethernet-match operation="none">
            <ethernet-type operation="none">
                <type operation="replace" orig-value="2054">2048</type>
            </ethernet-type>
            <ethernet-type>
                <type>2054</type>
            </ethernet-type>
        </ethernet-match>
        <ip-match operation="create">
            <ip-protocol operation="create">17</ip-protocol>
        </ip-match>
        <ethernet-source>
            <address>aa:aa:aa:aa:aa:aa</address>
        </ethernet-source>
       <in-port>8</in-port>
    </match>

In the second example, there are two <ethernet-type> elements since maybe JaxB can't merge that element understandably (I would need to manually prune though and detect an element like that since it won't have an operation or orig-value attribute).

What I actually get:

Match A = getMatchFor('A');
Match BA = getMatchFor('BA'); // these calls get valid JaxB XML Match Objects which implement MergeFrom2
Match B = new Match();
B.mergeFrom(A,BA);
    <match operation="none" orig-value="">
        <ethernet-match operation="none">
            <ethernet-type operation="none">
                <type operation="replace" orig-value="2054">2048</type>
            </ethernet-type>
        </ethernet-match>
        <ip-match operation="create">
            <ip-protocol operation="create">17</ip-protocol>
        </ip-match>
    </match>

That's not the expected merge. In fact, it looks just like BA. I also tried:

Match A = getMatchFor('A');
Match BA = getMatchFor('BA'); // these calls get valid JaxB XML Match Objects which implement MergeFrom2
Match B = new Match();
B.mergeFrom(BA,A);
    <match operation="none" orig-value="">
        <ethernet-match>
            <ethernet-type>
                <type>2054</type>
            </ethernet-type>
            <ethernet-source>
                <address>aa:aa:aa:aa:aa:aa</address>
            </ethernet-source>
        </ethernet-match>
        <in-port>8</in-port>
    </match>

Besides the top element having operation and orig-value attributes, the content of Match is the same as A. So, it seems like (not exactly but effectively) B.mergeFrom(BA,A) == A.copyTo(B) and B.mergeFrom(A,BA) == BA.copyTo(B).

Questions

Environment

Update

I think I found out why it appears to be more like a copy... https://github.com/highsource/jaxb2-basics/blob/a009a78971d8260daac09e1d5a2024bbd2e19128/runtime/src/main/java/org/jvnet/jaxb2_commons/lang/JAXBMergeStrategy.java#L25

senderic commented 3 years ago

Okay I realized I needed to implement my own MergeStrategy. By doing this, I got the merge to work the way I needed to. I'm not done yet, but I haven enough to show a bit:

package com.yyy.xxxx.data.model.strategies;

import com.yyy.xxxx.common.XxxxRuntimeException;
import com.yyy.xxxx.data.model.gen.Action;
import lombok.extern.log4j.Log4j2;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.ClassUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.reflect.MethodUtils;
import org.jvnet.jaxb2_commons.lang.JAXBMergeStrategy;
import org.jvnet.jaxb2_commons.locator.ObjectLocator;

import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Locale;
import java.util.function.Predicate;
import java.util.stream.Collectors;

@Log4j2
public class MergeFlowStrategy extends JAXBMergeStrategy {
    @Override
    protected Object mergeInternal(ObjectLocator leftLocator, ObjectLocator rightLocator, Collection leftCollection, Collection rightCollection) {
        log.debug("\n-leftCollection:\n{}\n-rightCollection:\n{}\n-leftLocator: {}\n-rightLocator: {}\n-left: {}\n-right: {}",
                () -> leftCollection.stream().map(Object::toString).collect(Collectors.joining("\n---\n")),
                () -> rightCollection.stream().map(Object::toString).collect(Collectors.joining("\n---\n")),
                () -> leftLocator, () -> rightLocator, () -> leftCollection, () -> rightCollection);

        // This is where the improper merge is taking place. Internally, all that is happening is a swap!
        // return super.mergeInternal(leftLocator, rightLocator, leftCollection, rightCollection);

        if (CollectionUtils.isEmpty(leftCollection) || CollectionUtils.isEmpty(rightCollection))
            return super.mergeInternal(leftLocator, rightLocator, leftCollection, rightCollection);
        else {
            Collection merge = new ArrayList<>();
            leftCollection.forEach(lEl -> mergeBasedOnOperation(leftLocator, rightLocator, rightCollection, merge, lEl));

            // Some elements may be in rightCollection and not in leftCollection. Make sure those elements survive.
            rightCollection.forEach(rEl -> {
                // no operation data will be found here.
                Predicate p = elem -> true;
                if (StringUtils.equals(ClassUtils.getSimpleName(Action.class), (ClassUtils.getSimpleName(rEl.getClass()))))
                    p = elem -> ((Action) elem).getOrder() == ((Action) rEl).getOrder();
                // TODO will need this for any array element that has a key (Action uses order as a key)

                Object lElL = leftCollection.stream().filter(rEl.getClass()::isInstance).map(rEl.getClass()::cast).filter(p).findFirst().orElse(null);
                if (lElL == null) merge.add(rEl);
            });
            return merge;
        }
    }

    private void mergeBasedOnOperation(ObjectLocator leftLocator, ObjectLocator rightLocator, Collection rightCollection, Collection merge, Object elem) {
        String operation = getOperation(elem);
        switch (operation.toLowerCase(Locale.US)) {
            case "none":
                Object rightElem = rightCollection.stream().filter(elem.getClass()::isInstance).map(elem.getClass()::cast).findFirst().orElse(null);
                Object m = super.merge(leftLocator, rightLocator, elem, rightElem);
                merge.add(m);
                break;
            case "replace":
            case "create": // if create, expect rightElem to be null.
                merge.add(elem);
                break;
            case "delete":
                break;
        }
    }

    private String getOperation(Object elem) {
        String operation;
        try {
            operation = String.valueOf(MethodUtils.getAccessibleMethod(elem.getClass(), "getOperation").invoke(elem));
        } catch (IllegalAccessException | InvocationTargetException e) {
            throw new XxxxRuntimeException("Could not get operation info out of element!", e);
        }
        return operation;
    }