Skip to content
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

Filter for blanks / non blanks #1569

Closed
3 tasks done
zewa666 opened this issue Jun 11, 2024 · 38 comments · Fixed by #1575
Closed
3 tasks done

Filter for blanks / non blanks #1569

zewa666 opened this issue Jun 11, 2024 · 38 comments · Fixed by #1575

Comments

@zewa666
Copy link
Contributor

zewa666 commented Jun 11, 2024

Clear and concise description of the problem

Similar to Excel it would be helpful if we could filter e.g. text columns to only show blank / non blank cells

https://docs.aspose.com/cells/net/how-to-filter-blanks-and-non-blanks/2.png

Suggested solution

I'm not sure where to place that feature. One idea I had was about an advanced filter, signaled by a gear icon placed next to the filter, which would popup a menu. in there buttons for blank/non blank could be placed. so it would be somewhat similar to the command popup.

Alternative

so far I can simulate non blanks by setting does not equal and "" as filter value. but that feels a bit weird

Additional context

No response

Validations

  • Follow our Code of Conduct
  • Read the Wikis.
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
@ghiscoding
Copy link
Owner

ghiscoding commented Jun 11, 2024

if it's a Compound Filter then it's nearly the same as this recent SO Question which brought a bug that I fixed yesterday in PR #1566 with the option skipCompoundOperatorFilterWithNullInput (see associated docs)

If you just want to filter blank/non-blank in the cell with a regular filter then just use <>, taking Example 11, I see that it works for returning blank (by using <> , however there seems to be a bug with != which is supposed to be equivalent to <> but currently behaves differently).

For the non-blank value then I don't think we have any ways currently to return them unless you use a Compound Filter without enabling skipCompoundOperatorFilterWithNullInput Actually there's a way which I just remembered, you can simply use > a (or > 0) and that will work because the values are compared against the ASCII table (case sensitive).

However, it's a bit weird that typing <> returns non-blank, I was expecting the inverse and that's probably a bug with the regex. As for the non-blank then I think > a is a totally fine way of getting these values

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 11, 2024

thanks for the hints and also skipCompoundOperatorFilterWithNullInput, good to know about that. yet the problem is that UX wise this feels a bit weird for a non-tech user.

another perhaps simpler approach would be to add a custom operator that ignores the value and mereley builds the blank/non-blank condition by itself.

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 11, 2024

well I mean we could have extra Compound Operators that could return blanks/non-blanks without typing anything (just selecting the operator) but that might feel a little weird because all the other operators expect the user to type a value.

For a regular text input then I don't really know, what did you have in mind exactly? Personally I feel ok with just using <> and > a because once the user knows it, it's easy to remember (though less techy users might not understand the ASCII table stuff) and actually by looking at the table again it's safer to use > A because A index is lower than a

On the other hand, the ASCII code above will probably just work with local JSON dataset, and probably not (or behave differently) with any Backend Services.

I think it's one of those thing that is good to have a cheat sheet not too far (perhaps some kind of Legend accessible maybe via the Grid Menu), I mean for the shortcuts that you can do directly with built-in filters. I presented some of them to our users couple years ago and they were a little impressed by these tricks.

By the way, isn't more of an external search feature? I mean, there's probably a limit to what we can add to our built-in filters, so at some point it might better to ask the user (you) to have its own external search like the one I've done in Angular-Slickgrid Example 23. Unless you have a clever way to add it into the built-in filters without cluttering them too much

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 11, 2024

I found the cheat sheet I had created years ago for the built-in filters, so here it is. By looking at it, I now realized what's the difference between <> and !=, they are not equivalent after all, I had completely forgot the differences 😆 and of course it's missing our new a*z operator 😉

image

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 11, 2024

yeah as said I feel a bit lost myself in where to add those. I'll bounce back to our users and see what they think of your proposed approach. the cheat sheet is huge and adding it to the command window as a help page is a fantastic idea 🎉

I'll let ya know what came out and worst case I can give a couple of ideas a shot and share some progress here if you're fine keeping this open. on the other hand if you feel that is way out of standard grid features please feel free to close the issue

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 11, 2024

We can keep it open until you circle back to your users and if anything comes out of it then we'll create a PR, otherwise close the issue later. For the cheat sheet, I guess you could do a print screen (or a new web page) or something that could be linked to a Grid Menu command, I'm not planning on adding it myself because there's English text in there and that might not be ideal for everyone and I know you also have to deal with multiple locales as well so... maybe the command could also detect which print screen to show depending on the locale 😄

But anyway I have to look into why typing <> (with a whitespace after the operator) is returning non-blanks when it should be the inverse, so that seems like a bug that I need to investigate (probably the regex)

BTW, where you using it with a Backend Service (OData) or not? I mean I know you're using OData but sometime developers use both types depending on how large their data is, so I'm just curious how you were querying this blank/non-blank values because like I said the local data filtering seems to have a bug but maybe not in OData?

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 11, 2024

oh yeah I just meant thanks for the tip, we'll create our own help page ofc.

exclusively OData. So in that case I still have a chance to manipulate the filter also on the backend to turn it into something that matches SQL.

@ghiscoding
Copy link
Owner

exclusively OData. So in that case I still have a chance to manipulate the filter also on the backend to turn it into something that matches SQL.

ok but I still wish to align the operator, so whatever you decide to use, I'd like to have a common approach for both local and backend since it's always more desirable to always filter the same way for both (less confusing for the user who don't care if you use backend or local dataset)

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 12, 2024

I now realized what's the difference between <> and !=, they are not equivalent after all, I had completely forgot the differences

Considering this difference, I think we might need some adjustments in the Backend Service(s) though it looks like OData doesn't have any "not contains" query filter (well I found this alternative). Do we need adjustment or should we keep them as is?

OData

case '<>':
case '!=':
map = 'ne';
break;

GraphQL

case '<>':
case '!=':
case 'NE':
map = OperatorType.notEqual;
break;

ghiscoding added a commit that referenced this issue Jun 12, 2024
- note that `!=` is "not equal" and is not equivalent to `<>` which is "not contains". So only the `!= ` will return non-blanks while `<> ` will often return no data when all data have white spaces.
- partially fixes a problem highlighted in issue #1569 on how to return non-blanks rows for a local JSON dataset
@zewa666
Copy link
Contributor Author

zewa666 commented Jun 12, 2024

thats a good catch.

for odata checking for blanks needs also to respect nulls. So xyz ne null AND xyz ne "" would be what I need I guess but thats perhaps already non-standard logic

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 12, 2024

@zewa666 I'll let you deal with that one then, if you think it needs any fixes or not. It looks like it's possible to make a distinction in GraphQL for "not contains" vs "not equal" but it might not be easy to do with OData (or not without extra or duplicate query like you suggested) so that might not be possible for all backend services. Do what you think is best since you are a user and I'm not 😉

Circling back to @jr01 again since he might also bring his point of view on this filter search for blank/non-blanks with OData and "not contains" vs "not equal". Thanks

@jr01
Copy link
Collaborator

jr01 commented Jun 12, 2024

Yes, to filter for non blanks you should use: $filter=Name ne '' and Name ne null.

In oData you can use $filter=not contains(Name, ''), but in my quick testing it returned no rows at all. I guess contains(Name, '') (without the not) is logically always true because every string (even the blank ones) will contain the empty string by definition.

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 12, 2024

@jr01 thanks for the confirmation. but do you also agree that this is a somewhat OData specific implementation hence might not suitable for slickgrid filter standards?

my intention right now, after @ghiscoding's insights, goes towards the direction to create a custom filter with said advanced menu and buttons for blank/non blank so we don't clutter the standard slickgrid experience.

@jr01
Copy link
Collaborator

jr01 commented Jun 12, 2024

Well, from a user perspective it is very nice to have "(Blank)" as an option I think. For example, in a dropdown like the Company filter:
image

I imagine it could have values "Select all, ABC, XYZ, (Blank)" and the latter would select those customers that don't have a company.

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 12, 2024

just a side note and not to get confused with similar terms here, the single select filter also accepts an empty (aka blank) value which is meant to "return everything". You could maybe add a "(blanks)" value instead to return blanks. So @zewa666 what was your vision here? I mean did you wanted to expand on an input text filter or did you want to use a select filter with extra values like "(blanks)" and "(non-blanks)"? If you want to use the select filter then I guess there's nothing to do here since you can already do this today by just adding these extra values.

image

Also note that on the GraphQL side we can use a not contains (we program anything really).

So I'll just move the <> from this case

case '<>':
case '!=':
case 'NE':
map = OperatorType.notEqual;
break;

to this instead case '<>':

case 'Not_Contains':
case 'NOT_CONTAINS':
map = OperatorType.notContains;
break;

so from this perspective, GraphQL is different than our OData implementation since OData doesn't support not contains in its syntax but that's about the only difference between these 2 backend services in the project and as @zewa666 mentioned I want to keep a certain standard in the project to not confuse the users

ghiscoding added a commit that referenced this issue Jun 12, 2024
…#1570)

- note that `!=` is "not equal" and is not equivalent to `<>` which is "not contains". So only the `!= ` will return non-blanks while `<> ` will often return no data when all data have white spaces.
- partially fixes a problem highlighted in issue #1569 on how to return non-blanks rows for a local JSON dataset
@zewa666
Copy link
Contributor Author

zewa666 commented Jun 12, 2024

I'll go ahead with a custom filter as example to show what I meant. post it here once I've got a POC

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 12, 2024

alright, here it is.

Witness my hacky wacky CSS skills of the popup 🤣
additional_filters_popup

The following code is really just for demonstration purpose on how to override the existing text filter rendering.

class MyFilter extends InputFilter {
  protected override createDomFilterElement(searchTerm?: SearchTerm) {
    super.createDomFilterElement(searchTerm);

    const searchButtonElm = document.createElement('button');
    searchButtonElm.innerHTML = '<span class="mdi mdi-cog mdi-18px"></span>';
    searchButtonElm.title = 'Additional filters';
    searchButtonElm.classList.add('search-button');
    searchButtonElm.style.marginLeft = '5px';
    searchButtonElm.addEventListener('click', () => {
      const position = searchButtonElm.getBoundingClientRect();
      const popupElm = document.createElement('div');
      popupElm.classList.add('popup-menu');
      popupElm.style.position = 'absolute';
      popupElm.style.top = `${position.top + 30}px`;
      popupElm.style.left = `${position.left + 5}px`;
      popupElm.style.width = '100px';
      popupElm.style.backgroundColor = 'white';
      popupElm.style.border = '1px solid #ccc';
      popupElm.style.borderRadius = '5px';
      popupElm.style.zIndex = '9999';
      popupElm.innerHTML = `
        <div class="popup-menu" style="width: 200px; background: lightyellow">
          <div class="popup-menu-item"><button class="button">Show blanks</button></div>
          <div class="popup-menu-item"><button class="button">Show non blanks</button></div>
        </div>
      `;
      const buttonBlanks = popupElm.querySelector('.popup-menu-item:nth-child(1) button');
      const buttonNonBlanks = popupElm.querySelector('.popup-menu-item:nth-child(2) button');
      buttonBlanks?.addEventListener('click', () => {
        ((window as any).sgb as SlickVanillaGridBundle).filterService.updateFilters([
          { columnId: `${this.columnDef.id || ''}`, operator: '', searchTerms: ['< a']}
        ], true, true, true);
        popupElm.parentElement?.removeChild(popupElm);
      });
      buttonNonBlanks?.addEventListener('click', () => {
        ((window as any).sgb as SlickVanillaGridBundle).filterService.updateFilters([
          { columnId: `${this.columnDef.id || ''}`, operator: '', searchTerms: ['> a']}
        ], true, true, true);
        popupElm.parentElement?.removeChild(popupElm);
      });
      document.body.appendChild(popupElm);
    });

    this._filterContainerElm.style.width = '80%';
    this._filterContainerElm!.parentElement!.style.display = 'flex';
    this._filterContainerElm!.parentElement!.appendChild(searchButtonElm);
  }
}

So my original point is/was whether we should do something like demonstrated here as default, use a custom MenuBaseClass and populate it with commands the user defines on the e.g filter.additionalCommands property.

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 12, 2024

hmm to me this seems more like a "Create your own Custom Filter" situation, I'm saying this mostly because as per your demo it's already doable by just typing the correct characters to make it work (I actually didn't think about using < a for blanks, nice idea). The only other thing I could think is to create another type of filter that would have these 2 options and maybe flip the dropdown position from the left to the right so that it's more obvious as being different than Compound Filter but... again I'm not sure if I really want to add this into the project though, this seems overkill to me and mostly just a matter of training your users (adding a Docs into the Grid Menu might suffice). If we decide to add it, then will that be a new Filter or will that work with other Filters? If so that might open a can of worms by opening a varieties of other non compatible problems (we can't filter more than 1 thing at a time, apart from the filter ranges and the new a*z combo)

Also a side note, I created the Compound Filters by using Bootstrap input group

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 12, 2024

well my idea with this was along the lines of filter templates for users. so instead of them having to remember things, we fill them in for them with a descriptive button. Hence why the list should be configurable from a collection of commands perhaps placed directly on the filter property of the column. and thats why it should potentially also work for all sorts of filters. the user can still go ahead and write/click their own filter.

But as you properly noted, this is already a very narrow use case plus one easily achievable in user land code, I'll just use the generic params on the filter property to store my commands. I'm equally not sure if it should be a standard feature but just wanted to share the idea and check back what you think.

that said, please feel free to close the issue.

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 12, 2024

if we look at how Ag-Grid does it, it has some good and bad part in their implementation. They have a small pyramid icon on the right side of each column filter. The bad side is that if you choose most type of >, <=, ... then it won't appear when you close the modal except for the little blue dot (so my Compound Filter approach is better than theirs I think) but on the other hand they show a readonly filter as "blank" or "nonBlanks" when we choose either of these 2, which seems like a good approach (at least now they show these 2 filter types)

msedge_1TNxCRn6NH

The Ag-Grid approach makes me think of 2 ways we can do it, 1st would be like them with an extra icon or the 2nd approach is that it just makes me think a lot about the Column Header Button (however for that we would need to get the instance ref of each filter in order to do anything with them OR call the dynamic filter set method), and there's actually a 3rd approach maybe via a context menu over the filter (this one would be much simpler to implement but a lot less visible to the user).

Personally, I think the Header Menu makes more sense and wouldn't add any extra UI because the goal is to not make it too clutter by adding more and more UI stuff. But again like I said, I would need to find each filter instance ref to play them but I don't think that would be much of a problem (there's already such ref list in the FilterService, I could maybe move it to the SharedService so that I could use it from the Header Menu or just reference FilterService from within HeaderMenu). I can give a try after work in the next couple of hours, I like this Header Menu approach more than the other 2 I've mentioned... oh and since I've added sub-menus to all menu plugins, I could maybe add a Filter menu and then sub-menus for all commands like blanks/non-blanks/...

also curious of what @jr01 thinks on all of that too

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 13, 2024

There you go, 2 shortcuts in my custom Header Menu via a sub-menu. It works "as is" and you can simply copy the code below and try it for yourself 🚀

Couple of things to note though, I say it works "as is" but the code could be improved a little. With the code below, I'm calling setValues() but that doesn't trigger the keyup event (that method was meant to only set the value in the DOM for unit testing purposes but without triggering a filter change), so I need to manually call the callback() method myself. However, technically speaking each filter already have their own protected onTriggerEvent() (or something similar, since it may vary depending on each type of filter), but for a much simpler code, I could add an extra 3rd argument to the setValues(searchTerms, operator, shouldTriggerCallback) so that it would trigger by itself and I wouldn't have to know the exact callback arguments required for each filter. Also note that the position of the command in the header menu can be adjusted by a positionOrder that I added a while ago to sort them however I want them in the command list.

With all of that in mind, I think what we could potentially do is to maybe provide an extra property in the Column interface that could loop through a list of Filter Shortcuts and add it to the Header Menu by itself instead of requiring the user to add them like the code below. I think this approach is ok and does not clutter the UI while also being visible to the end user.

Note I used < 0 just because of its char index in the ASCII table (or use < A if you're sure that it starts with a character, I'm not sure why they decided to put A before a though) and technically speaking if we also want to use special chars as text then maybe < # is better... long story short, see ASCII table for proper filtering

this.columnDefinitions = [
{
  id: 'countryOfOrigin', name: 'Country of Origin', field: 'countryOfOrigin',
  header: {
    menu: {
      commandItems: [
        {
          command: 'filter-shortcuts',
          iconCssClass: 'mdi mdi-filter-outline',
          title: 'Filter Shortcuts',
          commandItems: [
            {
              command: 'filter-blanks',
              title: 'Filter Blanks',
              iconCssClass: 'mdi mdi-filter-minus-outline',
              action: () => {
                const filterRef = this.sgb.filterService.getFiltersMetadata().find(f => f.columnDef.id === 'countryOfOrigin');
                if (filterRef) {
                  const searchTerms = ['< 0'];
                  filterRef.setValues(searchTerms);
                  filterRef.callback(undefined, { columnDef: filterRef.columnDef, operator: filterRef.operator, searchTerms, shouldTriggerQuery: true });
                }
              }
            },
            {
              command: 'filter-non-blanks',
              title: 'Filter Non-Blanks',
              iconCssClass: 'mdi mdi-filter-plus-outline',
              action: () => {
                const filterRef = this.sgb.filterService.getFiltersMetadata().find(f => f.columnDef.id === 'countryOfOrigin');
                if (filterRef) {
                  const searchTerms = ['> 0'];
                  filterRef.setValues(searchTerms);
                  filterRef.callback(undefined, { columnDef: filterRef.columnDef, operator: filterRef.operator, searchTerms, shouldTriggerQuery: true });
                }
              }
            },
          ]
        },
      ]
    }
  },
}];

brave_OBbiwRrDEL

ghiscoding added a commit that referenced this issue Jun 13, 2024
- while investigating for issue #1569, I found that the GraphQL `<>` was set to be the equivalent of `!=` but this is in fact false, the `<>` is meant to represent `Not_Contains` while `!=` is meant to represent `Not_Equals` and they both work differently since `<>` is for a substring but the `!=` is for the entire string
ghiscoding added a commit that referenced this issue Jun 13, 2024
…1571)

- while investigating for issue #1569, I found that the GraphQL `<>` was set to be the equivalent of `!=` but this is in fact false, the `<>` is meant to represent `Not_Contains` while `!=` is meant to represent `Not_Equals` and they both work differently since `<>` is for a substring but the `!=` is for the entire string
@ghiscoding
Copy link
Owner

ghiscoding commented Jun 13, 2024

BTW and totally out of context, did you guys know that even Microsoft uses SlickGrid in one of their open source app called Azure Data Studio which is a cross-platform tool that allows to easily connect to MsSQL, MySQL, PostgreSQL, MongoDB and CosmoDB. The usage of SlickGrid is to display the data returned by the query... However they use an old fork of SlickGrid with jQuery, but still I'm always happy to see SlickGrid used everywhere and even by the big tech companies 🚀

select-results 1

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 13, 2024

holy moly and I thought so often this looks familiar 😄

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 13, 2024

hmm now there is a new tricky one ... a number filter ... any ideas how to search for blanks/non blanks here? :)

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 13, 2024

