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

[MCC-833873] Fix single pg call for accessible_objects from replica #192

Merged
merged 5 commits into from
Nov 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
# Changelog

## 4.0.2
* Updated `accessible_objects_for_operations` code path to use a CTE when using a replica database.

## 4.0.1
* Updated PostgreSQL function `pm_accessible_objects_for_operations` to ensure uniqueness of results
* Updated PostgreSQL function `pm_accessible_objects_for_operations` to ensure uniqueness of results.

## 4.0.0
* Added a PostgreSQL function for `#accessible_objects` and `#accessible_objects_for_operations` which are performance
* Added a PostgreSQL function for `#accessible_objects` and `#accessible_objects_for_operations` which are performance.
optimized. Only supported for a single `field`, `direct_only`, and `ignore_prohibitions`.

– Execute `bundle exec rails generate the_policy_machine:accessible_objects_for_operations_function` and rerun
Expand Down
2 changes: 1 addition & 1 deletion lib/policy_machine/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
class PolicyMachine
VERSION = "4.0.1"
VERSION = "4.0.2"
end
8 changes: 8 additions & 0 deletions lib/policy_machine_storage_adapters/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ def self.db_config
end
end

def self.connection_db_config
if ::ActiveRecord::Base.respond_to?(:connection_db_config)
::ActiveRecord::Base.connection_db_config.configuration_hash
else
::ActiveRecord::Base.connection_config
end
end

def self.buffering?
@buffering
end
Expand Down
129 changes: 122 additions & 7 deletions lib/policy_machine_storage_adapters/active_record/postgresql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,22 @@ def self.accessible_objects_for_operations(user_id, operation_names, options)
field = options[:fields].first
filters = options.dig(:filters, :user_attributes) || {}

query = sanitize_sql_for_assignment([
'SELECT * FROM pm_accessible_objects_for_operations(?,?,?,?)',
user_id,
PG::TextEncoder::Array.new.encode(operation_names),
field,
JSON.dump(filters)
])
query =
if replica?
sanitize_sql_for_assignment([
accessible_objects_for_operations_cte(field, filters),
user_id,
operation_names
])
else
sanitize_sql_for_assignment([
'SELECT * FROM pm_accessible_objects_for_operations(?,?,?,?)',
user_id,
PG::TextEncoder::Array.new.encode(operation_names),
field,
JSON.dump(filters)
])
end

# [
# { 'unique_identifier' => 'op1', 'objects' => '{obj1,obj2,obj3}' },
Expand All @@ -105,6 +114,112 @@ def self.accessible_objects_for_operations(user_id, operation_names, options)
output[key] = objects
end
end

private

# For replica database connections which do not support temporary tables.
# This is a little slower than the PG function but still quicker than the
# existing ActiveRecord code path.
def self.accessible_objects_for_operations_cte(field, filters)
<<~SQL.squish
SET LOCAL enable_mergejoin TO FALSE;

