Replace Request#setHeaders with addHeader #30588

javanna · 2018-05-14T19:01:28Z

Adding headers rather than setting them all at once seems more
user-friendly and we already do it in a similar way for parameters
(see Request#addParameter).

Adding headers rather than setting them all at once seems more user-friendly and we already do it in a similar way for parameters (see Request#addParameter).

elasticmachine · 2018-05-14T19:01:30Z

Pinging @elastic/es-core-infra

javanna · 2018-05-14T19:02:26Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

     */
-    public void setHeaders(Header... headers) {
+    public void addHeaders(Header... headers) {


I was undecided on leaving the varargs or not. This way it breaks existing code much less. And it is kind of convenient. But it can be seen as confusing I guess.

nik9000 · 2018-05-15T13:45:49Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

-            Objects.requireNonNull(header, "header cannot be null");
-        }
-        this.headers = headers;
+    public void addHeader(Header header) {


What about addHeader(String key, String value) instead? I don't see us using anything but BasicHeader anyway and I like exposing a little less of httpclient publicly. And I like not having to import another class everywhere.

yea I think it's a good idea

nik9000 · 2018-05-15T13:46:58Z

client/rest/src/test/java/org/elasticsearch/client/RequestTests.java

@@ -127,31 +128,26 @@ public void testSetJsonEntity() throws IOException {
        assertEquals(json, new String(os.toByteArray(), ContentType.APPLICATION_JSON.getCharset()));
    }

-    public void testSetHeaders() {
+    public void testAddHeaders() {


testAddHeader?

javanna · 2018-05-17T08:46:20Z

@nik9000 would you mind reviewing this?

nik9000 · 2018-05-17T13:49:07Z

@nik9000 would you mind reviewing this?

Sorry, I didn't realize you were ready for another round. Right away!

nik9000 · 2018-05-17T13:52:13Z

client/rest/src/main/java/org/elasticsearch/client/Request.java

+        return Objects.hash(method, endpoint, parameters, entity, headers.hashCode(), httpAsyncResponseConsumerFactory);
+    }
+
+    static final class ReqHeader extends BasicHeader {


Did you do it so we'd have a nice equals and hashcode? Maybe it'd be better to iterate by hand in those cases rather than make this. I'm not sure!

yes I realized that BasicHeader doesn't have those and we were previously relying on the fact that the objects are the same. Since we now create the header internally, this breaks all our header tests. I initially went without this additional class but the needed code in Request#equals and Request#hashCode was quite ugly. I find this second approach more readable.

++. Stick some javadoc on the class to explain that and merge away!

* master: QA: Add xpack tests to rolling upgrade (#30795) Modify state of VerifyRepositoryResponse for bwc (#30762) Reduce CLI scripts to one-liners on Windows (#30772) Simplify number of shards setting (#30783) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) [Docs] Fix script-fields snippet execution (#30693) Upgrade to Lucene-7.4.0-snapshot-cc2ee23050 (#30778) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) Make http pipelining support mandatory (#30695) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) Increase the maximum number of filters that may be in the cache. (#30655) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Add more yaml tests for get alias API (#29513) Ignore empty completion input (#30713) [DOCS] fixed incorrect default [ML] Filter undefined job groups from update calendar actions (#30757) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Enable installing plugins from snapshots.elastic.co (#30765) Remove fedora 26, add 28 (#30683) Accept Gradle build scan agreement (#30645) Remove logging from elasticsearch-nio jar (#30761) Add Delete Repository High Level REST API (#30666)

Adding headers rather than setting them all at once seems more user-friendly and we already do it in a similar way for parameters (see Request#addParameter).

* 6.x: [DOCS] Fixes typos in security settings Add support for indexed shape routing in geo_shape query (#30760) [DOCS] Splits auditing.asciidoc into smaller files Painless: Types Section Clean Up (#30283) [test] java tests for archive packaging (#30734) Deprecate http.pipelining setting (#30786) [DOCS] Fix more edit URLs in Stack Overview (#30704) Use correct cluster state version for node fault detection (#30810) [DOCS] Fixes broken link for native realm [DOCS] Clarified audit.index.client.hosts (#30797) Change serialization version of doc-value fields. Add a `format` option to `docvalue_fields`. (#29639) [TEST] Don't expect acks when isolating nodes Fixes UpdateSettingsRequestStreamableTests mutate bug Revert "Add more yaml tests for get alias API (#29513)" Revert "Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled" Only allow x-pack metadata if all nodes are ready (#30743) Mutes MachineLearningTests.testNoAttributes_givenSameAndMlEnabled Use original settings on full-cluster restart (#30780) Only ack cluster state updates successfully applied on all nodes (#30672) Replace Request#setHeaders with addHeader (#30588) [TEST] remove endless wait in RestClientTests (#30776) QA: Add xpack tests to rolling upgrade (#30795) Add support for search templates to the high-level REST client. (#30473) Reduce CLI scripts to one-liners on Windows (#30772) Fold RestGetAllSettingsAction in RestGetSettingsAction (#30561) Add more yaml tests for get alias API (#29513) [Docs] Fix script-fields snippet execution (#30693) Convert FieldCapabilitiesResponse to a ToXContentObject. (#30182) Remove assert statements from field caps documentation. (#30601) Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (#30181) Add support for field capabilities to the high-level REST client. (#29664) [DOCS] Add SAML configuration information (#30548) [DOCS] Remove X-Pack references from SQL CLI (#30694) [Docs] Fix typo in circuit breaker docs (#29659) [Feature] Adding a char_group tokenizer (#24186) Increase the maximum number of filters that may be in the cache. (#30655) [Docs] Fix broken cross link in documentation Test: wait for netty threads in a JUnit ClassRule (#30763) [Security] Include an empty json object in an json array when FLS filters out all fields (#30709) [DOCS] fixed incorrect default [TEST] Wait for CS to be fully applied in testDeleteCreateInOneBulk Enable installing plugins from snapshots.elastic.co (#30765) Ignore empty completion input (#30713) Fix docs failure on language analyzers (#30722) [Docs] Fix inconsistencies in snapshot/restore doc (#30480) Add Delete Repository High Level REST API (#30666) Reduce CLI scripts to one-liners (#30759)

javanna added 3 commits May 14, 2018 20:53

Replace Request#setHeaders with addHeaders

4d53709

Adding headers rather than setting them all at once seems more user-friendly and we already do it in a similar way for parameters (see Request#addParameter).

remove empty line

7ce5692

fix more issues

608b139

javanna added >enhancement :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v7.0.0 v6.4.0 labels May 14, 2018

javanna requested a review from nik9000 May 14, 2018 19:01

javanna commented May 14, 2018

View reviewed changes

javanna added 4 commits May 15, 2018 09:56

Merge branch 'master' into enhancement/request_add_headers

299cf3b

Remove varargs argument, replace addHeaders with addHeader

d953894

fixes

f4116ad

remove unused imports

5b91a56

nik9000 reviewed May 15, 2018

View reviewed changes

javanna added 3 commits May 15, 2018 17:07

addHeader(String,String)

c9a560a

fix tests

6e456e9

rename test method

e3c74c2

nik9000 approved these changes May 17, 2018

View reviewed changes

javanna added 5 commits May 22, 2018 12:08

Merge branch 'master' into enhancement/request_add_headers

5a395f9

add javadocs for ReqHeader

a2721b5

fix compile error

ed19946

fix compile errors

61e4361

fix failing test

08e1d41

javanna changed the title ~~Replace Request#setHeaders with addHeaders~~ Replace Request#setHeaders with addHeader May 22, 2018

javanna merged commit a17d6ca into elastic:master May 22, 2018

javanna added the backport pending label May 22, 2018

javanna added a commit that referenced this pull request May 23, 2018

Replace Request#setHeaders with addHeader (#30588)

946f519

Adding headers rather than setting them all at once seems more user-friendly and we already do it in a similar way for parameters (see Request#addParameter).

javanna removed the backport pending label May 23, 2018

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

Replace Request#setHeaders with addHeader #30588

Replace Request#setHeaders with addHeader #30588

javanna commented May 14, 2018

elasticmachine commented May 14, 2018

javanna May 14, 2018

nik9000 May 15, 2018

javanna May 15, 2018

nik9000 May 15, 2018

javanna commented May 17, 2018

nik9000 commented May 17, 2018

nik9000 May 17, 2018

javanna May 17, 2018

nik9000 May 17, 2018

Replace Request#setHeaders with addHeader #30588

Replace Request#setHeaders with addHeader #30588

Conversation

javanna commented May 14, 2018

elasticmachine commented May 14, 2018

javanna May 14, 2018

Choose a reason for hiding this comment

nik9000 May 15, 2018

Choose a reason for hiding this comment

javanna May 15, 2018

Choose a reason for hiding this comment

nik9000 May 15, 2018

Choose a reason for hiding this comment

javanna commented May 17, 2018

nik9000 commented May 17, 2018

nik9000 May 17, 2018

Choose a reason for hiding this comment

javanna May 17, 2018

Choose a reason for hiding this comment

nik9000 May 17, 2018

Choose a reason for hiding this comment