I'm not sure why but < Infinity actually removes the blanks, I was expecting the inverse but anyway it works and you can the negative for the inverse so > -Infinity, the only problem is you can't type that in a number input so you will have to use the regular text input for that to work

alt text

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 13, 2024

So you like the Header Menu approach? Did you think it has to go in the internal code then? I think it could go in the Column interface and/or the GridOption, however the latter would probably need some kind of properties to differentiate the filter types maybe something like I've done with the recent Editor/Filter global configs

this.gridOptions = {
  defaultFilterOptions: { 
    autocompleter: { debounceWaitMs: 150 }, // auto-typed as AutocompleterOption
    date: { minDate: 'today' },
    longText: { cols: 50, rows: 5 } 
  }
}

so something along these lines maybe, the advantage of grid options is that it could be set globally

this.gridOptions = {
  defaultFilterShortcuts: { 
    inputText: [
      { command: 'blanks', iconCssClass: 'mdi mdi-filter-minus-outline', name: 'Filter Blanks', searchTerms: ['< 0'] },
      { command: 'non-blanks', iconCssClass: 'mdi mdi-filter-plus-outline', name: 'Filter Non-Blanks', searchTerms: ['< 0'] }
    ],
    inputNumber: [
      { command: 'blanks', iconCssClass: 'mdi mdi-filter-minus-outline', name: 'Filter Blanks', searchTerms: ['< Infinity'] },
      { command: 'non-blanks', iconCssClass: 'mdi mdi-filter-plus-outline', name: 'Filter Non-Blanks', searchTerms: ['> -Infinity'] }
    ],
  }
}

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 13, 2024

