Skip to content

Bug in TermsQueryDescriptor when passing to Terms IEnumerable<TValue> #1930

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
alekole opened this issue Mar 17, 2016 · 3 comments
Closed

Bug in TermsQueryDescriptor when passing to Terms IEnumerable<TValue> #1930

alekole opened this issue Mar 17, 2016 · 3 comments
Assignees

Comments

@alekole
Copy link

alekole commented Mar 17, 2016

if i have some descriptor like
var descr = new QueryContainerDescriptor<SearchProductCard>().Terms(t => t.Field("field").Terms(new List<int> {1,2}));

it will transfer in

"terms": {
     "field": [
        [
             "1",
             "2"
         ]
      ]
   }

This causes an error in elastic

"reason": {
               "type": "number_format_exception",
               "reason": "For input string: \"[\""
            }

I have noticed that before commit e1e467e2 there was an explicit conversion to array

public TermsQueryDescriptor<T, K> Terms(IEnumerable<K> terms) => Assign(a =>  
    {
    if (terms.HasAny())
    terms = terms.Where(t => t != null).ToArray();
    a.Terms = terms.Cast<object>();
});

and missed later

public TermsQueryDescriptor<T, TValue> Terms(IEnumerable<TValue> terms) => Assign(a => a.Terms = terms.Cast<object>());
@tormodu
Copy link

tormodu commented Mar 18, 2016

I am also experiencing this using Elasticsearch 2.2.|

@gmarz gmarz added Bug labels Mar 18, 2016
@russcam
Copy link
Contributor

russcam commented Mar 19, 2016

@alekole, Thanks for reporting. The TermsQueryDescriptor<T> looks like the following

    public class TermsQueryDescriptor<T> 
        : FieldNameQueryDescriptorBase<TermsQueryDescriptor<T>, ITermsQuery, T>
        , ITermsQuery where T : class
    {
        protected override bool Conditionless => TermsQuery.IsConditionless(this);
        MinimumShouldMatch ITermsQuery.MinimumShouldMatch { get; set; }
        bool? ITermsQuery.DisableCoord { get; set; }
        IEnumerable<object> ITermsQuery.Terms { get; set; }
        IFieldLookup ITermsQuery.TermsLookup { get; set; }

        public TermsQueryDescriptor<T> TermsLookup<TOther>(Func<FieldLookupDescriptor<TOther>, IFieldLookup> selector)
            where TOther : class => Assign(a => a.TermsLookup = selector(new FieldLookupDescriptor<TOther>()));

        public TermsQueryDescriptor<T> MinimumShouldMatch(MinimumShouldMatch minMatch) => Assign(a => a.MinimumShouldMatch = minMatch);

        public TermsQueryDescriptor<T> DisableCoord(bool? disable = true) => Assign(a => a.DisableCoord = disable);

        public TermsQueryDescriptor<T> Terms<TValue>(IEnumerable<TValue> terms) => Assign(a => a.Terms = terms?.Cast<object>());

        public TermsQueryDescriptor<T> Terms<TValue>(params TValue[] terms) => Assign(a => a.Terms = terms?.Cast<object>());

    }

The problem that you are experiencing here is that the compiler is preferring the params TValue[] overload of Terms<TValue>(), incorrectly inferring the type of TValue to be List<int> instead of correctly inferring it to be int. Specifying the type of TValue explicitly will rectify this

var descr = new QueryContainerDescriptor<SearchProductCard>().Terms(t => t
    .Field("field")
    // specify generic parameter type to be int
    .Terms<int>(new List<int> {1,2})
);

The Terms<TValue>(IEnumerable<TValue> terms) overload will now be called instead.

/cc @Mpdreamz, @gmarz

Mpdreamz added a commit that referenced this issue Mar 23, 2016
When calling

```csharp
.Terms(new List<T>());
```

The user expects T[] to be written to elasticsearch but due to C#
resolution they end up calling

```csharp
.Terms<List<T>>(params List<T>)
```

 and we serialize to T[][] which is not a valid terms query.

The fluent Terms() method now tries to preempt these cases and unwraps
to T[] when a single IEnumerable is passed as T, special casing `string`
to be exluded from the unwrapping behavior so we do not unwrap string to
char[]
@Mpdreamz
Copy link
Member

Thanks for reporting this one @tormodu 👍 whilst what we did was technically correct, it is very much unexpected behaviour from a users perspective

I just opened #1948 to try an manually unwrap these cases to "the right thing".

gmarz pushed a commit that referenced this issue Mar 23, 2016
When calling

```csharp
.Terms(new List<T>());
```

The user expects T[] to be written to elasticsearch but due to C#
resolution they end up calling

```csharp
.Terms<List<T>>(params List<T>)
```

 and we serialize to T[][] which is not a valid terms query.

The fluent Terms() method now tries to preempt these cases and unwraps
to T[] when a single IEnumerable is passed as T, special casing `string`
to be exluded from the unwrapping behavior so we do not unwrap string to
char[]
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

5 participants