Issue with boolean default values and mergeFrom, copyTo, equals

[kkasila]

I have a schema with a "testBoolean" attribute (type xs:boolean) with default value "true".

xs:attribute name="testBoolean" type="xs:boolean" default="true"

xjc will generate a "isTestBoolean" method for the testBoolean, which shows that internally the boolean can have a null value which is converted to the schema-defined true in the isTestBoolean method.

public boolean isTestBoolean() {
    if (testBoolean == null) {
        return true;
    } else {
        return testBoolean;
    }
}

Issue found is that the mergeFrom seems to convert undefined boolean values to "false" instead of the default value. The code line below shows that the generated code is directly checking the protected "testBoolean" field, instead of using "isTestBoolean()" method. It also converts a null value to "false".

lhsTestBoolean = ((leftObject.testBoolean!= null)?leftObject.isTestBoolean():false);

The result is that after running the mergeFrom function the booleans have an incorrect value "false" instead of staying undefined (null/true).

Tested with jaxb-basics 0.9.4 and 0.9.5.

There is similar code generated for many other methods as well (equals, copyTo ..) which I did not test.

[highsource]

Could you please PR a reproducing schema fragment to this schema?

https://github.com/highsource/jaxb2-basics/blob/master/tests/issues/src/main/resources/schema.xsd

[kkasila]

I was able to reproduce it by changing
<jaxb:globalBindings generateIsSetMethod="true"
to
<jaxb:globalBindings generateIsSetMethod="false"

[highsource]

This should be fixed now, could you please give it a try?

This is what I get now:

    public Object copyTo(ObjectLocator locator, Object target, CopyStrategy strategy) {
        final Object draftCopy = ((target == null)?createNewInstance():target);
        if (draftCopy instanceof IssueGH37Type) {
            final IssueGH37Type copy = ((IssueGH37Type) draftCopy);
            if (this.isSetTestBoolean()) {
                boolean sourceTestBoolean;
                sourceTestBoolean = (this.isSetTestBoolean()?this.isTestBoolean():true);
                boolean copyTestBoolean = strategy.copy(LocatorUtils.property(locator, "testBoolean", sourceTestBoolean), sourceTestBoolean);
                copy.setTestBoolean(copyTestBoolean);
            } else {
                copy.unsetTestBoolean();
            }
        }
        return draftCopy;
    }
    public void mergeFrom(ObjectLocator leftLocator, ObjectLocator rightLocator, Object left, Object right, MergeStrategy strategy) {
        if (right instanceof IssueGH37Type) {
            final IssueGH37Type target = this;
            final IssueGH37Type leftObject = ((IssueGH37Type) left);
            final IssueGH37Type rightObject = ((IssueGH37Type) right);
            {
                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, rhsTestBoolean));
                target.setTestBoolean(mergedTestBoolean);
            }
        }
    }

[kkasila]

I tested this on my project, where I have jaxb:globalBindings generateIsSetMethod="false".
I have a testBoolean in my schema with default value "true" and int TestA with default value "69".

I confirm the merge code is now generated differently, so if the boolean is null it is set to the default value. Like this (shortened):

lhsTestBoolean = ((leftObject.testBoolean!= null)?leftObject.isTestBoolean():true);
rhsTestBoolean = ((rightObject.testBoolean!= null)?rightObject.isTestBoolean():true);

Same generation applies also to the appendFields and copyTo code.

This does correct the reported fault, BUT it still has an issue that merging "null" with "null" equals "defined default value". A correct result of merging null and null should be null.

The xjc generated "is" method checks for the null value and returns the schema-defined default value (while retaining the null). This is correct because the default value is given without modifying the original data. I think this is the case for all the types, not just boolean.

public boolean isTestBoolean() {
    if (testBoolean == null) {
        return true;
    } else {
        return testBoolean;
    }
}

public int getTestA() {
    if (testA == null) {
        return  69;
    } else {
        return testA;
    }
}

So there should not be any reason for the merge to override null with default value, because the "getter" takes care of that without altering the original data. IMO a correct merge code is same as code generated now when no default value is set in schema:

lhsTestBoolean = leftObject.isTestBoolean();
rhsTestBoolean = rightObject.isTestBoolean();

This will keep the original null, and the xjc-generated get/is method will handle the default value without tampering with the data.

There is same issue when merging other attributes with default value, like the same int
lhsTestA = ((leftObject.testA!= null)?leftObject.getTestA(): 69);
rhsTestA = ((rightObject.testA!= null)?rightObject.getTestA(): 69);
This will also override the "null" value with defined value 69.

