RLS Policies #132
RLS Policies #132
Conversation
bplunkett-stripe
force-pushed
the
bplunkett/rls-policies
branch
2 times, most recently
from
49771b3 to
e13edf1
Compare
bplunkett-stripe
force-pushed
the
bplunkett/rls-policies
branch
2 times, most recently
from
31e22e7 to
c75e3c3
Compare
bplunkett-stripe
force-pushed
the
bplunkett/rls-policies
branch
from
c75e3c3 to
7f95e4e
Compare
| // roles is a list of roles that should be created before the DDL is applied | ||
| roles []string |
There was a problem hiding this comment.
thought: I wonder if we should make it possible to define roles in schema somehow. Since they're global I figure that would erode the "you're only applying a single schema" paradigm we have, but it feels like a shame that roles have to be defined outside of pg-schema-diff.
| // ResetInstance attempts to reset the cluster to a clean state. | ||
| // It deletes all cluster level objects, i.e., roles, which are not deleted | ||
| // by dropping database(s). This can be useful for re-using a cluster for multiple tests. | ||
| func ResetInstance(ctx context.Context, db *sql.DB) error { |
There was a problem hiding this comment.
thought: Shouldn't this also drop databases? As is it feels like this is implemented specifically for how it's used in the AT suite.
There was a problem hiding this comment.
We actually have the AT's drop the database. This is more design cluster-scope values. I could totally see databases considered cluster-scope though... Right now, we will just keep the individual AT's dropping their own databases, since if they fail to do that, it indicates a bug.
| // Weirdly, you can't actually drop a "USING EXPRESSION" clause from an ALL policy even though you | ||
| // can have an ALL policy with only a check expression. |
There was a problem hiding this comment.
shouldn't there be an ErrNotImplemented somewhere to cover this case?
There was a problem hiding this comment.
Ah i see that's handled by if diff.old isn't mutated to match diff.new
There was a problem hiding this comment.
Yep. That's the "canonical" pattern I'm going for. It makes "ErrNotImplemented" free for any feature we inherently haven't explicitly coded support for.
| return nil, fmt.Errorf("getting target columns: %w", err) | ||
| } | ||
|
|
||
| // Run after the new columns are added/altered |
There was a problem hiding this comment.
Policies on partitions must be dropped before the base table is altered, otherwise the SQL could fail because, e.g., the policy references a column that no longer exists.
Is this quirk from above covered by the column dependencies?
There was a problem hiding this comment.
Ah maybe it's handled by how the policy generator is called by the table generator?
There was a problem hiding this comment.
Yes, this is a quirk covered by the column dependencies. It's also the same reason we can't easily support partition-level RLS policies yet
Description
Little status update: Bit of a hiatus due to other priorities. Before adding more features to schema diffing, I'd like to clean up the tests and code a bit.
cc @sweatybridge
Motivation
Fixes #111
Testing
Tested via unit tests