Add pluggable XContentBuilder writers and human readable writers #29120

dakrone · 2018-03-17T00:14:55Z

This adds the ability to use SPI to plug in writers for XContentBuilder. By
implementing the XContentBuilderProvider class we can allow Elasticsearch to
plug in different ways to encode types to JSON.

Important caveat for this, we should always try to have the class implement
ToXContentFragment first, however, in the case of classes from our
dependencies (think Joda classes or Lucene classes) we need a way to specify
writers for these classes.

This also makes the human-readable field writers generic and pluggable, so that
we no longer need to tie XContentBuilder to things like TimeValue and
ByteSizeValue. Contained as part of this moves all the TimeValue human
readable fields to the new humanReadableField method. A future commit will
move the ByteSizeValue calls over to this method.

Relates to #28504

This adds the ability to use SPI to plug in writers for XContentBuilder. By implementing the XContentBuilderProvider class we can allow Elasticsearch to plug in different ways to encode types to JSON. Important caveat for this, we should always try to have the class implement `ToXContentFragment` first, however, in the case of classes from our dependencies (think Joda classes or Lucene classes) we need a way to specify writers for these classes. This also makes the human-readable field writers generic and pluggable, so that we no longer need to tie XContentBuilder to things like `TimeValue` and `ByteSizeValue`. Contained as part of this moves all the TimeValue human readable fields to the new `humanReadableField` method. A future commit will move the `ByteSizeValue` calls over to this method. Relates to elastic#28504

elasticmachine · 2018-03-17T00:14:58Z

Pinging @elastic/es-core-infra

nik9000 · 2018-03-17T01:39:34Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+     * Interface for transforming complex objects into their "raw" equivalents for human-readable fields
+     */
+    @FunctionalInterface
+    interface HumanReadableTransformer {


Do you think this should be an interface the thing implements rather than a transformer to look up? That feels a little more explicit.

With an interface we can't extend classes we don't control (Lucene or Joda ones)

But we could add both if we wanted? I can do that in subsequent work if you think it'd be worth adding

You are right. I don't think it is worth adding multiple ways to do this.

nik9000 · 2018-03-17T01:42:07Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

@@ -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)) {


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.

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.

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.

rjernst

I left some minor comments.

rjernst · 2018-03-17T02:38:37Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilderProvider.java

+ * 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 {


We've been naming these class with the Extension suffix.

Okay, I'll rename this

rjernst · 2018-03-17T02:42:52Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+        } 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");


You could use Objects.requireNonNull(additionalWriters.get(value.getClass()); above?

rjernst · 2018-03-17T02:47:30Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

+        } 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");


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?

Yep, I'll validate these when loading rather then usage, good idea

rjernst · 2018-03-19T16:33:48Z

server/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

    }

    @FunctionalInterface
