optimise 'run_sql' query #1406
optimise 'run_sql' query #1406
Conversation
|
Review app for commit 8bd39ac deployed to Heroku: https://hge-ci-pull-1406.herokuapp.com |
| let e = err400 PostgresError "query execution failed" | ||
| in e {qeInternal = Just $ toJSON txe} | ||
| throwErr s = throw500 $ "compiling regex failed: " <> T.pack s | ||
| regex = "alter table|drop table|alter view|drop view|replace view" |
There was a problem hiding this comment.
why not just have "alter|drop|replace"?
There was a problem hiding this comment.
This regex will catch all SQL having alter, drop and replace words. We explicitly need alter table, replace view etc.
There was a problem hiding this comment.
Our metadata can get inconsistent by dropping schemas/constraints too. Instead of explicitly listing all the objects it will be simpler to just use alter|drop|replace.
| _3 (_, _, z) = z | ||
|
|
||
| -- regex related | ||
| matchRegex :: String -> Bool -> T.Text -> Either String Bool |
There was a problem hiding this comment.
Text -> Bool -> Text -> Either String Bool
Use the Bytestring module of TDFA instead of String internally.
| , rCascade :: !(Maybe Bool) | ||
| { rSql :: T.Text | ||
| , rCascade :: !(Maybe Bool) | ||
| , rEnforceStateCheck :: !(Maybe Bool) |
There was a problem hiding this comment.
Let's call this check_metadata_consistency.
| adminOnly | ||
| encode <$> bool withoutMDChk runP2 chkMDCnstcy | ||
| where | ||
| chkMDCnstcy = fromMaybe False mChkMDCnstcy |
There was a problem hiding this comment.
We have to treat absence of check_metadata_consistency key differently from explicitly specifying it to False.
Cases
Nothing -> we do a check optimistically based on regex
Just True -> definitely enforce the check
Nothing -> do not enforce the check (i.e, we need not check what the sql contains)
There was a problem hiding this comment.
I assume last case is Just False
| let e = err400 PostgresError "query execution failed" | ||
| in e {qeInternal = Just $ toJSON txe} | ||
|
|
||
| runSqlP2 |
There was a problem hiding this comment.
Let's rename this to execWithMetadataCheck
| adminOnly >> runSqlP2 q | ||
| runRunSQL q@(RunSQL t _ mChkMDCnstcy) = do | ||
| adminOnly | ||
| encode <$> case mChkMDCnstcy of |
There was a problem hiding this comment.
can be simplified further:
chkMdCnstncy <- case mChkMdCnstncy of
Nothing -> isAltrDropReplace t
Just b -> return b
encode <$> bool (execRawSQL t) (execWithCheck q) chkMdCnstncy
|
Review app for commit 62a69f7 deployed to Heroku: https://hge-ci-pull-1406.herokuapp.com |
|
Review app for commit 6ccb8d4 deployed to Heroku: https://hge-ci-pull-1406.herokuapp.com |
|
Just checking in to make sure we're doing case insensitive match. |
|
Review app https://hge-ci-pull-1406.herokuapp.com is deleted |
…enDD query planning (#1406) ### What Enforce type permissions while building sub-selection for a nested field in OpenDD query planning. ### How <!-- How is it trying to accomplish it (what are the implementation steps)? --> - While populating sub-selection for a nested field, include only accessible fields to the role. - Raise permission exception if the type is not accessible to the role. V3_GIT_ORIGIN_REV_ID: 86f81f469627ca6bcb2f34878dfb80baab02af73
Description
What component does this PR affect?
Requires changes from other components? If yes, please mark the components:
Related Issue
close #1362
Solution and Design
For
"type": "run_sql"in/v1/query, check whether inputSQLcontains anyDDLstatement of typeDROP,ALTERandREPLACE. If present, performschema diffand apply those changes in HGEmetadata. If not present, just execute and return response ofSQLstatement.Optional argument to enforce
schema diffcheck:-Introduce new optional field
check_metadata_consistency(takes bool value, defaultfalse) inargssection of payload. This enforces performingschema diffirrespective ofSQLstatement.Ex:-
Regex used to match against
SQLstring:-alter table|drop table|alter view|drop view|replace viewType
Checklist: