-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix insert permission views are not unique for long role names (fix #3444) #3486
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
Conversation
|
Deploy preview for hasura-docs ready! Built with commit bfb3e02 |
|
Review app for commit 954e507 deployed to Heroku: https://hge-ci-pull-3486.herokuapp.com |
a55b30e to
ac76946
Compare
|
Review app for commit ac76946 deployed to Heroku: https://hge-ci-pull-3486.herokuapp.com |
| tnTxt = getTableTxt tn | ||
| tableName = TableName $ T.pack $ show hash | ||
| -- Blake2b_224 generates 56 character length hash | ||
| hash :: CH.Digest CH.Blake2b_224 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thought: it isn’t immediately clear to someone reading this code why we’re doing this hashing. This might be a good situation for something like GHC’s “notes” system. The idea is to create a long-form comment somewhere, like this:
{- Note [Postgres identifier length limitations]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Postgres truncates identifiers to a maximum of 63 characters by default (see
https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS). This causes
problems for A, B, and C because of X and Y, so we work around it by doing Z. -}Except obviously A, B, C, etc. are replaced with real explanations. :) Then somewhere else in the code you can write
-- see Note [Postgres identifier length limitations]
hash :: CH.Digest CH.Blake2b_224 =wherever the note is relevant. That way, explanations for tricky concepts can be centralized in one place, and they can be referenced from other places in the code. I think there is already at least one or two places in graphql-engine where we do some work to work around Postgres’s identifier length limitation, so it would be great to put a note somewhere and add comments in all those places referencing it!
ac76946 to
bfb3e02
Compare
|
Review app for commit bfb3e02 deployed to Heroku: https://hge-ci-pull-3486.herokuapp.com |
lexi-lambda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the notes, they’re great. 🙂 The one about BLAKE2 is very helpful as well!
|
Review app https://hge-ci-pull-3486.herokuapp.com is deleted |
…asura#3444) (hasura#3486) * fix insert permission views are not unique for long role names, fix hasura#3444 * Use GHC notes reference and improve comments
Description
All PostgreSQL identifiers are limited by 63 character length and longer ones are truncated. See Postgres docs.
The server makes use of views to define an insert trigger with check permission. The auto-generated view name is of format
{role}__insert__{table-schema}__{table-name}. For the really long role names, the generated view names may exceed 63 characters and truncated names may not be unique.This PR fixes the above issue by using hash string instead of formatted text.
Affected components
Related Issues
Fix #3444
Solution and Design
Use Blake2b_224 algorithm, which generates 56 character length hash for insert permission view names.
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
GraphQL
Breaking changes
Steps to test and verify
Define insert permissions with long role names which differ in the tail characters (> 63 characters).
Limitations, known bugs & workarounds