[ https://issues.apache.org/jira/browse/LUCENE-8460?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Namgyu Kim updated LUCENE-8460: ------------------------------- Attachment: LUCENE-8460.patch > Javadoc changes 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: javadocs, newbie > Attachments: 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-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org