Refactor FieldCapabilities creation by adding a proper builder object #121310

GalLalouche · 2025-01-30T16:05:56Z

Reduce boilerplate associated with creating FieldCapabilities instances.
Since it's a class with a huge number of fields, it makes sense to define a builder object, as that can also help with all the Boolean and null blindness going on.
Note while there is a static Builder class in FieldCapabilities, it is not a proper builder object (no setters, still need to pass a lot of otherwise default parameters) and also package-private. To avoid changing that, I defined a new FieldCapabilitiesBuilder class. I also went over the code and refactored places which used the old constructor.

elasticsearchmachine · 2025-01-30T16:15:06Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

drempapis · 2025-02-20T08:32:14Z

...er-extras/src/test/java/org/elasticsearch/index/mapper/extras/FieldCapsRankFeatureTests.java

        assertThat(fooRankField, Matchers.hasKey("rank_feature"));
        assertEquals(
-            new FieldCapabilities("fooRank", "rank_feature", false, true, false, null, null, null, Collections.emptyMap()),
+            new FieldCapabilitiesBuilder("fooRank", "rank_feature").isAggregatable(false).build(),


Why don't you use the fieldCapabilities method to create a builder?

Nice catch! Done.

drempapis · 2025-02-20T08:45:14Z

test/framework/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesBuilder.java

+        return this;
+    }
+
+    public FieldCapabilitiesBuilder indices(String... indices) {


This class is used only for testing, but this could be used in the main code in the future. Should we have defensive copies of the indices passed as parameters to avoid mutating the array content?

drempapis · 2025-02-20T08:49:37Z

test/framework/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesBuilder.java

+        this.isSearchable = true;
+        this.isAggregatable = true;
+
+        this.meta = Map.of();


I think the use of Collections.emptyMap() is more appropriate for creating an immutable empty map in the consturctor.

drempapis · 2025-02-20T08:51:00Z

test/framework/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesBuilder.java

+
+    private Map<String, Set<String>> meta;
+
+    public FieldCapabilitiesBuilder(String name, String type) {


The name and type look like required fields. Should any validation be applied?

What kind of validation did you have in mind? No piece of code validates these anywhere in the AFAICT.

This is minor; I was thinking of checking for nullity. I believe the @NotNull annotation should be enough

Since this annotation is very rarely used in the code base, and since IMHO everything is not nullable by default, unless explicitly marked as @Nullable, let's keep it as it is.

drempapis · 2025-02-20T08:52:20Z

test/framework/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesBuilder.java

+        return this;
+    }
+
+    public FieldCapabilitiesBuilder meta(Map<String, Set<String>> meta) {


As previously, should the map be copied?

drempapis · 2025-02-20T08:53:45Z

...c/test/java/org/elasticsearch/xpack/ml/dataframe/extractor/ExtractedFieldsDetectorTests.java

                    type,
-                    new FieldCapabilities(field, type, isMetadataField, true, isAggregatable, null, null, null, Collections.emptyMap())
+                    new FieldCapabilitiesBuilder(field, type).isMetadataField(isMetadataField)
+                        .isSearchable(true)


This is optional

drempapis · 2025-02-20T08:56:36Z

.../plugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolverTests.java

-            )
-        );
+        multi.put("long", new FieldCapabilitiesBuilder(fieldName, "long").indices("one-index").build());
+        multi.put("text", new FieldCapabilitiesBuilder(fieldName, "text").indices("another-index").build());


Should the method be called isAggregatable(false) in the builder?

Good catch! Done.

drempapis · 2025-02-20T08:59:23Z

Thank you, @GalLalouche, for working on this. The readability of the code has been increased, avoiding the 'telescoping constructor' annoying issue. I've left some minor comments to address.

drempapis

LGTM, thank you @GalLalouche for this update

…elastic#121310) Reduce boilerplate associated with creating `FieldCapabilities` instances. Since it's a class with a huge number of fields, it makes sense to define a builder object, as that can also help with all the Boolean and null blindness going on. Note while there is a static Builder class in `FieldCapabilities`, it is not a proper builder object (no setters, still need to pass a lot of otherwise default parameters) and also package-private. To avoid changing that, I defined a new `FieldCapabilitiesBuilder` class. I also went over the code and refactored places which used the old constructor.

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 labels Jan 30, 2025

GalLalouche added >non-issue >refactoring and removed >non-issue labels Jan 30, 2025

GalLalouche requested a review from jimczi January 30, 2025 16:12

benwtrent added the :Search Foundations/Search Catch all for Search Foundations label Jan 30, 2025

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Jan 30, 2025

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Jan 30, 2025

GalLalouche removed the request for review from jimczi January 30, 2025 17:03

GalLalouche force-pushed the tests/field_caps_refactor branch from c87de3e to 09f5c33 Compare January 30, 2025 17:57

GalLalouche added 3 commits January 30, 2025 19:57

FieldCapabilitiesBuilder

ff82a3b

Revert change to non-test class

8acb415

Move builder to test framework module

085ae87

GalLalouche force-pushed the tests/field_caps_refactor branch from 09f5c33 to 085ae87 Compare January 30, 2025 17:57

drempapis reviewed Feb 20, 2025

View reviewed changes

Merge branch 'main' into tests/field_caps_refactor

f2ca180

GalLalouche force-pushed the tests/field_caps_refactor branch 2 times, most recently from 25ab4b5 to 3211c62 Compare March 3, 2025 13:23

GalLalouche requested a review from drempapis March 3, 2025 14:31

Merge branch 'main' into tests/field_caps_refactor

a1c5eaf

GalLalouche force-pushed the tests/field_caps_refactor branch from 9767d5f to cbc52e5 Compare March 3, 2025 14:36

elastic deleted a comment from cla-checker-service bot Mar 3, 2025

GalLalouche added 2 commits March 3, 2025 16:38

Code review fixes.

923a924

Fix test, some more defensive copying.

d5bd523

GalLalouche force-pushed the tests/field_caps_refactor branch from cbc52e5 to d5bd523 Compare March 3, 2025 14:38

Fix test

04c3bf9

drempapis approved these changes Mar 4, 2025

View reviewed changes

Merge branch 'main' into tests/field_caps_refactor

2723656

GalLalouche enabled auto-merge (squash) March 5, 2025 11:04

GalLalouche merged commit a6e47ae into elastic:main Mar 5, 2025
17 checks passed

GalLalouche mentioned this pull request Mar 19, 2025

Refactor IndexFieldCapabilities creation by adding a proper builder object #125219

Merged


		private Map<String, Set<String>> meta;

		public FieldCapabilitiesBuilder(String name, String type) {

Refactor FieldCapabilities creation by adding a proper builder object #121310

Refactor FieldCapabilities creation by adding a proper builder object #121310

Uh oh!

Conversation

GalLalouche commented Jan 30, 2025

Uh oh!

elasticsearchmachine commented Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drempapis commented Feb 20, 2025

Uh oh!

drempapis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants