Skip to content

Commit

Permalink
Remove BytesRef usage from XContentParser and its subclasses (elastic…
Browse files Browse the repository at this point in the history 
…#28792)

* Remove BytesRef usage from XContentParser and its subclasses

This removes all the BytesRef usage from XContentParser in favor of directly
returning a CharBuffer (this was originally what was returned, it was just
immediately wraped in a BytesRef).

Relates to elastic#28504

* Rename method after Ryan's feedback
  • Loading branch information
@dakrone
dakrone authored Mar 7, 2018
1 parent 1d6402e commit f13a9f7
Show file tree
Hide file tree
Showing 16 changed files with 100 additions and 82 deletions.
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ public XContentBuilder field(String name, BytesRef value) throws IOException {
/**
* Writes the binary content of the given {@link BytesRef} as UTF-8 bytes.
*
* Use {@link XContentParser#utf8Bytes()} to read the value back
* Use {@link XContentParser#charBuffer()} to read the value back
*/
public XContentBuilder utf8Field(String name, BytesRef value) throws IOException {
return field(name).utf8Value(value);
Expand All @@ -615,7 +615,7 @@ public XContentBuilder binaryValue(BytesRef value) throws IOException {
/**
* Writes the binary content of the given {@link BytesRef} as UTF-8 bytes.
*
* Use {@link XContentParser#utf8Bytes()} to read the value back
* Use {@link XContentParser#charBuffer()} to read the value back
*/
public XContentBuilder utf8Value(BytesRef value) throws IOException {
if (value == null) {
Expand Down
14 changes: 4 additions & 10 deletions server/src/main/java/org/elasticsearch/common/xcontent/XContentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@

package org.elasticsearch.common.xcontent;

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.lease.Releasable;

import java.io.IOException;
import java.nio.CharBuffer;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -144,17 +144,13 @@ enum NumberType {

String textOrNull() throws IOException;

/**
* Returns a BytesRef holding UTF-8 bytes or null if a null value is {@link Token#VALUE_NULL}.
* This method should be used to read text only binary content should be read through {@link #binaryValue()}
*/
BytesRef utf8BytesOrNull() throws IOException;
CharBuffer charBufferOrNull() throws IOException;

/**
* Returns a BytesRef holding UTF-8 bytes.
* Returns a {@link CharBuffer} holding UTF-8 bytes.
* This method should be used to read text only binary content should be read through {@link #binaryValue()}
*/
BytesRef utf8Bytes() throws IOException;
CharBuffer charBuffer() throws IOException;

Object objectText() throws IOException;

Expand Down Expand Up @@ -248,8 +244,6 @@ enum NumberType {
*
* these methods write UTF-8 encoded strings and must be read through:
* <ul>
* <li>{@link XContentParser#utf8Bytes()}</li>
* <li>{@link XContentParser#utf8BytesOrNull()}}</li>
* <li>{@link XContentParser#text()} ()}</li>
* <li>{@link XContentParser#textOrNull()} ()}</li>
* <li>{@link XContentParser#textCharacters()} ()}}</li>
Expand Down
10 changes: 4 additions & 6 deletions server/src/main/java/org/elasticsearch/common/xcontent/json/JsonXContentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;

import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.IOUtils;
import org.elasticsearch.common.xcontent.DeprecationHandler;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
Expand Down Expand Up @@ -88,8 +87,8 @@ public String text() throws IOException {
}

@Override
public BytesRef utf8Bytes() throws IOException {
return new BytesRef(CharBuffer.wrap(parser.getTextCharacters(), parser.getTextOffset(), parser.getTextLength()));
public CharBuffer charBuffer() throws IOException {
return CharBuffer.wrap(parser.getTextCharacters(), parser.getTextOffset(), parser.getTextLength());
}

@Override
Expand All @@ -114,7 +113,7 @@ public Object objectText() throws IOException {
public Object objectBytes() throws IOException {
JsonToken currentToken = parser.getCurrentToken();
if (currentToken == JsonToken.VALUE_STRING) {
return utf8Bytes();
return charBuffer();
} else if (currentToken == JsonToken.VALUE_NUMBER_INT || currentToken == JsonToken.VALUE_NUMBER_FLOAT) {
return parser.getNumberValue();
} else if (currentToken == JsonToken.VALUE_TRUE) {
Expand All @@ -124,8 +123,7 @@ public Object objectBytes() throws IOException {
} else if (currentToken == JsonToken.VALUE_NULL) {
return null;
} else {
//TODO should this really do UTF-8 conversion?
return utf8Bytes();
return charBuffer();
}
}

Expand Down
7 changes: 3 additions & 4 deletions server/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.elasticsearch.common.xcontent.support;

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Numbers;
Expand All @@ -28,6 +27,7 @@
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
import java.nio.CharBuffer;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
Expand Down Expand Up @@ -240,13 +240,12 @@ public final String textOrNull() throws IOException {
return text();
}


@Override
public BytesRef utf8BytesOrNull() throws IOException {
public CharBuffer charBufferOrNull() throws IOException {
if (currentToken() == Token.VALUE_NULL) {
return null;
}
return utf8Bytes();
return charBuffer();
}

@Override
Expand Down
19 changes: 12 additions & 7 deletions server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
import java.nio.CharBuffer;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
Expand Down Expand Up @@ -194,27 +195,31 @@ public final int hashCode() {
protected abstract int doHashCode();

/**
* This helper method checks if the object passed in is a string, if so it
* converts it to a {@link BytesRef}.
* This helper method checks if the object passed in is a string or {@link CharBuffer},
* if so it converts it to a {@link BytesRef}.
* @param obj the input object
* @return the same input object or a {@link BytesRef} representation if input was of type string
*/
static Object convertToBytesRefIfString(Object obj) {
static Object maybeConvertToBytesRef(Object obj) {
if (obj instanceof String) {
return BytesRefs.toBytesRef(obj);
} else if (obj instanceof CharBuffer) {
return new BytesRef((CharBuffer) obj);
}
return obj;
}

/**
* This helper method checks if the object passed in is a {@link BytesRef}, if so it
* converts it to a utf8 string.
* This helper method checks if the object passed in is a {@link BytesRef} or {@link CharBuffer},
* if so it converts it to a utf8 string.
* @param obj the input object
* @return the same input object or a utf8 string if input was of type {@link BytesRef}
* @return the same input object or a utf8 string if input was of type {@link BytesRef} or {@link CharBuffer}
*/
static Object convertToStringIfBytesRef(Object obj) {
static Object maybeConvertToString(Object obj) {
if (obj instanceof BytesRef) {
return ((BytesRef) obj).utf8ToString();
} else if (obj instanceof CharBuffer) {
return new BytesRef((CharBuffer) obj).utf8ToString();
}
return obj;
}
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/elasticsearch/index/query/BaseTermQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public BaseTermQueryBuilder(String fieldName, Object value) {
throw new IllegalArgumentException("value cannot be null");
}
this.fieldName = fieldName;
this.value = convertToBytesRefIfString(value);
this.value = maybeConvertToBytesRef(value);
}

/**
Expand Down Expand Up @@ -144,14 +144,14 @@ public String fieldName() {
* If necessary, converts internal {@link BytesRef} representation back to string.
*/
public Object value() {
return convertToStringIfBytesRef(this.value);
return maybeConvertToString(this.value);
}

@Override
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(getName());
builder.startObject(fieldName);
builder.field(VALUE_FIELD.getPreferredName(), convertToStringIfBytesRef(this.value));
builder.field(VALUE_FIELD.getPreferredName(), maybeConvertToString(this.value));
printBoostAndQueryName(builder);
builder.endObject();
builder.endObject();
Expand Down
12 changes: 6 additions & 6 deletions server/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ public FuzzyQueryBuilder(String fieldName, Object value) {
throw new IllegalArgumentException("query value cannot be null");
}
this.fieldName = fieldName;
this.value = convertToBytesRefIfString(value);
this.value = maybeConvertToBytesRef(value);
}

/**
Expand Down Expand Up @@ -186,7 +186,7 @@ public String fieldName() {
}

public Object value() {
return convertToStringIfBytesRef(this.value);
return maybeConvertToString(this.value);
}

public FuzzyQueryBuilder fuzziness(Fuzziness fuzziness) {
Expand Down Expand Up @@ -238,7 +238,7 @@ public String rewrite() {
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(NAME);
builder.startObject(fieldName);
builder.field(VALUE_FIELD.getPreferredName(), convertToStringIfBytesRef(this.value));
builder.field(VALUE_FIELD.getPreferredName(), maybeConvertToString(this.value));
fuzziness.toXContent(builder, params);
builder.field(PREFIX_LENGTH_FIELD.getPreferredName(), prefixLength);
builder.field(MAX_EXPANSIONS_FIELD.getPreferredName(), maxExpansions);
Expand Down Expand Up @@ -274,9 +274,9 @@ public static FuzzyQueryBuilder fromXContent(XContentParser parser) throws IOExc
currentFieldName = parser.currentName();
} else if (token.isValue()) {
if (TERM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (VALUE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
boost = parser.floatValue();
} else if (Fuzziness.FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
Expand All @@ -303,7 +303,7 @@ public static FuzzyQueryBuilder fromXContent(XContentParser parser) throws IOExc
} else {
throwParsingExceptionOnMultipleFields(NAME, parser.getTokenLocation(), fieldName, parser.currentName());
fieldName = parser.currentName();
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
}
}
return new FuzzyQueryBuilder(fieldName, value)
Expand Down
24 changes: 12 additions & 12 deletions server/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public String fieldName() {
* of query to be equal regardless of whether it was created from XContent or via Java API.
*/
public RangeQueryBuilder from(Object from, boolean includeLower) {
this.from = convertToBytesRefIfString(from);
this.from = maybeConvertToBytesRef(from);
this.includeLower = includeLower;
return this;
}
Expand All @@ -178,7 +178,7 @@ public RangeQueryBuilder from(Object from) {
* Gets the lower range value for this query.
*/
public Object from() {
return convertToStringIfBytesRef(this.from);
return maybeConvertToString(this.from);
}

/**
Expand All @@ -199,7 +199,7 @@ public RangeQueryBuilder gte(Object from) {
* The to part of the range query. Null indicates unbounded.
*/
public RangeQueryBuilder to(Object to, boolean includeUpper) {
this.to = convertToBytesRefIfString(to);
this.to = maybeConvertToBytesRef(to);
this.includeUpper = includeUpper;
return this;
}
Expand All @@ -218,7 +218,7 @@ public RangeQueryBuilder to(Object to) {
* of query to be equal regardless of whether it was created from XContent or via Java API.
*/
public Object to() {
return convertToStringIfBytesRef(this.to);
return maybeConvertToString(this.to);
}

/**
Expand Down Expand Up @@ -334,8 +334,8 @@ public RangeQueryBuilder relation(String relation) {
protected void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(NAME);
builder.startObject(fieldName);
builder.field(FROM_FIELD.getPreferredName(), convertToStringIfBytesRef(this.from));
builder.field(TO_FIELD.getPreferredName(), convertToStringIfBytesRef(this.to));
builder.field(FROM_FIELD.getPreferredName(), maybeConvertToString(this.from));
builder.field(TO_FIELD.getPreferredName(), maybeConvertToString(this.to));
builder.field(INCLUDE_LOWER_FIELD.getPreferredName(), includeLower);
builder.field(INCLUDE_UPPER_FIELD.getPreferredName(), includeUpper);
if (timeZone != null) {
Expand Down Expand Up @@ -377,26 +377,26 @@ public static RangeQueryBuilder fromXContent(XContentParser parser) throws IOExc
currentFieldName = parser.currentName();
} else {
if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
from = parser.objectBytes();
from = maybeConvertToBytesRef(parser.objectBytes());
} else if (TO_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
to = parser.objectBytes();
to = maybeConvertToBytesRef(parser.objectBytes());
} else if (INCLUDE_LOWER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
includeLower = parser.booleanValue();
} else if (INCLUDE_UPPER_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
includeUpper = parser.booleanValue();
} else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
boost = parser.floatValue();
} else if (GT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
from = parser.objectBytes();
from = maybeConvertToBytesRef(parser.objectBytes());
includeLower = false;
} else if (GTE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
from = parser.objectBytes();
from = maybeConvertToBytesRef(parser.objectBytes());
includeLower = true;
} else if (LT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
to = parser.objectBytes();
to = maybeConvertToBytesRef(parser.objectBytes());
includeUpper = false;
} else if (LTE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
to = parser.objectBytes();
to = maybeConvertToBytesRef(parser.objectBytes());
includeUpper = true;
} else if (TIME_ZONE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
timeZone = parser.text();
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/elasticsearch/index/query/SpanTermQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ public static SpanTermQueryBuilder fromXContent(XContentParser parser) throws IO
currentFieldName = parser.currentName();
} else {
if (TERM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (BaseTermQueryBuilder.VALUE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
boost = parser.floatValue();
} else if (AbstractQueryBuilder.NAME_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
Expand All @@ -125,7 +125,7 @@ public static SpanTermQueryBuilder fromXContent(XContentParser parser) throws IO
} else {
throwParsingExceptionOnMultipleFields(NAME, parser.getTokenLocation(), fieldName, parser.currentName());
fieldName = parser.currentName();
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
}
}

Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/elasticsearch/index/query/TermQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,9 @@ public static TermQueryBuilder fromXContent(XContentParser parser) throws IOExce
currentFieldName = parser.currentName();
} else {
if (TERM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (VALUE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (AbstractQueryBuilder.NAME_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
queryName = parser.text();
} else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
Expand All @@ -116,7 +116,7 @@ public static TermQueryBuilder fromXContent(XContentParser parser) throws IOExce
} else if (token.isValue()) {
throwParsingExceptionOnMultipleFields(NAME, parser.getTokenLocation(), fieldName, parser.currentName());
fieldName = currentFieldName;
value = parser.objectBytes();
value = maybeConvertToBytesRef(parser.objectBytes());
} else if (token == XContentParser.Token.START_ARRAY) {
throw new ParsingException(parser.getTokenLocation(), "[term] query does not support array of values");
}
Expand Down
Loading

0 comments on commit f13a9f7

Please sign in to comment.