-
-
Notifications
You must be signed in to change notification settings - Fork 107
Use mime type from config field in Elasticsearch indexer while posting HTTP request #41
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
Changes from all commits
a359903
117ee91
d6e2d73
7576829
4146ffe
722137e
6a670a5
4fabced
e67b6a8
ed77e45
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 |
|---|---|---|
|
|
@@ -48,6 +48,16 @@ public synchronized T getInstance() | |
| return instance; | ||
| } | ||
|
|
||
| /** | ||
| * Purpose of this method is to validate the inputs (if required) and if found | ||
| * erroneous throw an exception so that it will be bubbled up to the UI. | ||
| * | ||
| * @throws Exception | ||
| */ | ||
| public void validate() throws Exception { | ||
|
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. [minor] it would be better to have something more specific than Exception in the signature (e.g. ConstraintViolationException); OTOH it would force us to catch and wrap MimeTypeParseException, so I'm not sure if it's worth it
Author
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.
Thats the reason I didn't use specific exception there. In this case I feel we should not bother about the specific exception. If validate() throws any exception we know it is due to invalid indexer config so simply displaying wrapped exception would be sufficient . Let me know if you feel there is a value in that I can make a change. |
||
| } | ||
|
|
||
|
|
||
|
|
||
| /** | ||
| * Creates a new {@link AbstractLogstashIndexerDao} instance corresponding to this configuration. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,27 +44,32 @@ | |
| import java.net.MalformedURLException; | ||
| import java.net.URI; | ||
| import java.net.URISyntaxException; | ||
|
|
||
| import java.nio.charset.Charset; | ||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| import com.google.common.collect.Range; | ||
|
|
||
| import jenkins.plugins.logstash.configuration.ElasticSearch; | ||
|
|
||
|
|
||
| /** | ||
| * Elastic Search Data Access Object. | ||
| * | ||
| * @author Liam Newman | ||
| * @since 1.0.4 | ||
| */ | ||
| public class ElasticSearchDao extends AbstractLogstashIndexerDao { | ||
|
|
||
| private final HttpClientBuilder clientBuilder; | ||
| private final URI uri; | ||
| private final String auth; | ||
| private final Range<Integer> successCodes = closedOpen(200,300); | ||
|
|
||
| private String username; | ||
| private String password; | ||
| private String mimeType; | ||
|
|
||
|
|
||
| //primary constructor used by indexer factory | ||
| public ElasticSearchDao(URI uri, String username, String password) { | ||
|
|
@@ -102,11 +107,11 @@ public ElasticSearchDao(URI uri, String username, String password) { | |
| clientBuilder = factory == null ? HttpClientBuilder.create() : factory; | ||
| } | ||
|
|
||
|
|
||
| public URI getUri() | ||
| { | ||
| return uri; | ||
| } | ||
|
|
||
| public String getHost() | ||
| { | ||
| return uri.getHost(); | ||
|
|
@@ -136,16 +141,27 @@ public String getKey() | |
| { | ||
| return uri.getPath(); | ||
| } | ||
|
|
||
|
|
||
| public String getMimeType() { | ||
| return this.mimeType; | ||
| } | ||
|
|
||
| public void setMimeType(String mimeType) { | ||
| this.mimeType = mimeType; | ||
| } | ||
|
|
||
| String getAuth() | ||
| { | ||
| return auth; | ||
| } | ||
|
|
||
| protected HttpPost getHttpPost(String data) { | ||
| HttpPost postRequest; | ||
| postRequest = new HttpPost(uri); | ||
| StringEntity input = new StringEntity(data, ContentType.APPLICATION_JSON); | ||
| HttpPost getHttpPost(String data) { | ||
| HttpPost postRequest = new HttpPost(uri); | ||
| String mimeType = this.getMimeType(); | ||
| // char encoding is set to UTF_8 since this request posts a JSON string | ||
| StringEntity input = new StringEntity(data, StandardCharsets.UTF_8); | ||
| mimeType = (mimeType != null) ? mimeType : ContentType.APPLICATION_JSON.toString(); | ||
|
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. mimeType will never be null. If you save the global configuration with no content for the mimeType you will get an empty string. Only when you update the plugin without ever saving the configuration it will be null. |
||
| input.setContentType(mimeType); | ||
| postRequest.setEntity(input); | ||
| if (auth != null) { | ||
| postRequest.addHeader("Authorization", "Basic " + auth); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| <div> | ||
| <p> | ||
| MIME type of the request body that is sent to ELASTICSEARCH indexer. It should be of the form <b>type/subtype</b> e.g. <i>application/json</i><br> | ||
| Since this is a field for MIME Type and not Content-Type we do not support additional content-type paramters like <b>charset</b> or <b>boundry</b> | ||
| </p> | ||
| </div> |
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 think a blank mimeType should be valid. In that case it should be set to null in the DAO so you have the old behaviour. This should be mentioned in the help.
You can also save with a blank mimeType which would result in no mimeType being configured in the StringEntity. Not sure if this will break anything,
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.
@mwinter69 IMHO, I think we should not allow user to save config with blank mimeType and it would not be of much help/value addition if we do not configure mimeType on StringEntity considering the nature of elasticsearch and logstash. I think we should set application/json by default which will make both ES and logstash happy.
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.
Does this also fail when you try to save with an empty string? If not then you can run into problems at runtime as you will pass an empty string. Maybe use Util.fixEmptyAndTrim in the DAO to avoid this or do not allow to save with an illegal mimetype.
Uh oh!
There was an error while loading. Please reload this page.
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.
@mwinter69 Apologies for delays in response. Recovering from bad health. Correct me if I mis-understand understand your question. As per current logic, user is not allowed to save an empty string for mimeType. It doesn't displays an error if user tries to save empty/invalid mimeType.
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.
Go to the configuration page and make the mimetype field empty. When you leave the field it will tell you that a value is required. Now click on apply. Congratulations you have set the mimetype to an empty string which will most likely fail.
You should throw an exception in the setter method if no valid mimetype is given.
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.
Looking into this it seems that Stapler is swallowing the exception and only logging when you try to use a setter method,
By initializing the mimetype in the constructor, and throwing the exception there you can avoid that it gets saved.
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.
Thanks for the inputs. I am hoping that I understood your suggestion correctly. Here is what I did. I initialized
mimeTypeinElasticSearchDao()constructor and if it is invalid, thrown an exception just like how its done foruri.toURL();currently. However, I realized thatElasticSeachDaois not instantiated every time we click on "Save" button on jenkins configuration page. Hence throwing exception in sideElasticSearchDao()did not work for me meaning UI does not show any exception. It only throws exception when you run a job.In an alternative approach and also you mentioned this in one of your earlier comments that we can allow blank
mimeType. If themimeTypeis blank we can set it tonullinElasticSearchand with current implementation ifmimeTypeisnullit will be set to defaultapplication/jsonvalue. Does this sound acceptable?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.
The dao is the wrong place to do the checks. You have to do it in the constructor of
configuration/ElasticSearch. Alternatively one could extend the base classLogstashIndexerwith a validate method and call the validate method in theLogstashConfiguration.configurebeforesave()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.
@mwinter69 Pushed updated code.
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.
@jakub-bochenski Updated code with your review comments