[
https://issues.apache.org/jira/browse/LUCENE-8460?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16592158#comment-16592158
]
Namgyu Kim commented on LUCENE-8460:
------------------------------------
Hi, [~jpountz].
Thank you for your reply.
I uploaded a new patch.
■ Added content
*1) Add Null Check (Field)*
{code:java}
if (type == null) {
throw new IllegalArgumentException("type must not be null");
}
{code}
I added a null check for "IndexableFieldType".
*2) Edit Javadoc (Field, StoredField)*
I changed Javadoc to fit the new "Field" class.
(like delete throws NullPointerException)
*3) Very Small Refactoring (Field)*
before
{code:java}
// Line 210 method
public Field(String name, BytesRef bytes, IndexableFieldType type) {
...
this.fieldsData = bytes;
this.type = type;
this.name = name;
}
// Line 234 method
public Field(String name, String value, IndexableFieldType type) {
...
this.type = type;
this.name = name;
this.fieldsData = value;
}
{code}
after
{code:java}
// Line 210 method
public Field(String name, BytesRef bytes, IndexableFieldType type) {
...
this.name = name;
this.fieldsData = bytes;
this.type = type;
}
// Line 234 method
public Field(String name, String value, IndexableFieldType type) {
...
this.name = name;
this.fieldsData = value;
this.type = type;
}
{code}
■ Discussion
1) Remaining NullPointerException throws
{code:java}
// Line 112 method
/**
* Create field with Reader value.
* ...
* @throws NullPointerException if the reader is null
*/
public Field(String name, Reader reader, IndexableFieldType type) {
...
if (reader == null) {
throw new NullPointerException("reader must not be null");
}
...
}
// Line 144 method
/**
* Create field with TokenStream value.
* ...
* @throws NullPointerException if the tokenStream is null
*/
public Field(String name, TokenStream tokenStream, IndexableFieldType type) {
...
if (tokenStream == null) {
throw new NullPointerException("tokenStream must not be null");
}
...
}{code}
Are the above throws NullPointerException codes okay?
2) Issue Name
The current issue name is "Javadoc changes in StoredField".
However, I think that the content has changed so much that it needs to be
modified.
What do you think a good name is?
> 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
