graphql-platform icon indicating copy to clipboard operation
graphql-platform copied to clipboard

Case insensitive string contains filter

Open tsinghammer opened this issue 3 years ago • 9 comments

Summary of the changes (Less than 80 chars)

  • Detail 1
  • Detail 2

Closes #bugnumber (in this specific format)

tsinghammer avatar Apr 18 '22 20:04 tsinghammer

@tsinghammer so the reason i has this on hold for a little was that i didnt like that the IQueryable specific extension just sits on the IFilterConventionDescriptor that is also used by other providers like mongo.

I think we should introduce a proxy of the IFilterConventionDescriptor and have more specific extension methods. I would also introduce this in mongo.

We would then also have a more specific extension method for IQueryable.

.AddQueryableFiltering(x =>...)

I started the work in https://github.com/ChilliCream/hotchocolate/pull/5181 but wanted to see what @michaelstaib thinks first.

PascalSenn avatar Jun 25 '22 21:06 PascalSenn

@tsinghammer So it seems like we have a bit of a problem here :) EF Core hast trouble converting string contains with invariant compare into a SQL query. There are a couple of ways how we could solve this, but it seems like we need DB specific drivers

PascalSenn avatar Aug 01 '22 08:08 PascalSenn

For MSSQL server I think the default setting is a case insensitive comparison, but I guess we won't rely on that. I would assume that most people set there column to something like this COLLATE Latin1_General_CI_AS and we could just use Contains. Is there already driver specific code in HC so far or everything agnostic?

tsinghammer avatar Aug 01 '22 09:08 tsinghammer

@tsinghammer so that would mean, the code we have now would only work on in memory data. We do not have db specific drivers yet. I have a couple of things that i want to add to postgres directly, the question is, how different will the implementations be for MariaDB, SQLLite and SQL. I mean we have EF Core that abstracts the database for us, but invariant contains is one of the things that seem to be very database specific: https://stackoverflow.com/questions/43277868/entity-framework-core-contains-is-case-sensitive-or-case-insensitive

I honestly hope we do not end up with AddPostgresFiltering , AddMariaDbFiltering and so on. This would not only be a pain to implement, but also a pain for the user. As with EF Core the user can basically just use the same filters for the DB as they use for the In Memory collections.

PascalSenn avatar Aug 01 '22 09:08 PascalSenn

And if we really just map IContains to Contains for EF and leave it up to the database configuration? We could see if anyone complains :-)

tsinghammer avatar Aug 01 '22 10:08 tsinghammer

Is contains working for all of the database drivers used so far?

tsinghammer avatar Aug 01 '22 10:08 tsinghammer

And what about converting it to a Linq expression first that compares both the field and the parameter to lower first?

tsinghammer avatar Aug 01 '22 10:08 tsinghammer

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

:white_check_mark: michaelstaib
:white_check_mark: PascalSenn
:white_check_mark: tsinghammer
:x: TassiloSinghammerVolue
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Aug 17 '22 14:08 CLAassistant

@PascalSenn , case insensitive methods (not only contains but everything like equality and so on) is what we are also interested in. But is there an issue in github to see the discussions and examples of the new API?

ademchenko avatar Aug 17 '22 19:08 ademchenko

Is this PR likely to make it to master/main now ?

mickdelaney avatar Dec 06 '22 11:12 mickdelaney

We are facing the same issue and would be happy if such an option is avaiable in the main branch. How can we contribute that this PR will be merged into main?

marcelbeutner avatar May 08 '23 11:05 marcelbeutner

@PascalSenn

I spent some time looking into this, and:

  • The default collation for MySQL and MS SQL Server is case-insensitive, and the default for PostgreSQL and SQLite is case-sensitive.
  • This means that the contains filter is not always case-sensitive – it depends on the collation of the database server, and/or the specific function used by the EF provider (LIKE, ILIKE, instr, etc.).
  • Adding a case-insensitive contains filter doesn't make sense, since MySQL and MS SQL Server are already case-insensitive (by default), so you'd have two case-insensitive filters.
  • Even if you did implement a case-insensitive filter, it would either be DB-specific (f.e. ILIKE on PostgreSQL), or use a hack like lower-casing both sides.

For these reasons, it probably isn't practical to add a case-insensitive contains filter to Hot Chocolate.

Depending on the specific situation, a developer can always add a custom filter, or install one via a NuGet package.

To add to this, it's generally better to avoid contains filters entirely, as they cannot utilize indexes. Better options include full-text indexes or search engines like Elastic/OpenSearch.

(I therefore suggest closing this PR. 🙂)

glen-84 avatar Jan 13 '24 14:01 glen-84

Closing for the above reasons.

glen-84 avatar Feb 16 '24 18:02 glen-84