`OnBeforeQuery` existing Multi-tenant and Soft-delete examples may have a flaw ?

[Nik]

Hi,

There have been examples in this group or the presentations where the `OnBeforeQuery` event has been used to further constraint the query, like in Multi-Tenant or Soft-Delete scenarios.

The provided approach can lead to very undesired behavior if there is any `OR` comparison used in the query:

Existing `OnBeforeQuery` examples:

Multi-tenant:

dynamic query = e.QueryCustomization;
query.AndAlso();
query.WhereEquals("TenantId", tenant.Id);

Soft delete:

switch (e.QueryCustomization)

    {

        case IDocumentQuery<User> userQuery:

            userQuery.AndAlso().WhereEquals("IsDeleted", false);

            break;

        case IAsyncDocumentQuery<User> userAsyncQuery:

            userAsyncQuery.AndAlso().WhereEquals("IsDeleted", false);

            break;

        default:

            break;

    } 

Problematic example:

Let's say we use the soft-delete approach described above with the following query where we have an `or` condition:

session.Query<Article>().search(article => article.Title, "foo").search(article => article.Description, "bar", options: SearchOptions.Or);

The final query produced will be:

select from 'Articles' where search(Title, "foo") or search(Description, "Bar") and IsDeleted = false;

This will return articles where the title matches "foo" disregarding the "IsDeleted" constraint.

Question:

Is there a way the `OnBeforeQuery` callback can be modified to enclose the existing query conditions into brackets, before adding the `.AndAlso().WhereEquals()` condition? That would eventually solve the problem.

[Arão Benjamin Garcea]

Hi Nick,

After some research, I found a solution that prevents the necessity for a modification in the query construction algorithm.

I didn't test it with a .search().search(), but I testes it with:

session

    .DocumentQuery<class>()

    .WhereEquals(a is true)

    .OrElse()

    .WhereEquals(a is false);

The solution, inside the OnBeforeQuery event handler was:

You can see my code here:

In fact, as you can see from my output, it really encloses all the conditions and add the final condition(s):

I don't know the performance implication of using .Intersect(),  but it worked just fine.

Perhaps one of RavenDB guys could say something about that.

Great journey!

[Oren Eini (Ayende Rahien)]

That is indeed an issue. 

https://issues.hibernatingrhinos.com/issue/RavenDB-16447

--
You received this message because you are subscribed to the Google Groups "RavenDB - an awesome database" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ravendb+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/ravendb/ce66556e-5d02-45ec-9ec9-2a88683fc434n%40googlegroups.com.

[Nik]

Thanks Arao, Oren

Intersect() indeed solves the problem but partially. 

The problem with Intersect() is that it throws an Exception in case there are no defined filters prior to calling it, and I don't see there is a way to check for existence of already defined filters.

Example:

session.Query<Article>.ToList();   // Intersect() used in OnBeforeQuery callback would throw an exception.

@Oren

It would be helpful if Intersect() would also work if there is no query condition defined before it. In that case, the query builder could just ignore the intersection?

That would also help in scenarios where we are building the query based on input parameters. Example:

var query = session.Query<Article>;

// if published is not null filter by published status

if (published is not null) {

    query = query.Where(article => article.published == published);

}

// filter by search query

if (!string.IsNullOrWhiteSpace(searchQuery)) {

    query = query

            .Intersect()   // we need intersect as we have an "OR" condition below

            .Search(article => article.Title, searchQuery)

            .Search(article => article.Description, searchQuery, options: SearchOptions.Or)

}

return await query.ToListAsync();

The above example would fail in case we are not filtering by "published" status and the "searchQuery" is set, as Intersect() requires that there are filters applied before it.

@Oren

Is there some performance penalty when using Intersect()?

[Arão Benjamin Garcea]

Hi @Nik,

I must confess, you're advancing a lot of work I would have in the future, so this discussion is being great for me.

You're completely right about intersecting an empty condition. It's not even intersecting an empty set, that should just don't return anything, but intersecting a void condition should really fail.

But, you can test the query string for the "where" word, and only append the .Intersect(); when it exists, like:

I tested with a conditionless query and it worked.

Again, we're dealing with a performance issue here, as "Intersect" rhymes too much with "Join".

@Oren, if you can enlighten us here.

[Nik]

Thanks Arao,

I see now that Intersect() has a side-effect of introducing skipped results (as described here) although there was no fanout index used.

The implication of that is that the `statistics.TotalResults` property no longer contains the true count of matching documents but it includes skipped documents as well.

I'm still trying to figure out the proper pagination technique when `skippedResults` is > 0, as according to the example from the documentation when targeting page N where N is greater than 1, we must know the previous skipped count.

According to that, if for example requesting page 10 directly, we would need to query the results for pages 1-9 in order to find out the skipped count?

Note: In practice, I was able to get the expected result without adding the `skippedResults` to `skip()` (as in documentation). Only the `TotalCount` was greater than the true document count by the `skippedResults` amount.

@Oren: Please can you comment on this one ? 

Example from the docs:

do

{

    results = session

        .Query<Product, Products_ByUnitsInStock>()

        .Statistics(out QueryStatistics stats)

        .Skip((pageNumber * pageSize) + skippedResults)

        .Take(pageSize)

        .Where(x => x.UnitsInStock > 10)

        .Distinct()

        .ToList();

    skippedResults += stats.SkippedResults;

    pageNumber++;

}

while (results.Count > 0);

 

Maybe the documentation is out of date, or I am misunderstanding it.

[Nik]

Hey Arao, 

Seems there's some problem with onBeforeQuery and streaming results, in case you are using it:

https://github.com/ravendb/ravendb/issues/12066