Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with mergeTo and optional attribute #38

Closed
kkasila opened this issue Nov 5, 2015 · 7 comments
Closed

Issue with mergeTo and optional attribute #38

kkasila opened this issue Nov 5, 2015 · 7 comments
Assignees
Labels
Milestone

Comments

@kkasila
Copy link

kkasila commented Nov 5, 2015

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.

@kkasila kkasila changed the title Issue with mergeTo and optional Issue with mergeTo and optional attribute Nov 5, 2015
@highsource highsource added the bug label Nov 22, 2015
@highsource highsource added this to the 0.11.x milestone Nov 22, 2015
@highsource highsource self-assigned this Nov 22, 2015
@highsource
Copy link
Owner

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

@highsource
Copy link
Owner

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

@highsource
Copy link
Owner

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:

  • The startegy now has a new shouldBeSet method which returns TRUE, FALSE or null if the field should be set, should be unset or should be left as is.
  • If the field should be set, the the merge method of the strategy will be invoked. And not just with values but also with boolean args signifying whether values were set. So you can get non-null value (default value) with false "isSet" there.
  • New merge methods do mostly the following:
    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.

highsource added a commit that referenced this issue Dec 23, 2015
highsource added a commit that referenced this issue Dec 23, 2015
@kkasila
Copy link
Author

kkasila commented Dec 28, 2015

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
Copy link
Owner

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
Copy link
Author

kkasila commented Dec 29, 2015

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
Copy link
Owner

Thank you for the confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants