From 8eef21a7bbfdcba709515529a40fadc1f5386b70 Mon Sep 17 00:00:00 2001

From: Ondrej Mular <omular@redhat.com>

Date: Fri, 8 Jul 2016 16:43:16 +0200

Subject: [PATCH 1/2] lib: use recipient id as identifier instead of its value

---

 pcs/common/report_codes.py          |   3 +-

 pcs/lib/cib/alert.py                | 129 +++++++----

 pcs/lib/cib/test/test_alert.py      | 449 +++++++++++++++++++++++++++++++-----

 pcs/lib/commands/alert.py           |  45 +++-

 pcs/lib/commands/test/test_alert.py | 111 ++++++---

 pcs/lib/reports.py                  |  29 ++-

 6 files changed, 597 insertions(+), 169 deletions(-)

diff --git a/pcs/common/report_codes.py b/pcs/common/report_codes.py

index 2b39938..53f2ccb 100644

--- a/pcs/common/report_codes.py

+++ b/pcs/common/report_codes.py

@@ -7,6 +7,7 @@ from __future__ import (

 # force cathegories

 FORCE_ACTIVE_RRP = "ACTIVE_RRP"

+FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE = "FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE"

 FORCE_CONSTRAINT_DUPLICATE = "CONSTRAINT_DUPLICATE"

 FORCE_CONSTRAINT_MULTIINSTANCE_RESOURCE = "CONSTRAINT_MULTIINSTANCE_RESOURCE"

 FORCE_LOAD_THRESHOLD = "LOAD_THRESHOLD"

@@ -22,7 +23,7 @@ AGENT_NOT_FOUND = "AGENT_NOT_FOUND"

 BAD_CLUSTER_STATE_FORMAT = 'BAD_CLUSTER_STATE_FORMAT'

 CIB_ALERT_NOT_FOUND = "CIB_ALERT_NOT_FOUND"

 CIB_ALERT_RECIPIENT_ALREADY_EXISTS = "CIB_ALERT_RECIPIENT_ALREADY_EXISTS"

-CIB_ALERT_RECIPIENT_NOT_FOUND = "CIB_ALERT_RECIPIENT_NOT_FOUND"

+CIB_ALERT_RECIPIENT_VALUE_INVALID = "CIB_ALERT_RECIPIENT_VALUE_INVALID"

 CIB_CANNOT_FIND_MANDATORY_SECTION = "CIB_CANNOT_FIND_MANDATORY_SECTION"

 CIB_LOAD_ERROR_BAD_FORMAT = "CIB_LOAD_ERROR_BAD_FORMAT"

 CIB_LOAD_ERROR = "CIB_LOAD_ERROR"

diff --git a/pcs/lib/cib/alert.py b/pcs/lib/cib/alert.py

index 6b72996..b5fe88c 100644

--- a/pcs/lib/cib/alert.py

+++ b/pcs/lib/cib/alert.py

@@ -7,14 +7,16 @@ from __future__ import (

 from lxml import etree

+from pcs.common import report_codes

 from pcs.lib import reports

-from pcs.lib.errors import LibraryError

+from pcs.lib.errors import LibraryError, ReportItemSeverity as Severities

 from pcs.lib.cib.nvpair import update_nvset, get_nvset

 from pcs.lib.cib.tools import (

     check_new_id_applicable,

     get_sub_element,

     find_unique_id,

     get_alerts,

+    validate_id_does_not_exist,

 )

@@ -61,7 +63,7 @@ def _update_optional_attribute(element, attribute, value):

 def get_alert_by_id(tree, alert_id):

     """

     Returns alert element with specified id.

-    Raises AlertNotFound if alert with specified id doesn't exist.

+    Raises LibraryError if alert with specified id doesn't exist.

     tree -- cib etree node

     alert_id -- id of alert

@@ -72,25 +74,53 @@ def get_alert_by_id(tree, alert_id):

     return alert

-def get_recipient(alert, recipient_value):

+def get_recipient_by_id(tree, recipient_id):

     """

     Returns recipient element with value recipient_value which belong to

     specified alert.

-    Raises RecipientNotFound if recipient doesn't exist.

+    Raises LibraryError if recipient doesn't exist.

-    alert -- parent element of required recipient

-    recipient_value -- value of recipient

+    tree -- cib etree node

+    recipient_id -- id of recipient

     """

-    recipient = alert.find(

-        "./recipient[@value='{0}']".format(recipient_value)

+    recipient = get_alerts(tree).find(

+        "./alert/recipient[@id='{0}']".format(recipient_id)

     )

     if recipient is None:

-        raise LibraryError(reports.cib_alert_recipient_not_found(

-            alert.get("id"), recipient_value

-        ))

+        raise LibraryError(reports.id_not_found(recipient_id, "Recipient"))

     return recipient

+def ensure_recipient_value_is_unique(

+    reporter, alert, recipient_value, recipient_id="", allow_duplicity=False

+):

+    """

+    Ensures that recipient_value is unique in alert.

+

+    reporter -- report processor

+    alert -- alert

+    recipient_value -- recipient value

+    recipient_id -- recipient id of to which value belongs to

+    allow_duplicity -- if True only warning will be shown if value already

+        exists

+    """

+    recipient_list = alert.xpath(

+        "./recipient[@value='{value}' and @id!='{id}']".format(

+            value=recipient_value, id=recipient_id

+        )

+    )

+    if recipient_list:

+        reporter.process(reports.cib_alert_recipient_already_exists(

+            alert.get("id", None),

+            recipient_value,

+            Severities.WARNING if allow_duplicity else Severities.ERROR,

+            forceable=(

+                None if allow_duplicity

+                else report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE

+            )

+        ))

+

+

 def create_alert(tree, alert_id, path, description=""):

     """

     Create new alert element. Returns newly created element.

@@ -116,7 +146,7 @@ def create_alert(tree, alert_id, path, description=""):

 def update_alert(tree, alert_id, path, description=None):

     """

     Update existing alert. Return updated alert element.

-    Raises AlertNotFound if alert with specified id doesn't exist.

+    Raises LibraryError if alert with specified id doesn't exist.

     tree -- cib etree node

     alert_id -- id of alert to be updated

@@ -134,7 +164,7 @@ def update_alert(tree, alert_id, path, description=None):

 def remove_alert(tree, alert_id):

     """

     Remove alert with specified id.

-    Raises AlertNotFound if alert with specified id doesn't exist.

+    Raises LibraryError if alert with specified id doesn't exist.

     tree -- cib etree node

     alert_id -- id of alert which should be removed

@@ -144,36 +174,38 @@ def remove_alert(tree, alert_id):

 def add_recipient(

+    reporter,

     tree,

     alert_id,

     recipient_value,

-    description=""

+    recipient_id=None,

+    description="",

+    allow_same_value=False

 ):

     """

     Add recipient to alert with specified id. Returns added recipient element.

-    Raises AlertNotFound if alert with specified id doesn't exist.

+    Raises LibraryError if alert with specified recipient_id doesn't exist.

     Raises LibraryError if recipient already exists.

+    reporter -- report processor

     tree -- cib etree node

     alert_id -- id of alert which should be parent of new recipient

     recipient_value -- value of recipient

+    recipient_id -- id of new recipient, if None it will be generated

     description -- description of recipient

+    allow_same_value -- if True unique recipient value is not required

     """

-    alert = get_alert_by_id(tree, alert_id)

+    if recipient_id is None:

+        recipient_id = find_unique_id(tree, "{0}-recipient".format(alert_id))

+    else:

+        validate_id_does_not_exist(tree, recipient_id)

-    recipient = alert.find(

-        "./recipient[@value='{0}']".format(recipient_value)

+    alert = get_alert_by_id(tree, alert_id)

+    ensure_recipient_value_is_unique(

+        reporter, alert, recipient_value, allow_duplicity=allow_same_value

     )

-    if recipient is not None:

-        raise LibraryError(reports.cib_alert_recipient_already_exists(

-            alert_id, recipient_value

-        ))

-

     recipient = etree.SubElement(

-        alert,

-        "recipient",

-        id=find_unique_id(tree, "{0}-recipient".format(alert_id)),

-        value=recipient_value

+        alert, "recipient", id=recipient_id, value=recipient_value

     )

     if description:

@@ -182,38 +214,49 @@ def add_recipient(

     return recipient

-def update_recipient(tree, alert_id, recipient_value, description):

+def update_recipient(

+    reporter,

+    tree,

+    recipient_id,

+    recipient_value=None,

+    description=None,

+    allow_same_value=False

+):

     """

     Update specified recipient. Returns updated recipient element.

-    Raises AlertNotFound if alert with specified id doesn't exist.

-    Raises RecipientNotFound if recipient doesn't exist.

+    Raises LibraryError if recipient doesn't exist.

+    reporter -- report processor

     tree -- cib etree node

-    alert_id -- id of alert, parent element of recipient

-    recipient_value -- recipient value

+    recipient_id -- id of recipient to be updated

+    recipient_value -- recipient value, stay unchanged if None

     description -- description, if empty it will be removed, stay unchanged

         if None

+    allow_same_value -- if True unique recipient value is not required

     """

-    recipient = get_recipient(

-        get_alert_by_id(tree, alert_id), recipient_value

-    )

+    recipient = get_recipient_by_id(tree, recipient_id)

+    if recipient_value is not None:

+        ensure_recipient_value_is_unique(

+            reporter,

+            recipient.getparent(),

+            recipient_value,

+            recipient_id=recipient_id,

+            allow_duplicity=allow_same_value

+        )

+        recipient.set("value", recipient_value)

     _update_optional_attribute(recipient, "description", description)

     return recipient

-def remove_recipient(tree, alert_id, recipient_value):

+def remove_recipient(tree, recipient_id):

     """

     Remove specified recipient.

-    Raises AlertNotFound if alert with specified id doesn't exist.

-    Raises RecipientNotFound if recipient doesn't exist.

+    Raises LibraryError if recipient doesn't exist.

     tree -- cib etree node

-    alert_id -- id of alert, parent element of recipient

-    recipient_value -- recipient value

+    recipient_id -- id of recipient to be removed

     """

-    recipient = get_recipient(

-        get_alert_by_id(tree, alert_id), recipient_value

-    )

+    recipient = get_recipient_by_id(tree, recipient_id)

     recipient.getparent().remove(recipient)

diff --git a/pcs/lib/cib/test/test_alert.py b/pcs/lib/cib/test/test_alert.py

index c387aaf..50eaef6 100644

--- a/pcs/lib/cib/test/test_alert.py

+++ b/pcs/lib/cib/test/test_alert.py

@@ -15,8 +15,10 @@ from pcs.lib.errors import ReportItemSeverity as severities

 from pcs.test.tools.assertions import(

     assert_raise_library_error,

     assert_xml_equal,

+    assert_report_item_list_equal,

 )

 from pcs.test.tools.pcs_mock import mock

+from pcs.test.tools.custom_mock import MockLibraryReportProcessor

 @mock.patch("pcs.lib.cib.alert.update_nvset")

@@ -129,54 +131,146 @@ class GetAlertByIdTest(TestCase):

         )

-class GetRecipientTest(TestCase):

+class GetRecipientByIdTest(TestCase):

     def setUp(self):

         self.xml = etree.XML(

             """

-                <alert id="alert-1">

-                    <recipient id="rec-1" value="value1"/>

-                    <recipient id="rec-2" value="value2"/>

-                    <not_recipient value="value3"/>

-                    <recipients>

-                        <recipient id="rec-4" value="value4"/>

-                    </recipients>

-                </alert>

+                <cib>

+                    <configuration>

+                        <alerts>

+                            <alert id="alert-1">

+                                <recipient id="rec-1" value="value1"/>

+                                <not_recipient id="rec-3" value="value3"/>

+                                <recipients>

+                                    <recipient id="rec-4" value="value4"/>

+                                </recipients>

+                            </alert>

+                            <recipient id="rec-2" value="value2"/>

+                        </alerts>

+                        <alert id="alert-2"/>

+                    </configuration>

+                </cib>

             """

         )

     def test_exist(self):

         assert_xml_equal(

-            '<recipient id="rec-2" value="value2"/>',

-            etree.tostring(alert.get_recipient(self.xml, "value2")).decode()

+            '<recipient id="rec-1" value="value1"/>',

+            etree.tostring(

+                alert.get_recipient_by_id(self.xml, "rec-1")

+            ).decode()

         )

     def test_different_place(self):

         assert_raise_library_error(

-            lambda: alert.get_recipient(self.xml, "value4"),

+            lambda: alert.get_recipient_by_id(self.xml, "rec-4"),

             (

                 severities.ERROR,

-                report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND,

+                report_codes.ID_NOT_FOUND,

                 {

-                    "alert": "alert-1",

-                    "recipient": "value4"

+                    "id": "rec-4",

+                    "id_description": "Recipient"

+                }

+            )

+        )

+

+    def test_not_in_alert(self):

+        assert_raise_library_error(

+            lambda: alert.get_recipient_by_id(self.xml, "rec-2"),

+            (

+                severities.ERROR,

+                report_codes.ID_NOT_FOUND,

+                {

+                    "id": "rec-2",

+                    "id_description": "Recipient"

                 }

             )

         )

     def test_not_recipient(self):

         assert_raise_library_error(

-            lambda: alert.get_recipient(self.xml, "value3"),

+            lambda: alert.get_recipient_by_id(self.xml, "rec-3"),

             (

                 severities.ERROR,

-                report_codes.CIB_ALERT_RECIPIENT_NOT_FOUND,

+                report_codes.ID_NOT_FOUND,

                 {

-                    "alert": "alert-1",

-                    "recipient": "value3"

+                    "id": "rec-3",

+                    "id_description": "Recipient"

                 }

             )

         )

+class EnsureRecipientValueIsUniqueTest(TestCase):

+    def setUp(self):

+        self.mock_reporter = MockLibraryReportProcessor()

+        self.alert = etree.Element("alert", id="alert-1")

+        self.recipient = etree.SubElement(

+            self.alert, "recipient", id="rec-1", value="value1"

+        )

+

+    def test_is_unique_no_duplicity_allowed(self):

+        alert.ensure_recipient_value_is_unique(

+            self.mock_reporter, self.alert, "value2"

+        )

+        self.assertEqual(0, len(self.mock_reporter.report_item_list))

+

+    def test_same_recipient_no_duplicity_allowed(self):

+        alert.ensure_recipient_value_is_unique(

+            self.mock_reporter, self.alert, "value1", recipient_id="rec-1"

+        )

+        self.assertEqual(0, len(self.mock_reporter.report_item_list))

+

+    def test_same_recipient_duplicity_allowed(self):

+        alert.ensure_recipient_value_is_unique(

+            self.mock_reporter, self.alert, "value1", recipient_id="rec-1",

+            allow_duplicity=True

+        )

+        self.assertEqual(0, len(self.mock_reporter.report_item_list))

+

+    def test_not_unique_no_duplicity_allowed(self):

+        report_item = (

+            severities.ERROR,

+            report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS,

+            {

+                "alert": "alert-1",

+                "recipient": "value1"

+            },

+            report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE

+        )

+        assert_raise_library_error(

+            lambda: alert.ensure_recipient_value_is_unique(

+                self.mock_reporter, self.alert, "value1"

+            ),

+            report_item

+        )

+        assert_report_item_list_equal(

+            self.mock_reporter.report_item_list, [report_item]

+        )

+

+    def test_is_unique_duplicity_allowed(self):

+        alert.ensure_recipient_value_is_unique(

+            self.mock_reporter, self.alert, "value2", allow_duplicity=True

+        )

+        self.assertEqual(0, len(self.mock_reporter.report_item_list))

+

+    def test_not_unique_duplicity_allowed(self):

+        alert.ensure_recipient_value_is_unique(

+            self.mock_reporter, self.alert, "value1", allow_duplicity=True

+        )

+        assert_report_item_list_equal(

+            self.mock_reporter.report_item_list,

+            [(

+                severities.WARNING,

+                report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS,

+                {

+                    "alert": "alert-1",

+                    "recipient": "value1"

+                }

+            )]

+        )

+

+

 class CreateAlertTest(TestCase):

     def setUp(self):

         self.tree = etree.XML(

@@ -462,6 +556,7 @@ class RemoveAlertTest(TestCase):

 class AddRecipientTest(TestCase):

     def setUp(self):

+        self.mock_reporter = MockLibraryReportProcessor()

         self.tree = etree.XML(

             """

             <cib>

@@ -476,11 +571,40 @@ class AddRecipientTest(TestCase):

             """

         )

-    def test_success(self):

+    def test_with_id(self):

+        assert_xml_equal(

+            '<recipient id="my-recipient" value="value1"/>',

+            etree.tostring(

+                alert.add_recipient(

+                    self.mock_reporter, self.tree, "alert", "value1",

+                    "my-recipient"

+                )

+            ).decode()

+        )

+        assert_xml_equal(

+            """

+            <cib>

+                <configuration>

+                    <alerts>

+                        <alert id="alert" path="/path">

+                            <recipient id="alert-recipient" value="test_val"/>

+                            <recipient id="my-recipient" value="value1"/>

+                        </alert>

+                    </alerts>

+                </configuration>

+            </cib>

+            """,

+            etree.tostring(self.tree).decode()

+        )

+        self.assertEqual([], self.mock_reporter.report_item_list)

+

+    def test_without_id(self):

         assert_xml_equal(

             '<recipient id="alert-recipient-1" value="value1"/>',

             etree.tostring(

-                alert.add_recipient(self.tree, "alert", "value1")

+                alert.add_recipient(

+                    self.mock_reporter, self.tree, "alert", "value1"

+                )

             ).decode()

         )

         assert_xml_equal(

@@ -498,23 +622,85 @@ class AddRecipientTest(TestCase):

             """,

             etree.tostring(self.tree).decode()

         )

+        self.assertEqual([], self.mock_reporter.report_item_list)

-    def test_recipient_exist(self):

+    def test_id_exists(self):

         assert_raise_library_error(

-            lambda: alert.add_recipient(self.tree, "alert", "test_val"),

+            lambda: alert.add_recipient(

+                self.mock_reporter, self.tree, "alert", "value1",

+                "alert-recipient"

+            ),

             (

                 severities.ERROR,

+                report_codes.ID_ALREADY_EXISTS,

+                {"id": "alert-recipient"}

+            )

+        )

+        self.assertEqual([], self.mock_reporter.report_item_list)

+

+    def test_duplicity_of_value_not_allowed(self):

+        report_item = (

+            severities.ERROR,

+            report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS,

+            {

+                "alert": "alert",

+                "recipient": "test_val"

+            },

+            report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE

+        )

+        assert_raise_library_error(

+            lambda: alert.add_recipient(

+                self.mock_reporter, self.tree, "alert", "test_val"

+            ),

+            report_item

+        )

+        assert_report_item_list_equal(

+            self.mock_reporter.report_item_list,

+            [report_item]

+        )

+

+    def test_duplicity_of_value_allowed(self):

+        assert_xml_equal(

+            '<recipient id="alert-recipient-1" value="test_val"/>',

+            etree.tostring(

+                alert.add_recipient(

+                    self.mock_reporter, self.tree, "alert", "test_val",

+                    allow_same_value=True

+                )

+            ).decode()

+        )

+        assert_xml_equal(

+            """

+            <cib>

+                <configuration>

+                    <alerts>

+                        <alert id="alert" path="/path">

+                            <recipient id="alert-recipient" value="test_val"/>

+                            <recipient id="alert-recipient-1" value="test_val"/>

+                        </alert>

+                    </alerts>

+                </configuration>

+            </cib>

+            """,

+            etree.tostring(self.tree).decode()

+        )

+        assert_report_item_list_equal(

+            self.mock_reporter.report_item_list,

+            [(

+                severities.WARNING,

                 report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS,

                 {

-                    "recipient": "test_val",

-                    "alert": "alert"

+                    "alert": "alert",

+                    "recipient": "test_val"

                 }

-            )

+            )]

         )

     def test_alert_not_exist(self):

         assert_raise_library_error(

-            lambda: alert.add_recipient(self.tree, "alert1", "test_val"),

+            lambda: alert.add_recipient(

+                self.mock_reporter, self.tree, "alert1", "test_val"

+            ),

             (

                 severities.ERROR,

                 report_codes.CIB_ALERT_NOT_FOUND,

@@ -532,7 +718,8 @@ class AddRecipientTest(TestCase):

             />

             """,

             etree.tostring(alert.add_recipient(

-                self.tree, "alert", "value1", "desc"

+                self.mock_reporter, self.tree, "alert", "value1",

+                description="desc"

             )).decode()

         )

         assert_xml_equal(

@@ -554,10 +741,12 @@ class AddRecipientTest(TestCase):

             """,

             etree.tostring(self.tree).decode()

         )

+        self.assertEqual([], self.mock_reporter.report_item_list)

 class UpdateRecipientTest(TestCase):

     def setUp(self):

+        self.mock_reporter = MockLibraryReportProcessor()

         self.tree = etree.XML(

             """

             <cib>

@@ -577,6 +766,157 @@ class UpdateRecipientTest(TestCase):

             """

         )

+    def test_update_value(self):

+        assert_xml_equal(

+            """

+            <recipient id="alert-recipient" value="new_val"/>

+            """,

+            etree.tostring(alert.update_recipient(

+                self.mock_reporter, self.tree, "alert-recipient",

+                recipient_value="new_val"

+            )).decode()

+        )

+        assert_xml_equal(

+            """

+            <cib>

+                <configuration>

+                    <alerts>

+                        <alert id="alert" path="/path">

+                            <recipient id="alert-recipient" value="new_val"/>

+                            

+                                id="alert-recipient-1"

+                                value="value1"

+                                description="desc"

+                            />

+                        </alert>

+                    </alerts>

+                </configuration>

+            </cib>

+            """,

+            etree.tostring(self.tree).decode()

+        )

+        self.assertEqual([], self.mock_reporter.report_item_list)

+

+    def test_update_same_value_no_duplicity_allowed(self):

+        assert_xml_equal(

+            '<recipient id="alert-recipient" value="test_val"/>',

+            etree.tostring(alert.update_recipient(

+                self.mock_reporter, self.tree, "alert-recipient",

+                recipient_value="test_val"

+            )).decode()

+        )

+        assert_xml_equal(

+            """

+            <cib>

+                <configuration>

+                    <alerts>

+                        <alert id="alert" path="/path">

+                            <recipient id="alert-recipient" value="test_val"/>

+                            

+                                id="alert-recipient-1"

+                                value="value1"

+                                description="desc"

+                            />

+                        </alert>

+                    </alerts>

+                </configuration>

+            </cib>

+            """,

+            etree.tostring(self.tree).decode()

+        )

+        self.assertEqual([], self.mock_reporter.report_item_list)

+

+    def test_update_same_value_duplicity_allowed(self):

+        assert_xml_equal(

+            '<recipient id="alert-recipient" value="test_val"/>',

+            etree.tostring(alert.update_recipient(

+                self.mock_reporter, self.tree, "alert-recipient",

+                recipient_value="test_val", allow_same_value=True

+            )).decode()

+        )

+        assert_xml_equal(

+            """

+            <cib>

+                <configuration>

+                    <alerts>

+                        <alert id="alert" path="/path">

+                            <recipient id="alert-recipient" value="test_val"/>

+                            

+                                id="alert-recipient-1"

+                                value="value1"

+                                description="desc"

+                            />

+                        </alert>

+                    </alerts>

+                </configuration>

+            </cib>

+            """,

+            etree.tostring(self.tree).decode()

+        )

+        self.assertEqual([], self.mock_reporter.report_item_list)

+

+    def test_duplicity_of_value_not_allowed(self):

+        report_item = (

+            severities.ERROR,

+            report_codes.CIB_ALERT_RECIPIENT_ALREADY_EXISTS,

+            {

+                "alert": "alert",

+                "recipient": "value1"

+            },

+            report_codes.FORCE_ALERT_RECIPIENT_VALUE_NOT_UNIQUE

+        )

+        assert_raise_library_error(

+            lambda: alert.update_recipient(

+                self.mock_reporter, self.tree, "alert-recipient", "value1"