WITH RECURSIVE "user" AS (
SELECT *
FROM policy_elements
WHERE
id = ?
),
user_attribute_ids AS (
(
SELECT
child_id,
parent_id
FROM assignments
WHERE parent_id = (SELECT id FROM "user")
)
UNION ALL
(
SELECT
a.child_id,
a.parent_id
FROM
assignments a
JOIN user_attribute_ids ua_id ON ua_id.child_id = a.parent_id
)
),
operation_set_ids AS (
SELECT
operation_set_id,
object_attribute_id
FROM policy_element_associations
WHERE user_attribute_id IN (SELECT child_id FROM user_attribute_ids #{accessible_object_filters(filters)})
),
accessible_operations AS (
(
SELECT
child_id,
parent_id AS operation_set_id
FROM assignments
WHERE parent_id IN (SELECT operation_set_id FROM operation_set_ids)
)
UNION ALL
(
SELECT
a.child_id,
op.operation_set_id AS operation_set_id
FROM
assignments a
JOIN accessible_operations op ON op.child_id = a.parent_id
)
),
operation_sets AS (
SELECT DISTINCT ao.operation_set_id, ops.unique_identifier
FROM
accessible_operations ao
JOIN policy_elements ops ON ops.id = ao.child_id
WHERE
ops.unique_identifier IN (?)
),
operation_objects AS (
SELECT
os.unique_identifier,
array_remove(array_agg(
(
SELECT pe.#{connection.quote_column_name(field)}
FROM policy_elements pe
WHERE
pe.id = os_id.object_attribute_id
AND "type" = 'PolicyMachineStorageAdapter::ActiveRecord::Object'
)
), NULL) AS objects
FROM
operation_set_ids os_id
JOIN operation_sets os ON os.operation_set_id = os_id.operation_set_id
GROUP BY os.unique_identifier
)
SELECT
unique_identifier,
ARRAY(SELECT DISTINCT o FROM UNNEST(objects) AS a(o)) as objects
FROM operation_objects;
SQL
end

def self.accessible_object_filters(filters)
return '' if filters.blank?

condition = 'WHERE child_id IN (SELECT id FROM policy_elements WHERE '

filters.each do |key, value|
condition << sanitize_sql_for_assignment(["#{connection.quote_column_name(key)} = ? AND ", value])
end

condition.chomp('AND ') << ')'
end

def self.replica?
ActiveRecord.connection_db_config[:replica] == true
end
end

class PolicyElementAssociation
Expand Down
2 changes: 1 addition & 1 deletion policy_machine.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@ Gem::Specification.new do |s|
s.add_development_dependency('byebug')
s.add_development_dependency('neography', '~> 1.1')
s.add_development_dependency('database_cleaner')
s.add_development_dependency('rails', '~> 6.0')
s.add_development_dependency('rails', '~> 6.1')
Copy link
Contributor

Choose a reason for hiding this comment

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

why the bump? asking cause im wondering if this is indeed a development dependency or actually a runtime dependency

Copy link
Contributor Author

@kjerinsky-mdsol kjerinsky-mdsol Nov 15, 2021

Choose a reason for hiding this comment

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

Because ActiveRecord::Base.connection_db_config.config is logging from Rails 6.1

DEPRECATION WARNING: DatabaseConfig#config will be removed in 6.2.0 in favor of DatabaseConfigurations#configuration_hash which returns a hash with symbol keys (called from irb_binding at (irb):20)

Rails 6.0 doesn't have DatabaseConfigurations#configuration_hash so either get ahead of it or let it potentially sneak up on us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That or check for either method.

Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is mainly for the test Rails app that is created for running specs I think. If we really cared about this gem we would make it use appraisals for various Ruby/Rails combinations... but, to me, just using the same version as Dalto is fine.

end
55 changes: 41 additions & 14 deletions spec/policy_machine_storage_adapters/active_record_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -741,25 +741,12 @@
end
end

describe 'accessible_objects_for_operations_function' do
shared_examples 'a single query for accessible objects' do
before do
priv_pm.add_association(color_1, creator, object_6)
priv_pm.add_association(color_2, creator, object_7)
end

it 'uses a PostgreSQL function' do
expect_any_instance_of(PolicyMachineStorageAdapter::ActiveRecord)
.to receive(:accessible_objects_for_operations_function)

priv_pm.accessible_objects_for_operations(
user_1,
[create, paint],
direct_only: true,
ignore_prohibitions: true,
fields: [:unique_identifier]
)
end

it 'returns objects accessible via each of multiple given operations' do
result = priv_pm.accessible_objects_for_operations(
user_1,
Expand Down Expand Up @@ -859,6 +846,46 @@
end
end

describe 'accessible_objects_for_operations_function' do
it_behaves_like 'a single query for accessible objects'

it 'uses a PostgreSQL function' do
expect_any_instance_of(PolicyMachineStorageAdapter::ActiveRecord)
.to receive(:accessible_objects_for_operations_function)

priv_pm.accessible_objects_for_operations(
user_1,
[create, paint],
direct_only: true,
ignore_prohibitions: true,
fields: [:unique_identifier]
)
end
end

describe 'accessible_objects_for_operations_cte' do
before do
allow(PolicyMachineStorageAdapter::ActiveRecord::PolicyElement)
.to receive(:replica?).and_return(true)
end

it_behaves_like 'a single query for accessible objects'

it 'uses a single CTE' do
expect(PolicyMachineStorageAdapter::ActiveRecord::PolicyElement)
.to receive(:accessible_objects_for_operations_cte)
.and_call_original

priv_pm.accessible_objects_for_operations(
user_1,
[create, paint],
direct_only: true,
ignore_prohibitions: true,
fields: [:unique_identifier]
)
end
end

describe 'accessible_ancestor_objects' do
context 'when policy element associations are not provided as an argument' do
it 'returns objects accessible via the filtered attribute on an object scope' do
Expand Down