I am not sure how to force a query to be in read-only mode, since run-sql is by definition an admin-only command. I am not sure how to test this, as a result.

Could you write a test that runs your example select attisdropped from pg_attribute query via run_sql and ensures it doesn’t fail? I’m not sure I quite understand why that wouldn’t work.

Review app for commit 5124945 deployed to Heroku: https://hge-ci-pull-4250.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4250-5124945e

I’m not sure I quite understand why that wouldn’t work.

To be completely honest, I don't quite understand yet either. But it's the reason the tests in #4204 kept failing. Upon investigating, I found this error message: a read-only query that matches "drop". This causes the code to treat it as as an alteration, to run the metadata check... and then it fails with an internal postgres error.

The only thing I don't know is why / when the console marks a query as being read only. But while writing that comment, I checked: it is simply a valid argument for run_sql, which means I know now how to write a test. ^^

Reading the code, AFAICT, the regex is only used to determine whether it can skip rebuilding the schema cache—executing the query in READ ONLY mode is determined elsewhere. So while this PR does appear to be an improvement—it makes the check a little less stupid—false positives should be okay. The check matching is the case where we take the slow path, so matching too much is alright; it’s just matching too little that’s bad.

Review app for commit f227328 deployed to Heroku: https://hge-ci-pull-4250.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4250-f227328c

Here's a short write-up of the investigation.

replicating the bug

Imagine the two following run_sql commands are sent to the server:

- type: run_sql
  args:
    sql: "create view v as select 1;"
- type: run_sql
  args:
    read_only: True
    sql: "select attisdropped from pg_attribute;"

The first one only creates a trivial view. What is interesting is what happens with the second one:

• as it is read only, the code (correctly) identifies it as being a request that does not modify the schema cache
• the code therefore opens the connection to postgres in read_only mode
• due to the erroneous regex this patch fixes, the query is marked as requiring a metadata check
• the metadata check starts by clearing all hdb views
• which, for each of them, issues a DROP; since we have one view, one DROP query is sent to postgres
• we're in read-only mode => internal postgres error, the query fails

tl;dr: when hdb_views is not empty (1), a read_only (2) run_sql query that match words such as "drop" or "alter" (3) will erroneously trigger a metadata check and issue a drop while in read-only mode.

fixing the bug

This PR offers a very simple solution to the problem: make the regex check for word boundaries. This is a quick-fix and it is not enough. The problem would still arise if one of the "bad words" were to be included in a string in the query, such as: select * from q where f = ' drop '.

There are three ways we could choose to tackle this; note that they are not mutually exclusive:

1. make the check more robust: use a full sql parser to properly identify whether the query requires a metadata check or not
2. use the read_only information to override that check: we only perform metadata checks if we're not in read only mode AND the query seems to match a drop / alter
3. we change metadata checks so that they don't alter the schema cache

This PR goes towards (1), making the check more robust. I am not sure if / how the two other points are reasonable.

There are three ways we could choose to tackle this; note that they are not mutually exclusive:

1. make the check more robust: use a full sql parser to properly identify whether the query requires a metadata check or not
2. use the read_only information to override that check: we only perform metadata checks if we're not in read only mode AND the query seems to match a drop / alter
3. we change metadata checks so that they don't alter the schema cache

This PR goes towards (1), making the check more robust. I am not sure if / how the two other points are reasonable.

I think that doing (1) is a Good Thing, since it’s clearly an improvement over the old code. However, I think that doing it alone is a bit sketchy, since it doesn’t actually fix the bug—it just sweeps it under the rug.

The imprecision of the regex is a red herring, since the code was written with the intent that the regex would enable a conservative optimization, so any regex at all ought to do! The code ought to work if the regex were .*. Anything else is a bug.

So I think we have to do (2) as well. We’ve discovered the bug, and the fix is easy, so we should fix it now! All we need to do is guard the use of withMetadataCheck with an additional check to see if we’re in ReadOnly mode or not. We can include the improvement to the regex as well, of course, but then we can claim we’re actually fixing the bug.

I've added the check for read-only mode, added more tests, and rewrote the PR description. I am unsure whether this needs a changelog entry; it is only a minor bug, in the grand scheme of things... What do you think?

Review app for commit 7259821 deployed to Heroku: https://hge-ci-pull-4250.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4250-72598214

Deploy preview for hasura-docs ready!

Built with commit 2b5073c

https://deploy-preview-4250--hasura-docs.netlify.com

Review app for commit 288bd6b deployed to Heroku: https://hge-ci-pull-4250.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4250-288bd6b2

Review app https://hge-ci-pull-4250.herokuapp.com is deleted