Skip to content

Support Ids in MultiTermVectors API #3382

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

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Support Ids in MultiTermVectors API #3382

merged 1 commit into from
Aug 29, 2018

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Aug 29, 2018

This commit adds support for providing a set of Ids to MultiTermVectors API to be used in
conjunction with index and type provided in the URI.

Index() and Type() methods added to MultiTermVectorOperation to allow the default typeof(T)
values to be overidden.

Closes #3219

This commit adds support for providing a set of Ids to MultiTermVectors API to be used in
conjunction with index and type provided in the URI.

Index() and Type() methods added to MultiTermVectorOperation to allow the default typeof(T)
values to be overidden.

Closes #3219

protected override object ExpectJson { get; } = new
{
ids = Developer.Developers.Select(p => (Id)p.Id).Take(2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Balmer-object orientation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about providing overloads for string, long, Guid, etc. much like the existing GetMany<T>(...).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be useful, but easy to add later :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what I err'ed on!

protected override void ExpectResponse(IMultiTermVectorsResponse response)
{
response.ShouldBeValid();
response.Documents.Should().NotBeEmpty().And.HaveCount(2).And.OnlyContain(d => d.Found);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And.OnlyContain... nice!

get { return this._operations; }
set { this._operations = value?.ToList(); }
get => this._operations;
set => this._operations = value?.ToList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, is there any reason why we .ToList() on setters like this; other than to force the execution of the IEnumerable at this point? Seems like we could save some allocations in these instances...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about this specific case, but in general would be to force execution as you say.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is unusual with respect to the general API design in NEST, is that GetMany<T>(..) is additive rather than assignative to the operations. I understand why this is, and it's not the only place where a method call is additive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mpdreamz - would be able to offer any insight?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Places where we are additive is AFAIK only in the bulk related API helpers since you can IndexMany and then IndexMany<Y> afterwards in a single bulk. That was the rationale behind the deviation. Definitely up for further discussion 😄

Copy link
Contributor

@codebrain codebrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. ++ on documentation. Minor question raised on the .ToList() setter, but its a common pattern already, so no need to hold back the code for this.

@russcam russcam merged commit cefcf74 into 6.3 Aug 29, 2018
@Mpdreamz
Copy link
Member

build seed:58181 test "multitermvectorsids fails with:

   Tests.Document.Multiple.MultiTermVectors.MultiTermVectorsIdsApiTests.SerializesInitializer [FAIL]
      Expected True because RoundTrip: 0 itteration
      Deserialization has result: False
      RoundTripResult`1 success: False
      This is the first time I am serializing
        {
          "ids": [
      -     {},
      -     {}
      +     1001,
      +     1002
          ]
        }
      
       C# approximation of actual ------ 
       new {
        ids = new [] {
          1001,
          1002
        }
      };
       C# approximation of expected ------ 
       new {
        ids = new [] {
          {},
          {}
        }
      };, but found False.
      Stack Trace:
           at FluentAssertions.Execution.XUnit2TestFramework.Throw(String message)
           at FluentAssertions.Execution.AssertionScope.FailWith(String message, Object[] args)
           at FluentAssertions.Primitives.BooleanAssertions.BeTrue(String because, Object[] becauseArgs)
        C:\projects\elasticsearch-net\src\Tests\Tests.Core\Extensions\SerializationTesterAssertionExtensions.cs(9,0): at Tests.Core.Extensions.SerializationTesterAssertionExtensions.ShouldBeValid(SerializationResult result, String message)
        C:\projects\elasticsearch-net\src\Tests\Tests.Core\Extensions\SerializationTesterAssertionExtensions.cs(33,0): at Tests.Core.Extensions.SerializationTesterAssertionExtensions.AssertSerialize[T](SerializationTester tester, T object, Object expectedJson, String message, Boolean preserveNullInExpected)
        C:\projects\elasticsearch-net\src\Tests\Tests.Core\Serialization\ExpectJsonTestBase.cs(24,0): at Tests.Core.Serialization.ExpectJsonTestBase.RoundTripsOrSerializes[T](T object)
        C:\projects\elasticsearch-net\src\Tests\Tests\Framework\EndpointTests\ApiTestBase.cs(37,0): at Tests.Framework.ApiTestBase`5.SerializesInitializer()
    Tests.Document.Multiple.MultiTermVectors.MultiTermVectorsIdsApiTests.SerializesFluent [FAIL]
      Expected True because RoundTrip: 0 itteration
      Deserialization has result: False
      RoundTripResult`1 success: False
      This is the first time I am serializing
        {
          "ids": [
      -     {},
      -     {}
      +     1001,
      +     1002
          ]
        }
      
       C# approximation of actual ------ 
       new {
        ids = new [] {
          1001,
          1002
        }
      };
       C# approximation of expected ------ 
       new {
        ids = new [] {
          {},
          {}
        }
      };, but found False.

@Mpdreamz Mpdreamz mentioned this pull request Sep 3, 2018
45 tasks
Mpdreamz pushed a commit that referenced this pull request Sep 3, 2018
This commit adds support for providing a set of Ids to MultiTermVectors API to be used in
conjunction with index and type provided in the URI.

Index() and Type() methods added to MultiTermVectorOperation to allow the default typeof(T)
values to be overidden.

Closes #3219
Mpdreamz pushed a commit that referenced this pull request Sep 3, 2018
This commit adds support for providing a set of Ids to MultiTermVectors API to be used in
conjunction with index and type provided in the URI.

Index() and Type() methods added to MultiTermVectorOperation to allow the default typeof(T)
values to be overidden.

Closes #3219
Mpdreamz pushed a commit that referenced this pull request Sep 6, 2018
This commit adds support for providing a set of Ids to MultiTermVectors API to be used in
conjunction with index and type provided in the URI.

Index() and Type() methods added to MultiTermVectorOperation to allow the default typeof(T)
values to be overidden.

Closes #3219

(cherry picked from commit 491a4a4)
@russcam russcam deleted the feature/3219 branch October 18, 2018 09:45
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

Successfully merging this pull request may close these issues.

3 participants