-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pluggable XContentBuilder writers and human readable writers #29120
Changes from 3 commits
3a1fcf9
2feab02
1cfba0d
24f3635
1a67223
6629733
01eab8b
cb07b88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,7 @@ | |
import java.util.Locale; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.ServiceLoader; | ||
import java.util.Set; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
|
@@ -87,6 +88,7 @@ public static XContentBuilder builder(XContent xContent, Set<String> includes, S | |
public static final DateTimeFormatter DEFAULT_DATE_PRINTER = ISODateTimeFormat.dateTime().withZone(DateTimeZone.UTC); | ||
|
||
private static final Map<Class<?>, Writer> WRITERS; | ||
private static final Map<Class<?>, HumanReadableTransformer> HUMAN_READABLE_TRANSFORMERS; | ||
static { | ||
Map<Class<?>, Writer> writers = new HashMap<>(); | ||
writers.put(Boolean.class, (b, v) -> b.value((Boolean) v)); | ||
|
@@ -110,13 +112,28 @@ public static XContentBuilder builder(XContent xContent, Set<String> includes, S | |
writers.put(Text.class, (b, v) -> b.value((Text) v)); | ||
|
||
WRITERS = Collections.unmodifiableMap(writers); | ||
|
||
Map<Class<?>, HumanReadableTransformer> humanReadableTransformer = new HashMap<>(); | ||
// These will be moved to a different class at a later time to decouple them from XContentBuilder | ||
humanReadableTransformer.put(TimeValue.class, v -> ((TimeValue) v).millis()); | ||
humanReadableTransformer.put(ByteSizeValue.class, v -> ((ByteSizeValue) v).getBytes()); | ||
|
||
HUMAN_READABLE_TRANSFORMERS = Collections.unmodifiableMap(humanReadableTransformer); | ||
} | ||
|
||
@FunctionalInterface | ||
private interface Writer { | ||
interface Writer { | ||
void write(XContentBuilder builder, Object value) throws IOException; | ||
} | ||
|
||
/** | ||
* Interface for transforming complex objects into their "raw" equivalents for human-readable fields | ||
*/ | ||
@FunctionalInterface | ||
interface HumanReadableTransformer { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think this should be an interface the thing implements rather than a transformer to look up? That feels a little more explicit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With an interface we can't extend classes we don't control (Lucene or Joda ones) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we could add both if we wanted? I can do that in subsequent work if you think it'd be worth adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. I don't think it is worth adding multiple ways to do this. |
||
Object rawValue(Object value) throws IOException; | ||
} | ||
|
||
/** | ||
* XContentGenerator used to build the XContent object | ||
*/ | ||
|
@@ -132,6 +149,16 @@ private interface Writer { | |
*/ | ||
private boolean humanReadable = false; | ||
|
||
/** | ||
* Additional pluggable writers for classes outside of XContent's control | ||
*/ | ||
private final Map<Class<?>, Writer> additionalWriters; | ||
|
||
/** | ||
* Additional transformers for human readable fields | ||
*/ | ||
private final Map<Class<?>, HumanReadableTransformer> additionalHumanReadableTransformers; | ||
|
||
/** | ||
* Constructs a new builder using the provided XContent and an OutputStream. Make sure | ||
* to call {@link #close()} when the builder is done with. | ||
|
@@ -164,6 +191,14 @@ public XContentBuilder(XContent xContent, OutputStream bos, Set<String> includes | |
public XContentBuilder(XContent xContent, OutputStream os, Set<String> includes, Set<String> excludes) throws IOException { | ||
this.bos = os; | ||
this.generator = xContent.createGenerator(bos, includes, excludes); | ||
Map<Class<?>, Writer> writers = new HashMap<>(); | ||
Map<Class<?>, HumanReadableTransformer> transformers = new HashMap<>(); | ||
for (XContentBuilderProvider service : ServiceLoader.load(XContentBuilderProvider.class)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do the service loading in a static block? Or during startup? It feels like we could still have one map if this were static. It wouldn't work with plugins but I think it'd work fine otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should definitely be static. Calling ServiceLoader.load is not "cheap" (must iterate classloaders/resources). The writers and transformers should be static members, and initialized in a static block which calls ServiceLoader. You can explicitly add writers and transformers for the known types there, in addition to the ones coming from the extensions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I'll move this to a static block for now, and move to passing it in at in a different place when the separation is made. |
||
writers.putAll(service.getXContentWriters()); | ||
transformers.putAll(service.getXContentHumanReadableTransformers()); | ||
} | ||
additionalWriters = Collections.unmodifiableMap(writers); | ||
additionalHumanReadableTransformers = Collections.unmodifiableMap(transformers); | ||
} | ||
|
||
public XContentType contentType() { | ||
|
@@ -798,6 +833,13 @@ private void unknownValue(Object value, boolean ensureNoSelfReferences) throws I | |
value((ReadableInstant) value); | ||
} else if (value instanceof ToXContent) { | ||
value((ToXContent) value); | ||
} else if (additionalWriters.containsKey(value.getClass())) { | ||
Writer additionalWriter = additionalWriters.get(value.getClass()); | ||
if (additionalWriter == null) { | ||
throw new NullPointerException("xcontent writer for class " + value.getClass() + " cannot be null"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use |
||
} else { | ||
additionalWriter.write(this, value); | ||
} | ||
} else { | ||
// This is a "value" object (like enum, DistanceUnit, etc) just toString() it | ||
// (yes, it can be misleading when toString a Java class, but really, jackson should be used in that case) | ||
|
@@ -891,33 +933,37 @@ private XContentBuilder value(Iterable<?> values, boolean ensureNoSelfReferences | |
} | ||
|
||
//////////////////////////////////////////////////////////////////////////// | ||
// Misc. | ||
// Human readable fields | ||
// | ||
// These are fields that have a "raw" value and a "human readable" value, | ||
// such as time values or byte sizes. The human readable variant is only | ||
// used if the humanReadable flag has been set | ||
////////////////////////////////// | ||
|
||
public XContentBuilder timeValueField(String rawFieldName, String readableFieldName, TimeValue timeValue) throws IOException { | ||
public XContentBuilder humanReadableField(String rawFieldName, String readableFieldName, Object value) throws IOException { | ||
if (humanReadable) { | ||
field(readableFieldName, timeValue.toString()); | ||
field(readableFieldName, Objects.toString(value)); | ||
} | ||
field(rawFieldName, timeValue.millis()); | ||
return this; | ||
} | ||
|
||
public XContentBuilder timeValueField(String rawFieldName, String readableFieldName, long rawTime) throws IOException { | ||
if (humanReadable) { | ||
field(readableFieldName, new TimeValue(rawTime).toString()); | ||
HumanReadableTransformer transformer = HUMAN_READABLE_TRANSFORMERS.get(value.getClass()); | ||
if (transformer != null) { | ||
Object rawValue = transformer.rawValue(value); | ||
field(rawFieldName, rawValue); | ||
} else if (additionalHumanReadableTransformers.containsKey(value.getClass())) { | ||
HumanReadableTransformer additionalTransformer = additionalHumanReadableTransformers.get(value.getClass()); | ||
if (additionalTransformer == null) { | ||
throw new NullPointerException("xcontent transformer for class " + value.getClass() + " cannot be null"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same, you can use requiresNonNull. But I think it would be better to validate these are non null when loading the extensions (ie statically) rather than at usage time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I'll validate these when loading rather then usage, good idea |
||
} else { | ||
field(rawFieldName, additionalTransformer.rawValue(value)); | ||
} | ||
} else { | ||
throw new IllegalArgumentException("no raw transformer found for class " + value.getClass()); | ||
} | ||
field(rawFieldName, rawTime); | ||
return this; | ||
} | ||
|
||
public XContentBuilder timeValueField(String rawFieldName, String readableFieldName, long rawTime, TimeUnit timeUnit) throws | ||
IOException { | ||
if (humanReadable) { | ||
field(readableFieldName, new TimeValue(rawTime, timeUnit).toString()); | ||
} | ||
field(rawFieldName, rawTime); | ||
return this; | ||
} | ||
//////////////////////////////////////////////////////////////////////////// | ||
// Misc. | ||
////////////////////////////////// | ||
|
||
|
||
public XContentBuilder percentageField(String rawFieldName, String readableFieldName, double percentage) throws IOException { | ||
|
@@ -928,14 +974,6 @@ public XContentBuilder percentageField(String rawFieldName, String readableField | |
return this; | ||
} | ||
|
||
public XContentBuilder byteSizeField(String rawFieldName, String readableFieldName, ByteSizeValue byteSizeValue) throws IOException { | ||
if (humanReadable) { | ||
field(readableFieldName, byteSizeValue.toString()); | ||
} | ||
field(rawFieldName, byteSizeValue.getBytes()); | ||
return this; | ||
} | ||
|
||
public XContentBuilder byteSizeField(String rawFieldName, String readableFieldName, long rawSize) throws IOException { | ||
if (humanReadable) { | ||
field(readableFieldName, new ByteSizeValue(rawSize).toString()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.common.xcontent; | ||
|
||
import java.util.Map; | ||
|
||
/** | ||
* This interface provides a way for non-JDK classes to plug in a way to serialize to xcontent. | ||
* | ||
* It is <b>greatly</b> preferred that you implement {@link ToXContentFragment} | ||
* in the class for encoding, however, in some situations you may not own the | ||
* class, in which case you can add an implementation here for encoding it. | ||
*/ | ||
public interface XContentBuilderProvider { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've been naming these class with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I'll rename this |
||
|
||
/** | ||
* Used for plugging in a generic writer for a class, for example, an example implementation: | ||
* | ||
* <pre> | ||
* Map<Class<?>, XContentBuilder.Writer> addlWriters = new HashMap<>(); | ||
* addlWriters.put(BytesRef.class, (builder, value) -> b.value(((BytesRef) value).utf8String())); | ||
* return addlWriters; | ||
* </pre> | ||
* | ||
* @return a map of class name to writer | ||
*/ | ||
Map<Class<?>, XContentBuilder.Writer> getXContentWriters(); | ||
|
||
/** | ||
* Used for plugging in a human readable version of a class's encoding. It is assumed that | ||
* the human readable equivalent is <b>always</b> behind the {@code toString()} method, so | ||
* this transformer returns the raw value to be used. | ||
* | ||
* An example implementation: | ||
* | ||
* <pre> | ||
* Map<Class<?>, XContentBuilder.HumanReadableTransformer> transformers = new HashMap<>(); | ||
* transformers.put(ByteSizeValue.class, (value) -> ((ByteSizeValue) value).bytes()); | ||
* </pre> | ||
* @return a map of class name to transformer used to retrieve raw value | ||
*/ | ||
Map<Class<?>, XContentBuilder.HumanReadableTransformer> getXContentHumanReadableTransformers(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to be
public
right? Otherwise they are package private?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't planning on making them public until they were needed to be, but I can make them public now