-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Client shouldn't silently fail if index name is wrong #1017
Comments
It wont silently fail (.IsValid is still false) but we could/should totally throw an ArgumentException in this case. There are a couple of rules to index names: ditto with repository names and others that we could obey better in the client. Will add first thing in the morning. |
Already added, see PR :) Didn't check Didn't add any tests, cause I couldn't find where you had all the other non integration create index tests. A simple test method should be something like
|
Pulled it in but rewrote it to mimic elasticsearch's validation and moved the validation in an appropriate place where all the CreateIndex() overloads pass through. In general NEST favours error messages from the server but this is especially confusing to people completely new to NEST and elasticsearch that are just getting started. @gmarz we could also move this validation to whenever an Thanks for reporting @mastoj 👍 |
No probs. @Mpdreamz |
I'm leaning towards not throwing an exception here at all. While I see the benefits (failing early without making a trip to ES) this is technically an Elasticsearch exception and the responsibility of Elasticsearch to determine if an index name is valid. IMO it would be better to stay consistent with how we handle Elasticsearch exceptions everywhere else- that is IsValid = false and reporting back the exception (unless ThrowOnElasticsearchExceptions == true). This also adds extra maintenance to the client, as we'll have to keep our validation rules in line with ES in case they ever change. Just my two cents. If you guys still feel strongly that NEST should validate and throw, then yea I would say |
That has been my philosophy as well but thinking as someone completely new to elasticsearch and NEST will most likely name their indexes in PascalCase and then not know where to look for the answer in the response. I feel in this case we should make an exception to the rule, will let it bake in my head over the weekend and perhaps see what the guys from the other clients think/do as well. |
I do agree with scenarios. It's always good to be consistent, but at the same time it sucks with failing when creating the index. The problem with the model as I see it now is that I don't think users look at the result when they are not querying, and that makes it hard to find these kinds of error since you write the Index request as:
At the same time, this is a rule in Elasticsearch and the rule should be there I think. Would it be possible to do the actual request for Index and then throw an exception if the request failed? I know this should not be done in general, but that might be a valid solution for the index case? |
@gmarz where are you at with throwing an exception here? I like @mastoj idea for throwing the exception when the request has failed but my vote still goes to doing clientside validation in this case. Elasticsearch only validates this on index creation as well and simply throws a 404 if you use an invalid index name later on. |
@Mpdreamz @mastoj I'm still a bit torn on how to handle this. While I completely get that it might be confusing for the user, I'm still leaning towards not throwing for a few reasons:
|
@mastoj Just to keep you in the loop, we ended up deciding not to implement any client side validation for index names for the above reasons. We also checked with the other clients, and confirmed that they also do not do any validation of this sort. |
Ok. Might be a good decision in the long run. -----Original Message----- @mastoj Just to keep you in the loop, we ended up deciding not to implement any client side validation for index names for the above reasons. We also checked with the other clients, and confirmed that they also do not do any validation of this sort. |
When preparing for a demo I tried to name an index with a capital letter. It took me a while to figure out what was wrong since I didn't get any indicators that it was wrong.
Two solution as I see it:
I would go for number one since it is more explicit and probably will guide the developers the right way.
The text was updated successfully, but these errors were encountered: