DataTables / Editor-NET

.NET Framework and .NET Core server-side libraries for Editor
Other
15 stars 12 forks source link

SearchPanes and SearchBuilder do not consider pre-filtering of the table. #11

Open gotmoo opened 8 months ago

gotmoo commented 8 months ago

When you apply a top level filter on a table, this is not taken into account by the options presented by SearchBuilderOptions or SearchPaneOptions.

Setup

Using the current example download for Editor. Showing this with SearchPanes because it is easier to see in the browser:

  1. Modify Controllers\SearchPanesController.cs to add a top level filter on site using the Where clause
    var response = new Editor(db, "users")
    .Model<UploadManyModel>()
    .Where("site", "1", "=")
  2. Run the project and visit /examples/extensions/searchPanes.html

Result

The resulting table shows the correct data set: image

However, the SearchPanes options still show the options from the full data set: image

This includes the other options like Name: image

Selecting one of the pre-filtered names results in zero records found, while the options show there should be one.

Expected behavior

Pre-filtered items are hidden from the SearchPane/SearchBuilder options.

This could result in data leaking, or weird results from logically deleted records.

gotmoo commented 8 months ago

I found a fairly simple solution, that appears to work pretty good:

1st, in editor.cs, expose the _GetWhere method internally:

/// <summary>
/// Apply the global Where filter to the supplied Query
/// </summary>
internal void GetGlobalWhere(Query query)
{
    _GetWhere(query);
}

Then in SearchPaneOptions, apply the filtering twice by calling the above function with the query:

var q = db.Query("select")
    .Distinct(true)
    .Table(table)
    .Get(label + " as label")
    .Get(value + " as value")
    .GroupBy(value)
    .Where(_where)
    .LeftJoin(join);
editor.GetGlobalWhere(q);

and again later:

var entriesQuery = db.Query("select")
    .Distinct(true)
    .Table(table)
    .LeftJoin(join);
editor.GetGlobalWhere(entriesQuery);

Similarly, in SearchBuilder:

var query = db.Query("select")
    .Table(this._table)
    .LeftJoin(_leftJoin);
editor.GetGlobalWhere(query);

If this looks reasonable, I've pushed this onto my fork and created a pull request for this.

See changes here: Editor.cs SearchPaneOptions.cs and SearchBuilderOptions.cs

gotmoo commented 7 months ago

Gentle nudge for @AllanJard to take a look. I hope that may be included in the January update for the NuGet package.

AllanJard commented 7 months ago

Apologies - I've got it in my inbox, but haven't had a chance to have a detailed look yet. I thought I'd added a comment here already, but apparently no - apologies! I can see the benefit of this approach - I'm not sure it is quite the right way to do it, as I think the API is a little confusing. I need to have a bit of a think about it, and it would need to be ported to our other server-side libraries as well. I fear it is unlikely to get in for the Editor 2.3 release, sorry.

gotmoo commented 7 months ago

No worries, this was a first draft approach that seems to work for me, but there's other ways to solve this. On top of that, my approach does not account for linked tables so that results from a linked table are not filtered against that subset. For example filtering on the ID of a site on the main table would not filter only to that site ID from the sites table.

If you have any suggestions, I could try to work out an alternative way to accomplish the same, perhaps by doing this directly in the Query classes, and basing it on established joins and table names.

I'll also have a think on the API naming for this.

AllanJard commented 7 months ago

I'm wondering about a method on the Options class .UseHostWhere() or something (that's a horrible name, I'm sure I can think of better than that...!). That would then give the flexibility using or not the main Where statement. Or it might be that we should just carry on requiring that it be explicitly set (it could be put into a function that is shared between all Options instances for example).

gotmoo commented 6 months ago

I will try to find some time to add this to the options class. How about .UseParentWhere() or .ApplyParentWhere()?

What are your thoughts about setting this option in the inverse? Adding an option to explicitly ignore the top level where statement and include all the data? I personally always forget about the options class, and with this the default would be that the search options do not include the pre-filtered data. Then if you do need it included, you can enable that with the options.

AllanJard commented 6 months ago

Given that the library is already deployed and used in production, for backwards compatibility the new feature should be opt in, rather than opt out. So .ParentWhere(true) or something like that would probably be the correct thing to do.

Sorry I've not had a chance to look into this yet. Too many things to do :-)

gotmoo commented 6 months ago

Agreed on not breaking existing deployments.

No worries on the timing, I know things are crazy with V2 and all that entails! I hope to find some time myself soon to have poke through the code for implementing this as well.