Skip to content

Verbatim and strict query fixes #2021

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
Apr 22, 2016
Merged

Verbatim and strict query fixes #2021

merged 1 commit into from
Apr 22, 2016

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Apr 7, 2016

  • Add ability to set IsStrict and IsVerbatim on OIS query objects and at
    the query level for the fluent syntax. Setting at the container level
    for both APIs is a noop.
  • Fix IsStrict check for initializer syntax. Previously, an exception was
    never thrown.

Closes #1898
Closes #1876

bool IQuery.IsWritable { get; }
public bool IsVerbatim { get; set; }
public bool IsStrict { get; set; }
public bool IsWritable { get { return this.IsVerbatim || IsConditionless(this); } }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be

public bool IsWritable { get { return this.IsVerbatim || !IsConditionless(this); } }

i.e. negate IsConditionless?

@russcam
Copy link
Contributor

russcam commented Apr 8, 2016

IsStrict and IsVerbatim are now a noop on the outer container so the expectation in #1898 will not work.

I've added some tests for this.

Also fixed SpanQuery IsWritable

@russcam
Copy link
Contributor

russcam commented Apr 8, 2016

I would have expected this to pass i.e. IsVerbatim to cascade from the bool query to the contained clauses, but it would fail currently

public class BoolVerbatimQueryUsageTests : QueryDslUsageTestsBase
{
    protected override bool SupportsDeserialization => false;

    public BoolVerbatimQueryUsageTests(ReadOnlyCluster cluster, EndpointUsage usage) : base(cluster, usage) { }

    protected override object QueryJson => new
    {
        @bool = new
        {
            must = new object[]
            {
                new
                {
                    term = new
                    {
                        description = new
                        {
                            value = ""
                        }
                    }
                },
                new
                {
                    term = new
                    {
                        name = new
                        {
                            value = "foo"
                        }
                    }
                }
            }
        }
    };

    protected override QueryContainer QueryInitializer =>
        new BoolQuery
        {
            // cascade to clauses?
            IsVerbatim = true,
            Must = new List<QueryContainer>
                {
                    new TermQuery
                    {
                        Field = "description",
                        Value = ""
                    },
                    new TermQuery
                    {
                        Field = "name",
                        Value = "foo"
                    }
                }
        };


    protected override QueryContainer QueryFluent(QueryContainerDescriptor<Project> q) => q
        .Bool(b => b
            // cascade to clauses?
            .Verbatim()
            .Must(qt => qt
                .Term(t => t
                    .Field(p => p.Description)
                    .Value("")
                ), qt => qt
                .Term(t => t
                    .Field(p => p.Name)
                    .Value("foo")
                )
            )
        );
}

Is my thinking incorrect here?

@@ -42,7 +42,7 @@ public class SpanQuery : ISpanQuery
bool IQuery.IsWritable { get; }
public bool IsVerbatim { get; set; }
public bool IsStrict { get; set; }
public bool IsWritable { get { return this.IsVerbatim || IsConditionless(this); } }
public bool IsWritable => this.IsVerbatim || !IsConditionless(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch 👍

@gmarz
Copy link
Contributor Author

gmarz commented Apr 8, 2016

IsStrict and IsVerbatim are now a noop on the outer container so the expectation in #1898 will not work.

That's correct. IsVerbatim/Strict has to now be set at each desired query to obtain this behavior, which wasn't possible before.

I would have expected this to pass i.e. IsVerbatim to cascade from the bool query to the contained clauses, but it would fail currently

That would be ideal, but not sure if it's possible for the same reason we can't cascade from QueryContainer. I could be wrong though...

@russcam
Copy link
Contributor

russcam commented Apr 8, 2016

That would be ideal, but not sure if it's possible for the same reason we can't cascade from QueryContainer. I could be wrong though...

OK, apologies if we've discussed this before 😄 That does feel a little bit unintuitive for compound queries like bool where Verbatim() and Strict() are exposed still as they're inherited from QueryBase and QueryDescriptorBase<TDescriptor, TInterface>.

@gmarz gmarz mentioned this pull request Apr 19, 2016
@Mpdreamz Mpdreamz force-pushed the fix/verbatim-queries branch from 5625a46 to 0bee7a5 Compare April 22, 2016 15:00
Mpdreamz added a commit that referenced this pull request Apr 22, 2016
- moved tests into their own namespace so they can be easily ran as a
  single unit
- Rebased against 2.x and removed newly introduces InvokeQuery()'s
- Renamed fluent Assign of the QueryCotainerDescriptor to
  WrapInContainer, since thats what it is doing and to mimic the OIS
  equivalent.
@Mpdreamz
Copy link
Member

Rebased against 2.x appended some small touch ups, please review @gmarz

LGTM 👍

- Add ability to set IsStrict and IsVerbatim on OIS query objects and at
  the query level for the fluent syntax.  Setting at the container level
  for both APIs is a noop.

- Fix IsStrict check for initializer syntax.  Previously, an exception was
  never thrown.

Closes #1898
Closes #1876
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