I didnt have the chance to talk to our users but think that could be a good + standard way to do it. I personally wouldnt do that globally. In general I'm not a fan of these kind of fallbacks as I prefer explicit column configuration as it becomes a challenge to trace all potential hops when you're searching for why something happens as it does.

so if not explicitly required I'd go with columns first and add the global on popular demand

@ghiscoding
Copy link
Owner

ghiscoding commented Jun 13, 2024

Yeah I mean that would be like most other flags like for example exportWithFormatter, if you define it in your grid options and have not defined in your column then the grid option will be used.. however if you have it defined in your column and the grid option then the column has a higher index so the column flag will be used. That's the typical approach that I do for most of these options, it's easy to implement and it brings you bigger flexibility with multiple level of settings

@zewa666
Copy link
Contributor Author

zewa666 commented Jun 13, 2024

i get that, just not a fan of it ;)

but our users are big fans of the header menu idea. So if you need a review for your PR please count me in

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 2, 2024

I now realized what's the difference between <> and !=, they are not equivalent after all, I had completely forgot the differences

Considering this difference, I think we might need some adjustments in the Backend Service(s) though it looks like OData doesn't have any "not contains" query filter (well I found this alternative). Do we need adjustment or should we keep them as is?

OData

case '<>':
case '!=':
map = 'ne';
break;