-    private interface Writer {
+    interface Writer {


These need to be public right? Otherwise they are package private?

I wasn't planning on making them public until they were needed to be, but I can make them public now

nik9000

LGTM as a start. I think it'd be nice to use the SPI as part of this PR to prove that it works but if you'd prefer to do it in a followup I'm fine with that too.

) * Add pluggable XContentBuilder writers and human readable writers This adds the ability to use SPI to plug in writers for XContentBuilder. By implementing the XContentBuilderProvider class we can allow Elasticsearch to plug in different ways to encode types to JSON. Important caveat for this, we should always try to have the class implement `ToXContentFragment` first, however, in the case of classes from our dependencies (think Joda classes or Lucene classes) we need a way to specify writers for these classes. This also makes the human-readable field writers generic and pluggable, so that we no longer need to tie XContentBuilder to things like `TimeValue` and `ByteSizeValue`. Contained as part of this moves all the TimeValue human readable fields to the new `humanReadableField` method. A future commit will move the `ByteSizeValue` calls over to this method. Relates to #28504

* master: [Docs] Update api.asciidoc (elastic#29166) Docs: Add note about missing mapping for doc values field (elastic#29036) Fix BWC issue for PreSyncedFlushResponse Remove BytesArray and BytesReference usage from XContentFactory (elastic#29151) Add pluggable XContentBuilder writers and human readable writers (elastic#29120) Add unreleased version 6.2.4 (elastic#29171) Add unreleased version 6.1.5 (elastic#29168) Add a note about using the `retry_failed` flag before accepting data loss (elastic#29160) Fix typo in percolate-query.asciidoc (elastic#29155) Require HTTP::Tiny 0.070 for release notes script

* es/master: (50 commits) Reject updates to the `_default_` mapping. (#29165) Improve similarity docs. (#29089) [Docs] Update api.asciidoc (#29166) Docs: Add note about missing mapping for doc values field (#29036) Fix BWC issue for PreSyncedFlushResponse Remove BytesArray and BytesReference usage from XContentFactory (#29151) Add pluggable XContentBuilder writers and human readable writers (#29120) Add unreleased version 6.2.4 (#29171) Add unreleased version 6.1.5 (#29168) Add a note about using the `retry_failed` flag before accepting data loss (#29160) Fix typo in percolate-query.asciidoc (#29155) Require HTTP::Tiny 0.070 for release notes script Set Java 9 checkstyle to depend on checkstyle conf (#28383) REST high-level client: add clear cache API (#28866) Docs: Add example of resetting index setting (#29048) Plugins: Fix module name conflict check for meta plugins (#29146) Build: Fix meta plugin bundled plugin names (#29147) Build: Simplify rest spec hack configuration (#29149) Build: Fix meta modules to not install as plugin in tests (#29150) Fix javadoc warning in Strings for missing parameter description ...

* es/6.x: (46 commits) Docs: Add note about missing mapping for doc values field (#29036) [DOCS] Removed 6.1.4, 6.2.2, and 6.2.3 coming tags Remove BytesArray and BytesReference usage from XContentFactory (#29151) Fix BWC issue for PreSyncedFlushResponse Add pluggable XContentBuilder writers and human readable writers (#29120) Add unreleased version 6.2.4 (#29171) Add unreleased version 6.1.5 (#29168) Add a note about using the `retry_failed` flag before accepting data loss (#29160) Fix typo in percolate-query.asciidoc (#29155) Add release notes for 6.1.4 and 6.2.3 Require HTTP::Tiny 0.070 for release notes script REST high-level client: add clear cache API (#28866) Relax remote check for bwc project checkouts (#28666) Set Java 9 checkstyle to depend on checkstyle conf (#28383) Docs: Add example of resetting index setting (#29048) Plugins: Fix module name conflict check for meta plugins (#29146) Build: Fix meta plugin bundled plugin names (#29147) Build: Simplify rest spec hack configuration (#29149) CLI: Close subcommands in MultiCommand (#28954) Build: Fix meta modules to not install as plugin in tests (#29150) ...

* master: Fix BWC issue for PreSyncedFlushResponse Remove BytesArray and BytesReference usage from XContentFactory (elastic#29151) Add pluggable XContentBuilder writers and human readable writers (elastic#29120) Add unreleased version 6.2.4 (elastic#29171) Add unreleased version 6.1.5 (elastic#29168) Add a note about using the `retry_failed` flag before accepting data loss (elastic#29160) Fix typo in percolate-query.asciidoc (elastic#29155) Require HTTP::Tiny 0.070 for release notes script

When index sorting is enabled, toXContent tried to serialize an SortField object, resulting in an exception, when using the _segments endpoint. Relates elastic#29120

When index sorting is enabled, toXContent tried to serialize an SortField object, resulting in an exception, when using the _segments endpoint. Relates #29120

dakrone added >non-issue :Core/Infra/Core Core issues without another label v7.0.0 v6.3.0 labels Mar 17, 2018

dakrone added 2 commits March 16, 2018 18:17

Fix type in javadoc

2feab02

Fix another typo in javadoc

1cfba0d

nik9000 reviewed Mar 17, 2018

View reviewed changes

rjernst reviewed Mar 17, 2018

View reviewed changes

dakrone added 3 commits March 19, 2018 09:57

Merge remote-tracking branch 'origin/master' into pluggable-xcb-writers

24f3635

Move pluggable implementation into static block

1a67223

Rename XContentBuilderProvider -> XContentBuilderExtension

6629733

rjernst reviewed Mar 19, 2018

View reviewed changes

dakrone added 2 commits March 19, 2018 10:46

Make functional interfaces public

01eab8b

Fix javadoc

cb07b88

nik9000 approved these changes Mar 20, 2018

View reviewed changes

dakrone merged commit 4bd217c into elastic:master Mar 20, 2018

dakrone deleted the pluggable-xcb-writers branch April 19, 2018 14:44

spinscale mentioned this pull request Sep 5, 2018

Core: Fix IndicesSegmentResponse.toXcontent() serialization #33414

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pluggable XContentBuilder writers and human readable writers #29120

Add pluggable XContentBuilder writers and human readable writers #29120

dakrone commented Mar 17, 2018

elasticmachine commented Mar 17, 2018

nik9000 Mar 17, 2018

dakrone Mar 19, 2018

dakrone Mar 19, 2018

nik9000 Mar 19, 2018

nik9000 Mar 17, 2018

rjernst Mar 17, 2018

dakrone Mar 19, 2018

rjernst left a comment •

edited

Loading

rjernst Mar 17, 2018

dakrone Mar 19, 2018

rjernst Mar 17, 2018

rjernst Mar 17, 2018

dakrone Mar 19, 2018

rjernst Mar 19, 2018

dakrone Mar 19, 2018

nik9000 left a comment

Add pluggable XContentBuilder writers and human readable writers #29120

Add pluggable XContentBuilder writers and human readable writers #29120

Conversation

dakrone commented Mar 17, 2018

elasticmachine commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

rjernst left a comment •

edited

Loading