#1785 (backport of #1782) broke some like queries on SQL DBs

[FedericoMassaioli]

Starting from loopback-datasource-juggler@3.33.2 queries using like, nlike, ilike, and nilike operators on SQL DBs (PostgreSQL in my case) fails, because the presence of parentheses (like in {where: {name: {nlike: 'Internal service (%'}}) or other characters forbidden by the escapeRegEx() cause the rewrite of the query string and the query to fail.

[dhmlau]

@mitsos1os, thanks for taking a look at this issue.

[mitsos1os]

Hi @FedericoMassaioli . Could you please give me some extra details about the problem you are facing since I am not extremely familiar with PostgreSQL?

• Isn't nlike supposed to accept a RegExp expression?
• What would be the expected query to end up in PostgreSQL DB?

Thanks

[mitsos1os]

Ok nevermind, I saw how the nlike is translated to PostgreSQL here

So since the actual values in the like, nlike, etc.. are not expected to be RegExp in all DB systems, should this functioality be removed and moved to connector specific logic as @FedericoMassaioli suggested?

What do you think @bajtos ?

[FedericoMassaioli]

@mitsos1os is not a PostgreSQL only thing, the like operator & its friends are standard SQL, so the issue affects all connectors to SQL DBs.

[FedericoMassaioli]

@mitsos1os is not a PostgreSQL only thing, the like operator & its friends are standard SQL, so the issue affects all connectors to SQL DBs.

After looking at the code you referenced, I devised the workaround of escaping with a double backslash the characters that trigger escapeRegEx(). I wonder if this works with connectors to different SQL DBs, though.

[mitsos1os]

@FedericoMassaioli you are right. My SQL was a bit rusty 😄 ...

After looking at the code you referenced, I devised the workaround of escaping with a double backslash the characters that trigger escapeRegEx(). I wonder if this works with connectors to different SQL DBs, though.

Without looking into it, I think that is the same result with escapeRegEx... Does it produce the DB results you want this way?

The thing is I did the fix while using MongoDB and also got carried away by the documentation which references like operators to be used with Regular Expressions https://loopback.io/doc/en/lb3/Where-filter.html#operators so maybe @dhmlau this needs to be revised as well..

So I believe a decision should be taken regarding the usage of the like operators. I believe since they end up in different DB systems without the need to be valid Regular Expressions, we need to take the path of per connector logic.

Let's take @bajtos opinion as well and I can then move on to the fix

[FedericoMassaioli]

Without looking into it, I think that is the same result with escapeRegEx... Does it produce the DB results you want this way?

It works with the escape I added. When escapeRegEx() sanitized the original string it didn't return the same results, but this was discovered on a live DB, undergoing modifications to the data in records targeted by the queries, I need to find time for a more complete analysis.
IF all connectors to SQL DBs take care to properly set the escape character to \ in LIKE strings, that would work.

For sure, the documentation is too terse on this aspect, there is no mention of the need to escape, or what character to escape, not even in the PostgreSQL specific section https://loopback.io/doc/en/lb3/Where-filter.html#ilike-and-nilike.
Moreover, wrt. documentation the console warning emitted by escapeRegEx() and caught by the log collector appears illogical.

And wrt. to tests, I think that this slipped by because no SQL tests are present in the test suite, as I commented in #1785
You can do a lot of 'noSQL' data management with modern SQL DBs, getting the best of both worlds, so do not discard them lightly ;-)

[mitsos1os]

It works with the escape I added. When escapeRegEx() sanitized the original string it didn't return the same results, but this was discovered on a live DB, undergoing modifications to the data in records targeted by the queries, I need to find time for a more complete analysis.

@FedericoMassaioli I would appreciate it if you could have the chance if it actually produced the same results.

IF all connectors to SQL DBs take care to properly set the escape character to \ in LIKE strings, that would work.

Connector logic is deeper in the execution since this escape is done in DAO layer before calling the connector methods... It will probably need to be removed...

[FedericoMassaioli]

@mitsos1os I had extensive checks made in a controlled situation and, as far as PostgreSQL is concerned, the escape is not harmful and results are correct, only an annoying and hard to explain console warning.
So, wrt. PostgreSQL, it reduces to a documentation issue: the documentation should be made more consistent with the implementation, the spurious console warning could be changed.

However, I had a look at source code of other SQL DBs loopback connectors (I do not have time or DBMSs available at the moment to perform actual tests), with the following results:

• a \ escape for like expressions is explicitly enforced for MS SQL Server, Oracle, and PostgreSQL, so none of them should experience query errors, just a surprising console warning;
• no escape character for like expressions is explicitly enforced for Informix, MySQL, and SQLite3, all of them anyhow default to \, again no query errors, just a surprising console warning;
• no escape character for like expressions is explicitly enforced for DB2, which to my knowledge has no default. Problems are foreseeable in this combination.

Moreover, let me make another remark.
Let's imagine you have LB running against memory and MongoDB datasources. basically, if something is wrong with the regex, escapeRegEx() will escape every control character, even the one which appears in a correct combination. Albeti the query will proceed smoothly to its completion, the resulting, escaped regex would probably not match what originally intended, but something different.
Now, if the developer/user is lucky, completely unexpected results will be returned, and the problem with the query will be detected. If, however, empty results or results similar to expectations are returned, the developer/user could even not notice. Unless it looks carefully at the console log, which is maybe overabundant for some other reason.
Wouldn't it be better if LB refused such a query returning a clear, meaningful error response?

FWIW in this scenario I'd definitely prefer to be rejected with an error response.

[mitsos1os]

@FedericoMassaioli Thank you for taking the time to perform the tests.

We wad a similar discussion when first implementing the issue. So since this is an architectural decision let's wait to get an extra opinion on how to proceed.

@dhmlau What do you think?

[dhmlau]

sounds good.
Looping in @bajtos @raymondfeng @jannyHou @emonddr who commented/reviewed the original PR: #1782.