The setters generated for the array fields cause NPE if the passed in array was null

[glassfishrobot]

public void setNames(String[] values) {
      int len = values.length;
      this.names = ((String[]) new String[len] );
      for (int i = 0; (i<len); i ++) {
          this.names[i] = values[i];
      }
    }

Here's how XJC generates a setter if a collectionType of indexed was specified in the JAXB binding file. This throws an NPE if the values attribute was null. Especially for non-mandatory elements, this can very easily cause an NPE.

What's the reason for doing so? Does the JAXB spec require it this way? If not, perhaps changing the way the method is generated to include an appropriate null check can help avoid this? Maybe if it the passed in parameter was null, an empty array can be assigned to the field in question?

I looked at the JAXB spec and there is a mention for indexed fields that the argument should be discarded if it was null:

Page 58 of JAXB 2.2 spec

• setId(Type []) The array setter method defines the property’s set value. If the argument itself is null then the property’s set value, if any, is discarded. If the argument is not null and....

I think this behavior can be observed in all the versions of jaxb-xjc, but the one my CXF client uses is 2.2.10 the CXF codegen plugin version being 3.0.2.

I'm OK with creating a pull request to get this fixed, if that's fine.

Affected Versions

[2.2.10]

[glassfishrobot]

Reported by mystarrocks

[glassfishrobot]

mystarrocks said: Any updates on this one? As it stands, the collectionType="indexed" is simply unusable.

[glassfishrobot]

yaroska said: I have to check. Now it looks like we are violating 2 rulers: getter returns empty array for null value and setter doesn't accept null argument. So array value couldn't be reset.

Even more. What we have to return in case if value is null and we are asked about it's length???

[glassfishrobot]

mystarrocks said: I don't think there's a need to change the getters or the length operations in any way. The getters should return the value as is, i.e null if null or the array if it was set previously using the setter; while the length should either return the length of the array or throw an NPE depending on if it was set previously or not.

The only requirement here is to comply with the spec for the setters for array fields by not setting explicitly if a null was passed as an argument.

Like:

public void setNames(String[] values) {
      if (values == null) return; // simply ignore a null argument
      int len = values.length;
      this.names = ((String[]) new String[len] );
      for (int i = 0; (i<len); i ++) {
          this.names[i] = values[i];
      }
    }

[glassfishrobot]

yaroska said: I've reread specification more carefully (JAXB 2.2, section 5.5.2.1):

For getter: null should be returned instead of empty array in case if value wasn't set. Now user has no chance to determine if array was set without editing source class generated by XJC.

For setter: "If the argument itself is null then the property’s set value, if any, is discarded." -> previous value set to null.

[glassfishrobot]

mystarrocks said: I'm not quite sure about this statement:

Now user has no chance to determine if array was set without editing source class generated by XJC.

What's the rationale here? Why would the user want to know if the property was set or not? The user would merely call the getter to access the field whose value should be returned as:

• null if it was not set previously or
• the array if it was set previously.

Besides, the field will be null if it wasn't set anyway - b/c we don't initialize it with an array be default, do we?

Sorry if I'm missing something obvious, but I'm not understanding why this change to the setter will affect anything to the other invariants on the field. Can you please elaborate?

[glassfishrobot]

yaroska said: In my opinion, your proposition

if (values == null) return; // simply ignore a null argument

for setter, is against the specification. It's explicitly written that null argument for setter should reset already set value. So couldn't be ignored.

[glassfishrobot]

mystarrocks said: You're right,

if (values == null) return; // simply ignore a null argument

deviates from the spec. How about:

public void setNames(String[] values) {
      if (values == null) {
          this.names = null; // discard the previously set value on the field per spec by setting the field to be null
          return;
      }
      int len = values.length;
      this.names = ((String[]) new String[len] );
      for (int i = 0; (i<len); i ++) {
          this.names[i] = values[i];
      }
    }

This complies with the spec by discarding the previously set value if the argument was null and do the usual thing otherwise. What the clients really expect here is null safety, so whatever variant of the above code that can offer that should be good.

[glassfishrobot]

yaroska said: Thanks for reporting. Will be fixed.

[glassfishrobot]

Was assigned to yaroska

[glassfishrobot]

This issue was imported from java.net JIRA JAXB-1064