lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Namgyu Kim (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (LUCENE-8460) Better argument validation in StoredField
Date Tue, 28 Aug 2018 15:50:00 GMT

     [ https://issues.apache.org/jira/browse/LUCENE-8460?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Namgyu Kim updated LUCENE-8460:
-------------------------------
    Attachment:     (was: LUCENE-8460.patch)

> Better argument validation in StoredField
> -----------------------------------------
>
>                 Key: LUCENE-8460
>                 URL: https://issues.apache.org/jira/browse/LUCENE-8460
>             Project: Lucene - Core
>          Issue Type: Improvement
>          Components: core/other
>            Reporter: Namgyu Kim
>            Priority: Major
>              Labels: arguments/parameters, javadocs
>         Attachments: LUCENE-8460.patch, LUCENE-8460.patch, LUCENE-8460.patch
>
>
> I have found some invalid Javadocs in StoredField Class.
>  (and I think Field Class has some problems too :D)
>  
> 1) Line 45 method
> {code:java}
> /**
>  * Expert: allows you to customize the {@link
>  * ...
>  * @throws IllegalArgumentException if the field name is null.
>  */
> protected StoredField(String name, FieldType type) {
>   super(name, type);
> }
> {code}
> It is misleading because there is no explanation for *type*.
>  If you follow that super class, you can see the following code(Field class).
> {code:java}
> /**
>  * 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;
> }{code}
> Field class has the exception handling(IllegalArgumentException) for *null IndexableFieldType object*.
>  For that reason, I changed the Javadoc to:
> {code:java}
> /**
>  * 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);
> }
> {code}
>  
> 2) Line 59 method
> {code:java}
> /**
>  * 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);
> }
> {code}
> It is misleading because there is no explanation for *bytes*.
>  If you follow that super class, you can see the following code(Field class).
> {code:java}
> /**
>  * 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;
> }
> {code}
> Field class has the exception handling(IllegalArgumentException) for *null BytesRef object*.
>  For that reason, I changed the Javadoc to:
> {code:java}
> /**
>  * 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);
> }
> {code}
>  
> 3) Line 71 method
> {code:java}
> /**
>  * 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);
> }
> {code}
> 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).
> {code:java}
> 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);
> }{code}
> When declaring a new BytesRef, an Illegal exception will be thrown if value is null.
> {code:java}
> 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");
>   }
>   ...
> }{code}
> For that reason, I changed the Javadoc to:
> {code:java}
> /**
>  * 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);
> }
> {code}
>  
> 4) Line 85 method
> For the *same reason as "3)"*, I changed the Javadoc to:
> {code:java}
> /**
>  * 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);
> }
> {code}
>  
> 5) Line 97 method
> For the *same reason as "2)"*, I changed the Javadoc to:
> {code:java}
> /**
>  * 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);
> }
> {code}
>  
> 6) Line 119 method
> {code:java}
> /**
>  * 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);
> }
> {code}
> It is misleading because there is no explanation for *type*.
>  If you follow that super class, you can see the following code(Field class).
> {code:java}
> /**
>  * 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;
> }
> {code}
> 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:
> {code:java}
> /**
>  * 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);
> }
> {code}
>  
> 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.
> {code:java}
> // 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;
> }{code}
> 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.
> {code:java}
> protected final IndexableFieldType type = null;
> {code}
> 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.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message