From 2542c713ea4a0d686d7b7ceae352929b60a80806 Mon Sep 17 00:00:00 2001 From: Mike Rylander Date: Tue, 4 Mar 2014 15:47:12 -0500 Subject: [PATCH] LP#1277731: Disambiguate parameter/column names Due to slightly different and more strict disabiguation rules for parameters and column names in PLPGSQL in modern versions of Postgres, our hold matrix functions were failing in 9.3. This commit attempts to address that by declaring and using aliases for parameters that share names with columns used in in-line SQL. Signed-off-by: Mike Rylander Signed-off-by: Dan Wells --- Open-ILS/src/sql/Pg/110.hold_matrix.sql | 18 +- .../XXXX.function.hold_test_params.sql | 387 ++++++++++++++++++ 2 files changed, 398 insertions(+), 7 deletions(-) create mode 100644 Open-ILS/src/sql/Pg/upgrade/XXXX.function.hold_test_params.sql diff --git a/Open-ILS/src/sql/Pg/110.hold_matrix.sql b/Open-ILS/src/sql/Pg/110.hold_matrix.sql index 25ac26491d..8f6518ba38 100644 --- a/Open-ILS/src/sql/Pg/110.hold_matrix.sql +++ b/Open-ILS/src/sql/Pg/110.hold_matrix.sql @@ -74,6 +74,8 @@ DECLARE matchpoint config.hold_matrix_matchpoint%ROWTYPE; weights config.hold_matrix_weights%ROWTYPE; denominator NUMERIC(6,2); + v_pickup_ou ALIAS FOR pickup_ou; + v_request_ou ALIAS FOR request_ou; BEGIN SELECT INTO user_object * FROM actor.usr WHERE id = match_user; SELECT INTO requestor_object * FROM actor.usr WHERE id = match_requestor; @@ -157,8 +159,8 @@ BEGIN FROM config.hold_matrix_matchpoint m /*LEFT*/ JOIN permission.grp_ancestors_distance( requestor_object.profile ) rpgad ON m.requestor_grp = rpgad.id LEFT JOIN permission.grp_ancestors_distance( user_object.profile ) upgad ON m.usr_grp = upgad.id - LEFT JOIN actor.org_unit_ancestors_distance( pickup_ou ) puoua ON m.pickup_ou = puoua.id - LEFT JOIN actor.org_unit_ancestors_distance( request_ou ) rqoua ON m.request_ou = rqoua.id + LEFT JOIN actor.org_unit_ancestors_distance( v_pickup_ou ) puoua ON m.pickup_ou = puoua.id + LEFT JOIN actor.org_unit_ancestors_distance( v_request_ou ) rqoua ON m.request_ou = rqoua.id LEFT JOIN actor.org_unit_ancestors_distance( item_cn_object.owning_lib ) cnoua ON m.item_owning_ou = cnoua.id LEFT JOIN actor.org_unit_ancestors_distance( item_object.circ_lib ) iooua ON m.item_circ_ou = iooua.id LEFT JOIN actor.org_unit_ancestors_distance( user_object.home_ou ) uhoua ON m.user_home_ou = uhoua.id @@ -237,9 +239,11 @@ DECLARE context_org_list INT[]; done BOOL := FALSE; hold_penalty TEXT; + v_pickup_ou ALIAS FOR pickup_ou; + v_request_ou ALIAS FOR request_ou; BEGIN SELECT INTO user_object * FROM actor.usr WHERE id = match_user; - SELECT INTO context_org_list ARRAY_AGG(id) FROM actor.org_unit_full_path( pickup_ou ); + SELECT INTO context_org_list ARRAY_AGG(id) FROM actor.org_unit_full_path( v_pickup_ou ); result.success := TRUE; @@ -270,7 +274,7 @@ BEGIN RETURN; END IF; - SELECT INTO matchpoint_id action.find_hold_matrix_matchpoint(pickup_ou, request_ou, match_item, match_user, match_requestor); + SELECT INTO matchpoint_id action.find_hold_matrix_matchpoint(v_pickup_ou, v_request_ou, match_item, match_user, match_requestor); result.matchpoint := matchpoint_id; SELECT INTO ou_skip * FROM actor.org_unit_setting WHERE name = 'circ.holds.target_skip_me' AND org_unit = item_object.circ_lib; @@ -344,7 +348,7 @@ BEGIN SELECT INTO transit_source * FROM actor.org_unit WHERE id = item_object.circ_lib; END IF; - PERFORM * FROM actor.org_unit_descendants( transit_source.id, transit_range_ou_type.depth ) WHERE id = pickup_ou; + PERFORM * FROM actor.org_unit_descendants( transit_source.id, transit_range_ou_type.depth ) WHERE id = v_pickup_ou; IF NOT FOUND THEN result.fail_part := 'transit_range'; @@ -417,9 +421,9 @@ BEGIN IF age_protect_date + age_protect_object.age > NOW() THEN IF hold_test.distance_is_from_owner THEN SELECT INTO item_cn_object * FROM asset.call_number WHERE id = item_object.call_number; - SELECT INTO hold_transit_prox prox FROM actor.org_unit_proximity WHERE from_org = item_cn_object.owning_lib AND to_org = pickup_ou; + SELECT INTO hold_transit_prox prox FROM actor.org_unit_proximity WHERE from_org = item_cn_object.owning_lib AND to_org = v_pickup_ou; ELSE - SELECT INTO hold_transit_prox prox FROM actor.org_unit_proximity WHERE from_org = item_object.circ_lib AND to_org = pickup_ou; + SELECT INTO hold_transit_prox prox FROM actor.org_unit_proximity WHERE from_org = item_object.circ_lib AND to_org = v_pickup_ou; END IF; IF hold_transit_prox > age_protect_object.prox THEN diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.function.hold_test_params.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.function.hold_test_params.sql new file mode 100644 index 0000000000..f51aa470fb --- /dev/null +++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.function.hold_test_params.sql @@ -0,0 +1,387 @@ +BEGIN; + +SELECT evergreen.upgrade_deps_block_check('XXXX', :eg_version); + +CREATE OR REPLACE FUNCTION action.find_hold_matrix_matchpoint(pickup_ou integer, request_ou integer, match_item bigint, match_user integer, match_requestor integer) + RETURNS integer AS +$func$ +DECLARE + requestor_object actor.usr%ROWTYPE; + user_object actor.usr%ROWTYPE; + item_object asset.copy%ROWTYPE; + item_cn_object asset.call_number%ROWTYPE; + my_item_age INTERVAL; + rec_descriptor metabib.rec_descriptor%ROWTYPE; + matchpoint config.hold_matrix_matchpoint%ROWTYPE; + weights config.hold_matrix_weights%ROWTYPE; + denominator NUMERIC(6,2); + v_pickup_ou ALIAS FOR pickup_ou; + v_request_ou ALIAS FOR request_ou; +BEGIN + SELECT INTO user_object * FROM actor.usr WHERE id = match_user; + SELECT INTO requestor_object * FROM actor.usr WHERE id = match_requestor; + SELECT INTO item_object * FROM asset.copy WHERE id = match_item; + SELECT INTO item_cn_object * FROM asset.call_number WHERE id = item_object.call_number; + SELECT INTO rec_descriptor * FROM metabib.rec_descriptor WHERE record = item_cn_object.record; + + SELECT INTO my_item_age age(coalesce(item_object.active_date, now())); + + -- The item's owner should probably be the one determining if the item is holdable + -- How to decide that is debatable. Decided to default to the circ library (where the item lives) + -- This flag will allow for setting it to the owning library (where the call number "lives") + PERFORM * FROM config.internal_flag WHERE name = 'circ.holds.weight_owner_not_circ' AND enabled; + + -- Grab the closest set circ weight setting. + IF NOT FOUND THEN + -- Default to circ library + SELECT INTO weights hw.* + FROM config.weight_assoc wa + JOIN config.hold_matrix_weights hw ON (hw.id = wa.hold_weights) + JOIN actor.org_unit_ancestors_distance( item_object.circ_lib ) d ON (wa.org_unit = d.id) + WHERE active + ORDER BY d.distance + LIMIT 1; + ELSE + -- Flag is set, use owning library + SELECT INTO weights hw.* + FROM config.weight_assoc wa + JOIN config.hold_matrix_weights hw ON (hw.id = wa.hold_weights) + JOIN actor.org_unit_ancestors_distance( item_cn_object.owning_lib ) d ON (wa.org_unit = d.id) + WHERE active + ORDER BY d.distance + LIMIT 1; + END IF; + + -- No weights? Bad admin! Defaults to handle that anyway. + IF weights.id IS NULL THEN + weights.user_home_ou := 5.0; + weights.request_ou := 5.0; + weights.pickup_ou := 5.0; + weights.item_owning_ou := 5.0; + weights.item_circ_ou := 5.0; + weights.usr_grp := 7.0; + weights.requestor_grp := 8.0; + weights.circ_modifier := 4.0; + weights.marc_type := 3.0; + weights.marc_form := 2.0; + weights.marc_bib_level := 1.0; + weights.marc_vr_format := 1.0; + weights.juvenile_flag := 4.0; + weights.ref_flag := 0.0; + weights.item_age := 0.0; + END IF; + + -- Determine the max (expected) depth (+1) of the org tree and max depth of the permisson tree + -- If you break your org tree with funky parenting this may be wrong + -- Note: This CTE is duplicated in the find_circ_matrix_matchpoint function, and it may be a good idea to split it off to a function + -- We use one denominator for all tree-based checks for when permission groups and org units have the same weighting + WITH all_distance(distance) AS ( + SELECT depth AS distance FROM actor.org_unit_type + UNION + SELECT distance AS distance FROM permission.grp_ancestors_distance((SELECT id FROM permission.grp_tree WHERE parent IS NULL)) + ) + SELECT INTO denominator MAX(distance) + 1 FROM all_distance; + + -- To ATTEMPT to make this work like it used to, make it reverse the user/requestor profile ids. + -- This may be better implemented as part of the upgrade script? + -- Set usr_grp = requestor_grp, requestor_grp = 1 or something when this flag is already set + -- Then remove this flag, of course. + PERFORM * FROM config.internal_flag WHERE name = 'circ.holds.usr_not_requestor' AND enabled; + + IF FOUND THEN + -- Note: This, to me, is REALLY hacky. I put it in anyway. + -- If you can't tell, this is a single call swap on two variables. + SELECT INTO user_object.profile, requestor_object.profile + requestor_object.profile, user_object.profile; + END IF; + + -- Select the winning matchpoint into the matchpoint variable for returning + SELECT INTO matchpoint m.* + FROM config.hold_matrix_matchpoint m + /*LEFT*/ JOIN permission.grp_ancestors_distance( requestor_object.profile ) rpgad ON m.requestor_grp = rpgad.id + LEFT JOIN permission.grp_ancestors_distance( user_object.profile ) upgad ON m.usr_grp = upgad.id + LEFT JOIN actor.org_unit_ancestors_distance( v_pickup_ou ) puoua ON m.pickup_ou = puoua.id + LEFT JOIN actor.org_unit_ancestors_distance( v_request_ou ) rqoua ON m.request_ou = rqoua.id + LEFT JOIN actor.org_unit_ancestors_distance( item_cn_object.owning_lib ) cnoua ON m.item_owning_ou = cnoua.id + LEFT JOIN actor.org_unit_ancestors_distance( item_object.circ_lib ) iooua ON m.item_circ_ou = iooua.id + LEFT JOIN actor.org_unit_ancestors_distance( user_object.home_ou ) uhoua ON m.user_home_ou = uhoua.id + WHERE m.active + -- Permission Groups + -- AND (m.requestor_grp IS NULL OR upgad.id IS NOT NULL) -- Optional Requestor Group? + AND (m.usr_grp IS NULL OR upgad.id IS NOT NULL) + -- Org Units + AND (m.pickup_ou IS NULL OR (puoua.id IS NOT NULL AND (puoua.distance = 0 OR NOT m.strict_ou_match))) + AND (m.request_ou IS NULL OR (rqoua.id IS NOT NULL AND (rqoua.distance = 0 OR NOT m.strict_ou_match))) + AND (m.item_owning_ou IS NULL OR (cnoua.id IS NOT NULL AND (cnoua.distance = 0 OR NOT m.strict_ou_match))) + AND (m.item_circ_ou IS NULL OR (iooua.id IS NOT NULL AND (iooua.distance = 0 OR NOT m.strict_ou_match))) + AND (m.user_home_ou IS NULL OR (uhoua.id IS NOT NULL AND (uhoua.distance = 0 OR NOT m.strict_ou_match))) + -- Static User Checks + AND (m.juvenile_flag IS NULL OR m.juvenile_flag = user_object.juvenile) + -- Static Item Checks + AND (m.circ_modifier IS NULL OR m.circ_modifier = item_object.circ_modifier) + AND (m.marc_type IS NULL OR m.marc_type = COALESCE(item_object.circ_as_type, rec_descriptor.item_type)) + AND (m.marc_form IS NULL OR m.marc_form = rec_descriptor.item_form) + AND (m.marc_bib_level IS NULL OR m.marc_bib_level = rec_descriptor.bib_level) + AND (m.marc_vr_format IS NULL OR m.marc_vr_format = rec_descriptor.vr_format) + AND (m.ref_flag IS NULL OR m.ref_flag = item_object.ref) + AND (m.item_age IS NULL OR (my_item_age IS NOT NULL AND m.item_age > my_item_age)) + ORDER BY + -- Permission Groups + CASE WHEN rpgad.distance IS NOT NULL THEN 2^(2*weights.requestor_grp - (rpgad.distance/denominator)) ELSE 0.0 END + + CASE WHEN upgad.distance IS NOT NULL THEN 2^(2*weights.usr_grp - (upgad.distance/denominator)) ELSE 0.0 END + + -- Org Units + CASE WHEN puoua.distance IS NOT NULL THEN 2^(2*weights.pickup_ou - (puoua.distance/denominator)) ELSE 0.0 END + + CASE WHEN rqoua.distance IS NOT NULL THEN 2^(2*weights.request_ou - (rqoua.distance/denominator)) ELSE 0.0 END + + CASE WHEN cnoua.distance IS NOT NULL THEN 2^(2*weights.item_owning_ou - (cnoua.distance/denominator)) ELSE 0.0 END + + CASE WHEN iooua.distance IS NOT NULL THEN 2^(2*weights.item_circ_ou - (iooua.distance/denominator)) ELSE 0.0 END + + CASE WHEN uhoua.distance IS NOT NULL THEN 2^(2*weights.user_home_ou - (uhoua.distance/denominator)) ELSE 0.0 END + + -- Static User Checks -- Note: 4^x is equiv to 2^(2*x) + CASE WHEN m.juvenile_flag IS NOT NULL THEN 4^weights.juvenile_flag ELSE 0.0 END + + -- Static Item Checks + CASE WHEN m.circ_modifier IS NOT NULL THEN 4^weights.circ_modifier ELSE 0.0 END + + CASE WHEN m.marc_type IS NOT NULL THEN 4^weights.marc_type ELSE 0.0 END + + CASE WHEN m.marc_form IS NOT NULL THEN 4^weights.marc_form ELSE 0.0 END + + CASE WHEN m.marc_vr_format IS NOT NULL THEN 4^weights.marc_vr_format ELSE 0.0 END + + CASE WHEN m.ref_flag IS NOT NULL THEN 4^weights.ref_flag ELSE 0.0 END + + -- Item age has a slight adjustment to weight based on value. + -- This should ensure that a shorter age limit comes first when all else is equal. + -- NOTE: This assumes that intervals will normally be in days. + CASE WHEN m.item_age IS NOT NULL THEN 4^weights.item_age - 86400/EXTRACT(EPOCH FROM m.item_age) ELSE 0.0 END DESC, + -- Final sort on id, so that if two rules have the same sorting in the previous sort they have a defined order + -- This prevents "we changed the table order by updating a rule, and we started getting different results" + m.id; + + -- Return just the ID for now + RETURN matchpoint.id; +END; +$func$ LANGUAGE 'plpgsql'; + +CREATE OR REPLACE FUNCTION action.hold_request_permit_test( pickup_ou INT, request_ou INT, match_item BIGINT, match_user INT, match_requestor INT, retargetting BOOL ) RETURNS SETOF action.matrix_test_result AS $func$ +DECLARE + matchpoint_id INT; + user_object actor.usr%ROWTYPE; + age_protect_object config.rule_age_hold_protect%ROWTYPE; + standing_penalty config.standing_penalty%ROWTYPE; + transit_range_ou_type actor.org_unit_type%ROWTYPE; + transit_source actor.org_unit%ROWTYPE; + item_object asset.copy%ROWTYPE; + item_cn_object asset.call_number%ROWTYPE; + item_status_object config.copy_status%ROWTYPE; + item_location_object asset.copy_location%ROWTYPE; + ou_skip actor.org_unit_setting%ROWTYPE; + result action.matrix_test_result; + hold_test config.hold_matrix_matchpoint%ROWTYPE; + use_active_date TEXT; + age_protect_date TIMESTAMP WITH TIME ZONE; + hold_count INT; + hold_transit_prox INT; + frozen_hold_count INT; + context_org_list INT[]; + done BOOL := FALSE; + hold_penalty TEXT; + v_pickup_ou ALIAS FOR pickup_ou; + v_request_ou ALIAS FOR request_ou; +BEGIN + SELECT INTO user_object * FROM actor.usr WHERE id = match_user; + SELECT INTO context_org_list ARRAY_AGG(id) FROM actor.org_unit_full_path( v_pickup_ou ); + + result.success := TRUE; + + -- The HOLD penalty block only applies to new holds. + -- The CAPTURE penalty block applies to existing holds. + hold_penalty := 'HOLD'; + IF retargetting THEN + hold_penalty := 'CAPTURE'; + END IF; + + -- Fail if we couldn't find a user + IF user_object.id IS NULL THEN + result.fail_part := 'no_user'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + RETURN; + END IF; + + SELECT INTO item_object * FROM asset.copy WHERE id = match_item; + + -- Fail if we couldn't find a copy + IF item_object.id IS NULL THEN + result.fail_part := 'no_item'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + RETURN; + END IF; + + SELECT INTO matchpoint_id action.find_hold_matrix_matchpoint(v_pickup_ou, v_request_ou, match_item, match_user, match_requestor); + result.matchpoint := matchpoint_id; + + SELECT INTO ou_skip * FROM actor.org_unit_setting WHERE name = 'circ.holds.target_skip_me' AND org_unit = item_object.circ_lib; + + -- Fail if the circ_lib for the item has circ.holds.target_skip_me set to true + IF ou_skip.id IS NOT NULL AND ou_skip.value = 'true' THEN + result.fail_part := 'circ.holds.target_skip_me'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + RETURN; + END IF; + + -- Fail if user is barred + IF user_object.barred IS TRUE THEN + result.fail_part := 'actor.usr.barred'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + RETURN; + END IF; + + SELECT INTO item_cn_object * FROM asset.call_number WHERE id = item_object.call_number; + SELECT INTO item_status_object * FROM config.copy_status WHERE id = item_object.status; + SELECT INTO item_location_object * FROM asset.copy_location WHERE id = item_object.location; + + -- Fail if we couldn't find any matchpoint (requires a default) + IF matchpoint_id IS NULL THEN + result.fail_part := 'no_matchpoint'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + RETURN; + END IF; + + SELECT INTO hold_test * FROM config.hold_matrix_matchpoint WHERE id = matchpoint_id; + + IF hold_test.holdable IS FALSE THEN + result.fail_part := 'config.hold_matrix_test.holdable'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + + IF item_object.holdable IS FALSE THEN + result.fail_part := 'item.holdable'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + + IF item_status_object.holdable IS FALSE THEN + result.fail_part := 'status.holdable'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + + IF item_location_object.holdable IS FALSE THEN + result.fail_part := 'location.holdable'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + + IF hold_test.transit_range IS NOT NULL THEN + SELECT INTO transit_range_ou_type * FROM actor.org_unit_type WHERE id = hold_test.transit_range; + IF hold_test.distance_is_from_owner THEN + SELECT INTO transit_source ou.* FROM actor.org_unit ou JOIN asset.call_number cn ON (cn.owning_lib = ou.id) WHERE cn.id = item_object.call_number; + ELSE + SELECT INTO transit_source * FROM actor.org_unit WHERE id = item_object.circ_lib; + END IF; + + PERFORM * FROM actor.org_unit_descendants( transit_source.id, transit_range_ou_type.depth ) WHERE id = v_pickup_ou; + + IF NOT FOUND THEN + result.fail_part := 'transit_range'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + END IF; + + FOR standing_penalty IN + SELECT DISTINCT csp.* + FROM actor.usr_standing_penalty usp + JOIN config.standing_penalty csp ON (csp.id = usp.standing_penalty) + WHERE usr = match_user + AND usp.org_unit IN ( SELECT * FROM unnest(context_org_list) ) + AND (usp.stop_date IS NULL or usp.stop_date > NOW()) + AND csp.block_list LIKE '%' || hold_penalty || '%' LOOP + + result.fail_part := standing_penalty.name; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END LOOP; + + IF hold_test.stop_blocked_user IS TRUE THEN + FOR standing_penalty IN + SELECT DISTINCT csp.* + FROM actor.usr_standing_penalty usp + JOIN config.standing_penalty csp ON (csp.id = usp.standing_penalty) + WHERE usr = match_user + AND usp.org_unit IN ( SELECT * FROM unnest(context_org_list) ) + AND (usp.stop_date IS NULL or usp.stop_date > NOW()) + AND csp.block_list LIKE '%CIRC%' LOOP + + result.fail_part := standing_penalty.name; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END LOOP; + END IF; + + IF hold_test.max_holds IS NOT NULL AND NOT retargetting THEN + SELECT INTO hold_count COUNT(*) + FROM action.hold_request + WHERE usr = match_user + AND fulfillment_time IS NULL + AND cancel_time IS NULL + AND CASE WHEN hold_test.include_frozen_holds THEN TRUE ELSE frozen IS FALSE END; + + IF hold_count >= hold_test.max_holds THEN + result.fail_part := 'config.hold_matrix_test.max_holds'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + END IF; + + IF item_object.age_protect IS NOT NULL THEN + SELECT INTO age_protect_object * FROM config.rule_age_hold_protect WHERE id = item_object.age_protect; + IF hold_test.distance_is_from_owner THEN + SELECT INTO use_active_date value FROM actor.org_unit_ancestor_setting('circ.holds.age_protect.active_date', item_cn_object.owning_lib); + ELSE + SELECT INTO use_active_date value FROM actor.org_unit_ancestor_setting('circ.holds.age_protect.active_date', item_object.circ_lib); + END IF; + IF use_active_date = 'true' THEN + age_protect_date := COALESCE(item_object.active_date, NOW()); + ELSE + age_protect_date := item_object.create_date; + END IF; + IF age_protect_date + age_protect_object.age > NOW() THEN + IF hold_test.distance_is_from_owner THEN + SELECT INTO item_cn_object * FROM asset.call_number WHERE id = item_object.call_number; + SELECT INTO hold_transit_prox prox FROM actor.org_unit_proximity WHERE from_org = item_cn_object.owning_lib AND to_org = v_pickup_ou; + ELSE + SELECT INTO hold_transit_prox prox FROM actor.org_unit_proximity WHERE from_org = item_object.circ_lib AND to_org = v_pickup_ou; + END IF; + + IF hold_transit_prox > age_protect_object.prox THEN + result.fail_part := 'config.rule_age_hold_protect.prox'; + result.success := FALSE; + done := TRUE; + RETURN NEXT result; + END IF; + END IF; + END IF; + + IF NOT done THEN + RETURN NEXT result; + END IF; + + RETURN; +END; +$func$ LANGUAGE plpgsql; + +COMMIT; + -- 2.43.2