GraphQL

case '<>':
case '!=':
case 'NE':
map = OperatorType.notEqual;
break;

well actually I've tried it now out with something like (not contains(OrderNo, 'AX1507170')) and my sql helper manages to convert that to WHERE NOT ([OrderNo] like 'AX1507170')

@jr01 could you tell me what interprets your OData query and is not able to produce a NOT Contains?

@ghiscoding since I've already subclassed GridOdataService for other purposes I can easily override updateFilters and map <> manually to using:

override updateFilters(
    columnFilters: ColumnFilters | CurrentFilter[],
    isUpdatedByPresetOrDynamically?: boolean
  ): void {
    Object.values(columnFilters).forEach((filter) => {
      if (filter.operator === "<>") {
        filter.operator = OperatorType.notContains;
      }
    });
    super.updateFilters(columnFilters, isUpdatedByPresetOrDynamically);
  }

@ghiscoding
Copy link
Owner

you can imagine my answer... I would welcome any fix as long as it's not a breaking change (if it's not at all what you would expect then I consider that a bug and not a breaking change). Since the <> should have been considered different than != all along, I consider this a bug

@jr01
Copy link
Collaborator

jr01 commented Jul 2, 2024

@zewa666 - I don't think I mentioned that my backend can't handle not contains. It can and I tested that, but it didn't produce what you want for the 'non blanks' case.

