这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@rakeshkky
Copy link
Member

@rakeshkky rakeshkky commented Dec 4, 2019

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

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Build System
  • Tests
  • Other (list it)

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?

  • No
  • Yes

Metadata

Does this PR add a new Metadata feature?

  • No
  • Yes

GraphQL

  • No new GraphQL schema is generated
  • New GraphQL schema is being generated:

Breaking changes

  • No Breaking changes
  • There are 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

@rakeshkky rakeshkky added the c/server Related to server label Dec 4, 2019
@rakeshkky rakeshkky self-assigned this Dec 4, 2019
@netlify
Copy link

netlify bot commented Dec 4, 2019

Deploy preview for hasura-docs ready!

Built with commit bfb3e02

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

@hasura-bot
Copy link
Contributor

Review app for commit 954e507 deployed to Heroku: https://hge-ci-pull-3486.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3486-954e507f

@rakeshkky rakeshkky force-pushed the issue-3444-ins-perm-view branch 2 times, most recently from a55b30e to ac76946 Compare December 5, 2019 09:44
@hasura-bot
Copy link
Contributor

Review app for commit ac76946 deployed to Heroku: https://hge-ci-pull-3486.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3486-ac76946c

@rakeshkky rakeshkky marked this pull request as ready for review December 5, 2019 10:00
tnTxt = getTableTxt tn
tableName = TableName $ T.pack $ show hash
-- Blake2b_224 generates 56 character length hash
hash :: CH.Digest CH.Blake2b_224 =
Copy link
Contributor

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!

@rakeshkky rakeshkky force-pushed the issue-3444-ins-perm-view branch from ac76946 to bfb3e02 Compare December 9, 2019 08:23
@rakeshkky rakeshkky requested a review from lexi-lambda December 9, 2019 08:25
@hasura-bot
Copy link
Contributor

Review app for commit bfb3e02 deployed to Heroku: https://hge-ci-pull-3486.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3486-bfb3e028

Copy link
Contributor

@lexi-lambda lexi-lambda left a 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!

@lexi-lambda lexi-lambda merged commit 3f8a1d9 into hasura:master Dec 9, 2019
@hasura-bot
Copy link
Contributor

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

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c/server Related to server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permission metadata name cropped after 63 characters

3 participants