Skip to content

GetAsync fails when specifying a null index. #1980

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

Closed
niemyjski opened this issue Apr 1, 2016 · 14 comments
Closed

GetAsync fails when specifying a null index. #1980

niemyjski opened this issue Apr 1, 2016 · 14 comments
Assignees

Comments

@niemyjski
Copy link
Contributor

We have some generic code that runs and trys to specify an index for getbyid but if it can't be found it returns null. This worked in <2.0 of the client I believe this is a bug.

Fails:

var response = await elasticClient.GetAsync<T>(id, s => s.Index(null));


System.ArgumentException

Dispatching Get() from NEST into to Elasticsearch.NET failed
Received a request marked as GET
This endpoint accepts GET
The request might not have enough information provided to make any of these endpoints:
  - /{index=<NULL>}/{type=employee}/{id=56fed5baae14593188842ee5}


   at Nest.LowLevelDispatch.GetDispatchAsync[T](IRequest`1 p) in C:\users\russ\source\elasticsearch-net\src\Nest\_Generated\_LowLevelDispatch.generated.cs:line 932
   at Nest.ElasticClient.<Nest-IHighLevelToLowLevelDispatcher-DispatchAsync>d__24`4.MoveNext() in C:\users\russ\source\elasticsearch-net\src\Nest\ElasticClient.cs:line 75
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Foundatio.Elasticsearch.Repositories.ElasticReadOnlyRepositoryBase`1.<GetByIdAsync>d__15.MoveNext() in C:\Code\Foundatio.Repositories\src\Elasticsearch\Repositories\ElasticReadOnlyRepositoryBase.cs:line 196

Works:

var response = await elasticClient.GetAsync<T>(id);
@niemyjski
Copy link
Contributor Author

If you pull down foundatio repositories project and run the tests you'll run into this.

@gmarz
Copy link
Contributor

gmarz commented Apr 1, 2016

You need to specify an index when getting a document. This exception is telling you exactly that. If it worked in 1.x then it was wrong :).

@niemyjski
Copy link
Contributor Author

If I pass null to the index that's the same as not specifying one.. They should work the same

@niemyjski
Copy link
Contributor Author

I feel pretty strongly that this is a bug in the client

@gmarz
Copy link
Contributor

gmarz commented Apr 1, 2016

When you don't specify an index, it's being inferred from T. Passing null explicitly is overriding the inferred index.

@niemyjski
Copy link
Contributor Author

I get this but what use case would you ever pass null in here and get a valid query?

@niemyjski
Copy link
Contributor Author

And if you'd never get a valid query then, should we throw a argumentnullexception? This difference really threw me off guard. As a lot of tests started failing.

@gmarz
Copy link
Contributor

gmarz commented Apr 1, 2016

I get this but what use case would you ever pass null in here and get a valid query?

Exactly. This is why we throw the exception when you pass it null.

I get your point as well, but we try to generalize this logic for all API calls within LowLevelDispatch, so if any required part of the path is missing we throw. For some calls it's valid to override the inferred index and set it to null (_search for instance).

@niemyjski
Copy link
Contributor Author

The stack trace is terrible due to how its executed :(

@gmarz
Copy link
Contributor

gmarz commented Apr 1, 2016

But

System.ArgumentException

Dispatching Get() from NEST into to Elasticsearch.NET failed
Received a request marked as GET
This endpoint accepts GET
The request might not have enough information provided to make any of these endpoints:
  - /{index=<NULL>}/{type=employee}/{id=56fed5baae14593188842ee5}

Is pretty descriptive :)

@niemyjski
Copy link
Contributor Author

Yes, it's much better than what it is but kinda of misleading until your used to all the new stack traces. I was expecting a request to be made and failed so I was looking in fiddler first ready to grab the request json.

@gmarz
Copy link
Contributor

gmarz commented Apr 1, 2016

Gotcha. Going to close this. Please do keep opening issues though as you run into things. 👍

@gmarz gmarz closed this as completed Apr 1, 2016
@gmarz
Copy link
Contributor

gmarz commented Apr 4, 2016

@niemyjski I stand corrected here :)

Discussed this further with @Mpdreamz and @russcam and it makes sense for .Index(null) to behave as if it was never called. This is a general rule the fluent API should follow everywhere.

@gmarz gmarz reopened this Apr 4, 2016
@gmarz gmarz added Bug labels Apr 4, 2016
Mpdreamz added a commit that referenced this issue Apr 12, 2016
```csharp
var response = await elasticClient.GetAsync<T>(id, s =>
  s.Index(null));
```

Would not default index back to inferred type of T.

This PR makes specifying null for required route values a noop

For optional routevalues null still clears the routevalue so the
following still works

```csharp
c => c.Index(project, i => i.Id(null))
```

will reset the id on and send the index request to
`/inferredindex/inferredtype`
gmarz added a commit that referenced this issue Apr 12, 2016
…lues

fix #1980 GetAsync fails when specifying a null index.
@gmarz
Copy link
Contributor

gmarz commented Apr 12, 2016

Closed via #2030

@gmarz gmarz closed this as completed Apr 12, 2016
Mpdreamz added a commit that referenced this issue Apr 12, 2016
```csharp
var response = await elasticClient.GetAsync<T>(id, s =>
  s.Index(null));
```

Would not default index back to inferred type of T.

This PR makes specifying null for required route values a noop

For optional routevalues null still clears the routevalue so the
following still works

```csharp
c => c.Index(project, i => i.Id(null))
```

will reset the id on and send the index request to
`/inferredindex/inferredtype`
Mpdreamz added a commit that referenced this issue Jul 26, 2016
```csharp
var response = await elasticClient.GetAsync<T>(id, s =>
  s.Index(null));
```

Would not default index back to inferred type of T.

This PR makes specifying null for required route values a noop

For optional routevalues null still clears the routevalue so the
following still works

```csharp
c => c.Index(project, i => i.Id(null))
```

will reset the id on and send the index request to
`/inferredindex/inferredtype`
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

No branches or pull requests

3 participants