[LUCENE-8460] Better argument validation in StoredField

• Type: Improvement
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 7.5, 8.0
• Component/s: core/other
• Labels: arguments/parameters javadocs
• Lucene Fields: New
• Assignee: Unassigned
• Reporter: Namgyu Kim
• Created: 20/Aug/18 17:35
• Updated: 28/Aug/22 15:34
• Resolved: 30/Aug/18 20:06

Description

I have found some invalid Javadocs in StoredField Class.
(and I think Field Class has some problems too )

 

1) Line 45 method

/**
 * Expert: allows you to customize the {@link
 * ...
 * @throws IllegalArgumentException if the field name is null.
 */
protected StoredField(String name, FieldType type) {
  super(name, type);
}

It is misleading because there is no explanation for type.
If you follow that super class, you can see the following code(Field class).

/**
 * Expert: creates a field with no initial value.
 * ...
 * @throws IllegalArgumentException if either the name or type
 *         is null.
 */
protected Field(String name, IndexableFieldType type) {
  if (name == null) {
    throw new IllegalArgumentException("name must not be null");
  }
  this.name = name;
  if (type == null) {
    throw new IllegalArgumentException("type must not be null");
  }
  this.type = type;
}

Field class has the exception handling(IllegalArgumentException) for null IndexableFieldType object.
For that reason, I changed the Javadoc to:

/**
 * Expert: allows you to customize the {@link
 * ...
 * @throws IllegalArgumentException if the field name or type
 *         is null.
 */
protected StoredField(String name, FieldType type) {
  super(name, type);
}

 

2) Line 59 method

/**
 * Expert: allows you to customize the {@link
 * ...
 * @throws IllegalArgumentException if the field name
 */
public StoredField(String name, BytesRef bytes, FieldType type) {
  super(name, bytes, type);
}

It is misleading because there is no explanation for bytes.
If you follow that super class, you can see the following code(Field class).

/**
 * Create field with binary value.
 *
 * ...
 * @throws IllegalArgumentException if the field name is null,
 *         or the field's type is indexed()
 * @throws NullPointerException if the type is null
 */
public Field(String name, BytesRef bytes, IndexableFieldType type) {
  if (name == null) {
    throw new IllegalArgumentException("name must not be null");
  }
  if (bytes == null) {
    throw new IllegalArgumentException("bytes must not be null");
  }
  this.fieldsData = bytes;
  this.type = type;
  this.name = name;
}

Field class has the exception handling(IllegalArgumentException) for null BytesRef object.
For that reason, I changed the Javadoc to:

/**
 * Expert: allows you to customize the {@link
 * ...
 * @throws IllegalArgumentException if the field name or value
 *         is null.
 */
public StoredField(String name, BytesRef bytes, FieldType type) {
  super(name, bytes, type);
}

 

3) Line 71 method

/**
 * Create a stored-only field with the given binary value.
 * ...
 * @throws IllegalArgumentException if the field name is null.
 */
public StoredField(String name, byte[] value) {
  super(name, value, TYPE);
}

It is misleading because there is no explanation for byte array.
If you follow that super class, you can see the following code(Field class).

public Field(String name, byte[] value, IndexableFieldType type) {
  this(name, value, 0, value.length, type);
}
// To
public Field(String name, byte[] value, int offset, int length, IndexableFieldType type) {
  this(name, new BytesRef(value, offset, length), type);
}

When declaring a new BytesRef, an Illegal exception will be thrown if value is null.

public BytesRef(byte[] bytes, int offset, int length) {
  this.bytes = bytes;
  this.offset = offset;
  this.length = length;
  assert isValid();
}

public boolean isValid() {
  if (bytes == null) {
    throw new IllegalStateException("bytes is null");
  }
  ...
}

For that reason, I changed the Javadoc to:

/**
 * Create a stored-only field with the given binary value.
 * <p>NOTE: the provided byte[] is not copied so be sure
 * not to change it until you're done with this field.
 * @param name field name
 * @param value byte array pointing to binary content (not copied)
 * @throws IllegalArgumentException if the field name is null.
 * @throws IllegalStateException if the value is null.
 */
public StoredField(String name, byte[] value) {
  super(name, value, TYPE);
}

 

4) Line 85 method

For the same reason as "3)", I changed the Javadoc to:

/**
 * Create a stored-only field with the given binary value.
 * <p>NOTE: the provided byte[] is not copied so be sure
 * not to change it until you're done with this field.
 * @param name field name
 * @param value byte array pointing to binary content (not copied)
 * @param offset starting position of the byte array
 * @param length valid length of the byte array
 * @throws IllegalArgumentException if the field name is null.
 * @throws IllegalStateException if the value is null.
 */
public StoredField(String name, byte[] value, int offset, int length) {
  super(name, value, offset, length, TYPE);
}

 

5) Line 97 method

For the same reason as "2)", I changed the Javadoc to:

/**
 * Create a stored-only field with the given binary value.
 * <p>NOTE: the provided BytesRef is not copied so be sure
 * not to change it until you're done with this field.
 * @param name field name
 * @param value BytesRef pointing to binary content (not copied)
 * @throws IllegalArgumentException if the field name or value
 *         is null.
 */
public StoredField(String name, BytesRef value) {
  super(name, value, TYPE);
}

 

6) Line 119 method

/**
 * Expert: allows you to customize the {@link
 * ...
 * @throws IllegalArgumentException if the field name or value is null.
 */
public StoredField(String name, String value, FieldType type) {
  super(name, value, type);
}

It is misleading because there is no explanation for type.
If you follow that super class, you can see the following code(Field class).

/**
 * Create field with String value.
 * @param name field name
 * @param value string value
 * @param type field type
 * @throws IllegalArgumentException if either the name or value
 *         is null, or if the field's type is neither indexed() nor stored(), 
 *         or if indexed() is false but storeTermVectors() is true.
 * @throws NullPointerException if the type is null
 */
public Field(String name, String value, IndexableFieldType type) {
  if (name == null) {
    throw new IllegalArgumentException("name must not be null");
  }
  if (value == null) {
    throw new IllegalArgumentException("value must not be null");
  }
  if (!type.stored() && type.indexOptions() == IndexOptions.NONE) {
    throw new IllegalArgumentException("it doesn't make sense to have a field that "
      + "is neither indexed nor stored");
  }
  this.type = type;
  this.name = name;
  this.fieldsData = value;
}

Field class has the exception handling(NPE) for null IndexableFieldType object.
(if type is null, NPE can be occured when run type.stored())
For that reason, I changed the Javadoc to:

/**
 * Expert: allows you to customize the {@link
 * FieldType}.
 * @param name field name
 * @param value string value
 * @param type custom {@link FieldType} for this field
 * @throws IllegalArgumentException if the field name or value is null.
 * @throws NullPointerException if the type is null.
 */
public StoredField(String name, String value, FieldType type) {
  super(name, value, type);
}

 

7) Wrong Javadocs in Field Class.

I saw the wrong "NullPointerException" throws in Javadoc.

Line 176, 194 and 210 methods have a NPE throws in Javadoc.

// Line 176 method
/**
 * Create field with binary value.
 * ...
 * @throws NullPointerException if the type is null
 */
public Field(String name, byte[] value, IndexableFieldType type) {
  this(name, value, 0, value.length, type);
}

// Line 194 method
/**
 * Create field with binary value.
 * ...
 * @throws NullPointerException if the type is null
 */
public Field(String name, byte[] value, int offset, int length, IndexableFieldType type) {
  this(name, new BytesRef(value, offset, length), type);
}

// Line 210 method
/**
 * Create field with binary value.
 * ...
 * @throws NullPointerException if the type is null
 */
public Field(String name, BytesRef bytes, IndexableFieldType type) {
  if (name == null) {
    throw new IllegalArgumentException("name must not be null");
  }
  if (bytes == null) {
    throw new IllegalArgumentException("bytes must not be null");
  }
  this.fieldsData = bytes;
  this.type = type;
  this.name = name;
}

Line 176 and 194 methods call Line 210 method.
However, this method can not cause the NullPointerException.
If type is null, it is just the following code.

protected final IndexableFieldType type = null;

In fact, I think the null check is missing.
But it's just my idea, so I can not decide whether to remove Javadoc's throws or insert a null check code.

If it is decided, I will work on the related issue separately.