To expand on my short message #1569 (comment)

here is a snip from Entity Framework Core logs with sensitive values on. This is for $filter=not contains(Name, '')

Parameters=[@__TypedProperty_1_contains='%'

AND [c].[Name]  NOT LIKE @__TypedProperty_1_contains ESCAPE N'\'

so you can see how it translates.

The question is if that can be used for non blanks. To answer that lets do an experiment in SQL server.

-- drop table Person
create table Person (Name nvarchar(10) NULL)
insert into Person (name) VALUES ('A')
insert into Person (name) VALUES ('B')
insert into Person (name) VALUES ('')
insert into Person (name) VALUES (NULL)

-- $filter=contains(Name, 'A')
select * from Person WHERE ([Name] like '%A%')

-- $filter=contains(Name, '') then "could" (depending on implementation) translate to
select * from Person WHERE ([Name] like '%')

-- and if that is the case a $filter=not contains(Name, '') logically translates to
select * from Person WHERE NOT ([Name] like '%')

-- and that gives different results from `$filter=Name ne '' and Name ne null`
select * from Person WHERE ([Name] != '' ) AND (NOT [Name] IS NULL)

How oData contains to SQL LIKE should be translated is perhaps debatable and I can imagine there differnet libraries make different choices. To avoid any of that just use the $filter=Name ne '' and Name ne null for non blanks.

@ghiscoding
Copy link
Owner

ghiscoding commented Jul 2, 2024

How oData contains to SQL LIKE should be translated is perhaps debatable and I can imagine there differnet libraries make different choices. To avoid any of that just use the $filter=Name ne '' and Name ne null for non blanks.

I think that a special case could be acceptable for the blank use case, you might just need to tweak this if condition. You guys are the 2 biggest users of OData (I don't use it anymore on my side), so I'm ok with an approach that seems acceptable by both of you 😉

} else if ((operator === '' || operator === OperatorType.contains || operator === OperatorType.notContains) &&

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 2, 2024

@jr01 oh ok I get the confusion I've created now. well the not contains scenario was mainly mentioned to fix the current behavior of <> resulting in ne which is wrong for text cases. the blank/non blanks we handled with the new filter templates feature.

@ghiscoding
Copy link
Owner

@zewa666 are you going to create a PR for this or were you waiting for more comments from @jr01? I pushed a couple of fixes and wanted to know if I should go ahead with a new patch release or wait for a possible PR from your side? There's no rush though, just asking.

I think I'm done with pretty much everything and I don't see any other fixes or features to do at this point

@zewa666
Copy link
Contributor Author

zewa666 commented Jul 4, 2024

nope all good. I'm not sure its a huge benefit and more of an edge case, so I'm fine having this taken care in our own codebase especially since we're gonna modify it further with treatments for empty/null coverage. thx for all the work of yours

@ghiscoding
Copy link
Owner

okie dokie, will push a patch this weekend

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 a pull request may close this issue.

3 participants