This can lead to fault situation, when author of the document wants to leave things undefined so the receiver of the document can use the default value.

For example we have two systems in the network: system A and system B. System A is older and has schema v1.0 with int TestA default = "69". System B is newer and has schema v1.1 with int TestA default = "42".

Now user creates an original document D, and leaves the int TestA undefined ("let the system decide, since the system knows"). Now system A receives document D and stores a "copyTo" of the document (DA). With current implementation the TestA in DA would be set to system A default value "69" in the document DA. So original D has int TestA undefined ("let the system decide") and DA has int TestA "69" (system A default value).

But from system B point DA is broken because DA is not the original document, and it has an incorrect value from the System A schema v1.0 . When system A passes the stored document DA to system B, the "let system decide" has been broken and system B is forced to use the incorrect "69" set by system A.

So in summary, the original problem is corrected, but IMO the problem that "copyTo" and "mergeFrom" methods actually modify the original document is not correct.

[highsource]

Yes, this does not correct the merging, but that's another issue (#38),
right? Which is acknowledged as a bug and will be fixed.

I just mean you did not have to type all of this to persuade me. :)

You say that copyTo actually modifies the original document - could you
demonstrate, where it happens? If "isSet"-methods are generated, copyTo
methods don't call the actual getter if isSetProperty does not return true.

mergeFrom is different, there's an issue there. I just want to know if
there's also an issue in copyTo which I don't see at the moment.

#38 will be fixed by checking if values are set (via isSet methods). The
strategy will the deside if the new value should be set, unset or left
unchanged (TRUE, FALSE, null). If the strategy says "set", then the
peroperty will be set with the result of strategy.merge. If "unset" then
the "unset"-Method will be called. If "leave unchanged" then the property
in the target object will be left unchanged.

On Fri, Dec 18, 2015 at 12:35 PM, Kimmo Kasila [email protected]
wrote:

I tested this on my project, where I do not have .
I have a testBoolean in my schema with default value "true".

I confirm the merge code is now generated differently, so if the boolean
is null it is set to the default value. Like this (shortened):

lhsTestBoolean = ((leftObject.testBoolean!=
null)?leftObject.isTestBoolean():true);
rhsTestBoolean = ((rightObject.testBoolean!=
null)?rightObject.isTestBoolean():true);

Same generation applies also to the appendFields and copyTo code.

This does correct the reported fault, BUT it still has an issue that
merging "null" with "null" equals "defined default value". A correct result
of merging null and null should be null.

The xjc generated "is" method checks for the null value and returns the
schema-defined default value (while retaining the null). This is correct
because the default value is given without modifying the original data. I
think this is the case for all the types, not just boolean.

public boolean isTestBoolean() {
if (testBoolean == null) {
return true;
} else {
return testBoolean;
}
}

public int getTestA() {
if (testA == null) {
return 69;
} else {
return testA;
}
}

So there should not be any reason for the merge to override null with
default value, because the "getter" takes care of that without altering the
original data. IMO a correct merge is same as without default value:

lhsTestBoolean = leftObject.isTestBoolean();
rhsTestBoolean = rightObject.isTestBoolean();

This will keep the original null, and the xjc-generated get/is method will
handle the default value without tampering with the data.

There is same issue when merging other attributes with default value, like
the same int
lhsTestA = ((leftObject.testA!= null)?leftObject.getTestA(): 69);
rhsTestA = ((rightObject.testA!= null)?rightObject.getTestA(): 69);
This will also override the "null" value with defined value 69.

This can lead to fault situation, when author of the document wants to
leave things undefined so the receiver of the document can use the default
value.

For example we have two systems in the network: system A and system B.
System A is older and has schema v1.0 with int TestA default = "69". System
B is newer and has schema v1.1 with int TestA default = "42".

Now user creates an original document D, and leaves the int TestA
undefined ("let the system decide, since the system knows"). Now system A
receives document D and stores a "copyTo" of the document (DA). With
current implementation the TestA in DA would be set to system A default
value "69" in the document DA. So original D has int TestA undefined ("let
the system decide") and DA has int TestA "69" (system A default value).

But from system B point DA is broken because DA is not the original
document, and it has an incorrect value from the System A schema v1.0 .
When system A passes the stored document DA to system B, the "let system
decide" has been broken and system B is forced to use the incorrect "69"
set by system A.

So in summary, the original problem is corrected, but IMO the problem that
"copyTo" and "mergeFrom" methods actually modify the original document is
not correct.

—
Reply to this email directly or view it on GitHub
#37 (comment)
.