highsource / jaxb2-basics

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

Issue with mergeTo and optional attribute #38

Closed kkasila closed 8 years ago

kkasila commented 9 years ago

I have an element in my schema with an optional attribute "testAttribute" with type xs:positiveInteger. I am generating mergeTo code with -Xmergeable , and I have
<jaxb:globalBindings generateIsSetMethod="true">.

When I process XML files where the "testAttribute" is omitted, and use mergeTo method, the omitted (null) values change to defined 0 values (A defined value 0 is not allowed in my schema , while null or "not defined" is legal).

The reason is that the merge code calls the generated "isSet" method, but when the attribute is not set it uses a defined value "0" instead.

 int lhsTestAttribute;
 lhsTestAttribute = (leftObject.isSetTestAttribute()?leftObject.getTestAttribute(): 0);
 int rhsTestAttribute;
 rhsTestAttribute = (rightObject.isSetTestAttribute()?rightObject.getTestAttribute(): 0);

In case I switch to <jaxb:globalBindings generateIsSetMethod="false"> the issue disappears because the merge code is generated with "Integer" instead of "int" , which properly keeps the null value like this:

 Integer lhsTestAttribute;
 lhsTestAttribute= this.getTestAttribute();
 Integer rhsTestAttribute;
 rhsTestAttribute= that.getTestAttribute();

here the value is merged correctly as null.

highsource commented 8 years ago

Reproduced. The Mergeable plugin should consider if values were set here.

highsource commented 8 years ago

I think I'll treat isSetProperty getter as yet another property.

highsource commented 8 years ago

Hi @kkasila,

I've implemented the first draft of the solution.

It turns out to be more complicated than I thought. I really need a review/feedback on this - so I'd be extremely grateful if you'd give it a try and let me know what you think.

Ok, the problem is that with default values and isSet/unset we relly have to consider if the value was explicitly set vs. if there was some default value.

Here's a mergeFrom method generated at the moment:

    public void mergeFrom(ObjectLocator leftLocator, ObjectLocator rightLocator, Object left, Object right, MergeStrategy2 strategy) {
        if (right instanceof IssueGH37Type) {
            final IssueGH37Type target = this;
            final IssueGH37Type leftObject = ((IssueGH37Type) left);
            final IssueGH37Type rightObject = ((IssueGH37Type) right);
            {
                Boolean testBooleanShouldBeSet = strategy.shouldBeSet(leftLocator, rightLocator, leftObject.isSetTestBoolean(), rightObject.isSetTestBoolean());
                if (testBooleanShouldBeSet == Boolean.TRUE) {
                    boolean lhsTestBoolean;
                    lhsTestBoolean = (leftObject.isSetTestBoolean()?leftObject.isTestBoolean():true);
                    boolean rhsTestBoolean;
                    rhsTestBoolean = (rightObject.isSetTestBoolean()?rightObject.isTestBoolean():true);
                    boolean mergedTestBoolean = ((boolean) strategy.merge(LocatorUtils.property(leftLocator, "testBoolean", lhsTestBoolean), LocatorUtils.property(rightLocator, "testBoolean", rhsTestBoolean), lhsTestBoolean, leftObject.isSetTestBoolean(), rhsTestBoolean, rightObject.isSetTestBoolean()));
                    target.setTestBoolean(mergedTestBoolean);
                } else {
                    if (testBooleanShouldBeSet == Boolean.FALSE) {
                        target.unsetTestBoolean();
                    }
                }
            }
        }
    }

To sum up:

    public boolean merge(ObjectLocator leftLocator, ObjectLocator rightLocator,
            boolean left, boolean leftSet, boolean right, boolean rightSet) {
        if (leftSet && !rightSet) {
            return left;
        } else if (!leftSet && rightSet) {
            return right;
        } else {
            return merge(leftLocator, rightLocator, left, right);
        }
    }

If both values were set or both values were not set, then the old implementation will be invoked, otherwise the method will return the appropriate value.

This should allow to distinguish "not set + default value" vs. "set value" cases.

As far as I can see, changes in the runtime are backwards compatible. I've made new strategy (MergeStrategy2) extend the old strategy. So classes generated before should work with the new strategy as well.

Please give it a try and let me know what you think.

I think I'll have to rework other strategies as well.

kkasila commented 8 years ago

So classes generated before should work with the new strategy as well.

I have an episodic project, where a second schema complexType is extending a complexType from schema in first episode. In java xjc generation looks like "public class Extended extends Base"

In the copy, merge etc. code I get a call to merge/copy the Base first:

    super.copyTo(locator, draftCopy, strategy);

this now results to compile time error, because I did not re-generate Base episode:

"incompatible types: CopyStrategy2 cannot be converted to CopyStrategy"

since the previously generated episode expects CopyStrategy not CopyStrategy2 .

A cast would remove the compile error: "super.copyTo(locator, draftCopy, (CopyStrategy) strategy);" but since I have the possibility to re-generate also the base episode, I will proceed with that option.

highsource commented 8 years ago

What you've quoted means that JAXBCopyStrategy.INSTANCE can be passed to CopyTo as well as CopyTo2.

The situation you're describing seems like your Extended class is generated as CopyTo2 and Base as CopyTo. That won't work, that's correct. Regeneration is the best "workaround".

kkasila commented 8 years ago

I regenerated the Base, removed all workaround code related to this issue and #37 from my custom mergeStrategy. It seems to work now as expected. I get no incorrect values (from #37) or undefined-values-turning-to-defined (#38). I tested with both generateIsSetMethod="true" and generateIsSetMethod="false".

I am running combinations of mergeFrom, copyTo, clone and equals in test cases. Of course my input schemas and instance documents limit the coverage.

highsource commented 8 years ago

Thank you for the confirmation.