Skip to content
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

checks for comments, exclusion constraints, array set_eq, and rls #306

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dmfay
Copy link

@dmfay dmfay commented Mar 17, 2023

hello! We've added a few checks at Beacon:

  • whether RLS is enabled for a table
  • array-to-array set_eq
  • whether a column array has an exclusion constraint set
  • whether multiline comments contain a given line (checking for PostGraphile smart tags)

There are a few gaps (e.g. single-column exclusion constraints still have to be provided as an array) but if these are good for inclusion I can always add the overloads. What else needs to happen?

Copy link
Owner

@theory theory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

col_has_exclusion is very welcome, and I like the idea of testing comment contents. The array comparison function seems superfluous, though.

Would you add tests for all of the few functions too, please? Will need to ensure they work across all supported OSes and Postgres versions; the automated testing workflow does that.

Thanks, love seeing new stuff here!

doc/pgtap.mmd Outdated Show resolved Hide resolved
Comment on lines +7993 to +7994
Test whether [row-level security](https://www.postgresql.org/docs/current/ddl-rowsecurity.html)
is enabled (`:desired_value` true) or disabled (`:desired_value` false).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description should go after the parameters to be consistent with the existing docs. Same goes for some of the other documentation added here.

-- col_has_exclusion(schema, table, columns, description)
CREATE OR REPLACE FUNCTION col_has_exclusion(TEXT, TEXT, TEXT[], TEXT)
RETURNS TEXT AS $$
SELECT ok(array_agg(attr.attname)::TEXT[] @> $3 AND $3 @> array_agg(attr.attname)::TEXT[])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the @> operator will work on Postgres 9.1. Would you add tests for these new functions?

sql/pgtap.sql.in Outdated
-- set_eq( array, array, description )
CREATE OR REPLACE FUNCTION set_eq(anyarray, anyarray, TEXT)
RETURNS TEXT AS $$
SELECT ok($1 @> $2 AND $2 @> $1, $3);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems superfluous. Why not just use is(array1, array2)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is(a1, a2) is order-dependent but the order of our arrays is not guaranteed. Is there something else that would suit, short of unnesting both sides?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I hadn't thought of that, of course! I like it! Would be useful to have something in the docs that talks about that difference.

@@ -11366,3 +11390,65 @@ RETURNS TEXT AS $$
'Function ' || quote_ident($1) || '() should not be a procedure'
);
$$ LANGUAGE sql;

-- table_comment_has(schema, table, comment, description)
CREATE OR REPLACE FUNCTION table_comment_has(TEXT, TEXT, TEXT, TEXT)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WE use _has more to indicate that a database has an object, like a table. I think a better term to use here would be _contains. Or perhaps _matches and use a regular expression? That could help avoid variations in line endings across platforms.

dmfay and others added 2 commits March 20, 2023 16:27
Co-authored-by: David E. Wheeler <46604+theory@users.noreply.github.com>
@dmfay dmfay marked this pull request as draft April 2, 2023 20:46
@rodo
Copy link
Contributor

rodo commented Sep 24, 2024

@dmfay I've found this PR just before starting to work on some functions to check comments, what are your intention about this one ?

@rodo rodo mentioned this pull request Sep 24, 2024
@dmfay
Copy link
Author

dmfay commented Sep 24, 2024

@dmfay I've found this PR just before starting to work on some functions to check comments, what are your intention about this one ?

I have not been able to prioritize working on this and this is on track to continue for the foreseeable future. If you'd like to use the comment functions in whole or in part please be my guest!

@theory
Copy link
Owner

theory commented Nov 5, 2024

Hi, what's the status of this PR? Is it ready for review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants