Skip to content

JENA-1378 : Set the accept header in the default use case.#271

Merged
asfgit merged 2 commits intoapache:masterfrom
afs:conneg
Jul 28, 2017
Merged

JENA-1378 : Set the accept header in the default use case.#271
asfgit merged 2 commits intoapache:masterfrom
afs:conneg

Conversation

@afs
Copy link
Copy Markdown
Member

@afs afs commented Jul 27, 2017

...while still respecting any direct HttpClient setup.

});
HttpClient hc = CachingHttpClientBuilder.create().setDefaultHeaders(hdrs).build();
return hc;
HttpClient hc = HttpOp.createPoolingHttpClientBuilder()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe just put the default headers into the clients built by HttpOp as options (e.g. HttpOp.createPoolingHttpClientBuilder(boolean useDefaultAcceptHeaderForRdf) or something like that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possibility though I like that HttpOp is neutral as to its usage - it is not RDF-specific and the default headers are none. There are different headers for graph, datasets and result sets. Responsibility for the customization is in the RDFParser which here (this is the case of the app making detailed settings) captures those choices for a reusable parser process.

Adding to the complexity of HttpOp.createPoolingHttpClient seems a slippery slope that goes against the builder pattern (IMO). It would be easier if there were an HttpComponents way to say "make a builder that would produce this HttpClient" capturing the internal settings details of an HttpClient.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged, which fixes the bug as reported. That does not preclude refining it some more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future refining, my understanding of HttpClientBuilder is that it does exactly what you describe above. IOW, you can call build() on it and get a client but keep it around to call build() on it later for another same-config client, over and over again.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is required is a builder that is based on another but isolated from it by cloning (or in some way) so that adding new builder settings only changes the clone, not also the original (the current active default in HttpOp in this case).HttpClientBuilder is a setter only style, you can't get settings from it (builder builder needed ?!). RDFparserBuilder has clone().

@asfgit asfgit merged commit b11e873 into apache:master Jul 28, 2017
asfgit pushed a commit that referenced this pull request Jul 28, 2017
@afs afs deleted the conneg branch July 28, 2017 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants