Skip to content
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

DefaultMappingFor IndexName is not respected in searches #8184

Closed
Genide opened this issue May 8, 2024 · 3 comments
Closed

DefaultMappingFor IndexName is not respected in searches #8184

Genide opened this issue May 8, 2024 · 3 comments
Labels
8.x Relates to 8.x client version Category: Not an issue

Comments

@Genide
Copy link

Genide commented May 8, 2024

Elastic.Clients.Elasticsearch version: 8.13.12

Elasticsearch version: 8.5.3

.NET runtime version: DOTNET 8

Operating system version: Windows 10 64 bit

Description of the problem including expected versus actual behavior:
Setting a default index name for a given type no longer works. This behavior is what I expected in Elastic.Clients.Elasticsearch version 8.13.11.

Steps to reproduce:

  1. Create an ElasticsearchClient with client settings that include a default mapping for index names.
   var connectionSettings = new ElasticsearchClientSettings(options.Value.CloudId, new ApiKey(options.Value.ApiKey))
       .DefaultMappingFor<Branch>(m => m.IndexName("branches"))
       .EnableDebugMode();

   return new ElasticsearchClient(connectionSettings);
  1. Use the ElasticsearchClient to search for an object with the mapped index name.
       var result = await _client.SearchAsync<Branch>(s => s
           .Query(q => q
               .Term(t => t.Field(f => f.FinancialInstitutionId.Suffix("keyword")).Value(fiid))));
       return result.Documents;
  1. See results being returned from all indexes. This is a snippet from result.DebugInformation. Notice how the URL path does not include the index name.

Valid Elasticsearch response built from a successful (200) low level call on POST: /_search?pretty=true&error_trace=true

Expected behavior
I expected the following message in result.DebugInformation.

Valid Elasticsearch response built from a successful (200) low level call on POST: /branches/_search?pretty=true&error_trace=true

Provide ConnectionSettings (if relevant): NA

Provide DebugInformation (if relevant):

Valid Elasticsearch response built from a successful (200) low level call on POST: /_search?pretty=true&error_trace=true

# Audit trail of this API call:
 - [1] HealthyResponse: Node:  OMITTING NODE NAME Took: 00:00:04.9044904
# Request:
{
  "query": {
    "term": {
      "fiid.keyword": {
        "value": "SANDBOX"
      }
    }
  }
}
# Response:
{
  "took" : 4805,
  "timed_out" : false,
  "_shards" : {
    "total" : 57,
    "successful" : 57,
    "skipped" : 46,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 8250,
      "relation" : "eq"
    },
    "max_score" : 11.089042,
    "hits" : [
      {
        "_index" : "branches",
        "_id" : "Rzv3AokBFOgoV9vSRWhT",
        "_score" : 11.089042,
        "_source" : {
          "fiid" : "SANDBOX",
          "branchId" : 0,
          "lat" : 29.8,
          "lon" : -95.4,
          "location" : {
            "lat" : 29.8,
            "lon" : -95.4
          }
        }
      },
      {
        "_index" : "branches_dev",
        "_id" : "Rzv3AokBFOgoV9vSRWhT",
        "_score" : 11.089042,
        "_source" : {
          "fiid" : "SANDBOX",
          "branchId" : 0,
          "lat" : 29.8,
          "lon" : -95.4,
          "location" : {
            "lat" : 29.8,
            "lon" : -95.4
          }
        }
      },
      {
        "_index" : "financial_institutions_dev",
        "_id" : "SANDBOX",
        "_score" : 8.753898,
        "_source" : {
          "fiid" : "SANDBOX",
          "name" : "Dummy Financial"
        }
      },
      {
        "_index" : "financial_institutions",
        "_id" : "SANDBOX",
        "_score" : 8.753687,
        "_source" : {
          "fiid" : "SANDBOX",
          "name" : "Dummy Financial"
        }
      },
    // Omitting the remainder for brevity
    ]
  }
}

# TCP states:
  Established: 301
  TimeWait: 179
  SynSent: 1
  CloseWait: 3

# ThreadPool statistics:
  Worker: 
    Busy: 1
    Free: 32766
    Min: 8
    Max: 32767
  IOCP: 
    Busy: 0
    Free: 1000
    Min: 1
    Max: 1000
@flobernd
Copy link
Member

flobernd commented May 9, 2024

@Genide The behavior of 8.13.12 is the intended behavior. 8.13.11 had a bug that caused the index to be inferred. The versions prior to 8.13.x as well do not infer the index here and there is a reason for that.

So, why don't we want to infer the index in this case?

Inferring the index for mandatoy IndexName (singular) arguments is not an issue, because these arguments are required to execute the query. At the same time, the query always operates on a single index (e.g. the default index for the entity) which means there is no ambiguity in using the configured default index.

Inferring the index for optional Indices (plural) arguments is problematic. Usually these are used in endpoints where you can either set different indices on operation/element level (e.g. MultiGet, MultiSearch, Bulk, ...) or to generally restrict the action to specific indicies (note, that in these cases, it's completely valid to specify no index as an alternative to all). Examples for this would be the mentioned Search endpoint.

One of our main principles is to prefer explicit over implicit. If we would infer an index here, this would cause unexpected behavior, if you have multiple entities of e.g. Branch stored in different indices. You would have to "undo" the implicit changes by settings the optional indices argument to all. The default way (again, note that the "indices" argument is optional) for Search is to query all indices.

I'm going to close this issue here, but feel free to add further comments or open new issues, if you have other concerns.

@Genide
Copy link
Author

Genide commented May 9, 2024

Then is the use of TConnectionSettings DefaultMappingFor<TDocument> and ClrTypeMappingDescriptor<TDocument> IndexName obsolete?

@flobernd
Copy link
Member

flobernd commented May 9, 2024

@Genide No, it's still working - just not for optional Indices pararameter 🙂 It works for all other cases like e.g. Get or most index management APIs, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Relates to 8.x client version Category: Not an issue
Projects
None yet
Development

No branches or pull requests

2 participants