[
https://issues.apache.org/jira/browse/LUCENE-8460?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16592435#comment-16592435
]
Lucene/Solr QA commented on LUCENE-8460:
----------------------------------------
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
|| || || || {color:brown} Prechecks {color} ||
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m
0s{color} | {color:red} The patch doesn't appear to include any new or modified
tests. Please justify why no new tests are needed for this patch. Also please
list what manual steps were performed to verify this patch. {color} |
|| || || || {color:brown} master Compile Tests {color} ||
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
20s{color} | {color:green} master passed {color} |
|| || || || {color:brown} Patch Compile Tests {color} ||
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Release audit (RAT) {color} |
{color:green} 0m 18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Check forbidden APIs {color} |
{color:green} 0m 18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} Validate source patterns {color} |
{color:green} 0m 18s{color} | {color:green} the patch passed {color} |
|| || || || {color:brown} Other Tests {color} ||
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 11m
13s{color} | {color:green} core in the patch passed. {color} |
| {color:black}{color} | {color:black} {color} | {color:black} 13m 26s{color} |
{color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | LUCENE-8460 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12937064/LUCENE-8460.patch |
| Optional Tests | compile javac unit ratsources checkforbiddenapis
validatesourcepatterns |
| uname | Linux lucene1-us-west 4.4.0-130-generic #156~14.04.1-Ubuntu SMP Thu
Jun 14 13:51:47 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | ant |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-LUCENE-Build/sourcedir/dev-tools/test-patch/lucene-solr-yetus-personality.sh
|
| git revision | master / f26dd13 |
| ant | version: Apache Ant(TM) version 1.9.3 compiled on July 24 2018 |
| Default Java | 1.8.0_172 |
| Test Results |
https://builds.apache.org/job/PreCommit-LUCENE-Build/79/testReport/ |
| modules | C: lucene/core U: lucene/core |
| Console output |
https://builds.apache.org/job/PreCommit-LUCENE-Build/79/console |
| Powered by | Apache Yetus 0.7.0 http://yetus.apache.org |
This message was automatically generated.
> 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: [email protected]
For additional commands, e-mail: [email protected]
