[hooks] Ensure we don't have several children with cardinality != 1
authorSylvain Thénault <sylvain.thenault@logilab.fr>
Fri, 01 Sep 2017 11:19:42 +0200
changeset 2679 39d1d971e058
parent 2671 fcf239b4e613
child 2680 4941eab6a289
[hooks] Ensure we don't have several children with cardinality != 1 By trying to validate several profiles using Jing (in Asalae), it appears that it isn't possible to validate a profile where there are several elements with the same name with optional or/and multiple cardinality, e.g. with a profile telling we expect 1..n Document followed by 1 Document, a transfer with two documents won't be accepted, telling a required document is missing. This is much probably because the validator "eats" the two documents for the first rule, then it misses one to satisfy the second rule. In order to avoid generating such insatisfyable profiles, while not changing much the UI for now, we decided to add hook preventing usage of several sibling children with a cardinality != 1 during edition, and then at RNG profile generation time to put the one with cardinality != 1 as last children. That way profiles should always be validable. This patch introduces the hooks preventing insatisfyable profiles to be created. The next one will ensure the order of RNG export. Two hooks are added to check for unhandled case one for new watched relations and the other for update cardinalities. Added test cases for the 3 most prominent cases: archive unit, document and keywords, with more coverage in the first one and only simple check for the two others. Also, some test have to be updated to follow this change. Notice diff of exported RNG isn't nice to read but is actually only removal of a surrounding "oneOrMore" element. Related to #17098404
cubicweb_seda/hooks.py
cubicweb_seda/i18n/en.po
cubicweb_seda/i18n/fr.po
test/data/seda_02_export.rng
test/data/seda_02_export.xsd
test/data/seda_1_export.rng
test/data/seda_1_export.xsd
test/test_hooks.py
test/test_profile_generation.py
--- a/cubicweb_seda/hooks.py	Fri Sep 01 11:03:28 2017 +0200
+++ b/cubicweb_seda/hooks.py	Fri Sep 01 11:19:42 2017 +0200
@@ -26,7 +26,8 @@
 from cubicweb.server import hook
 
 from .entities import rule_type_from_etype, diag
-from .entities.generated import CHOICE_RTYPE
+from .entities.generated import (CHOICE_RTYPE,
+                                 CHECK_CARD_ETYPES, CHECK_CHILDREN_CARD_RTYPES)
 
 
 SEDA_PARENT_RTYPES = {}
@@ -373,6 +374,78 @@
             CheckProfileSEDACompatiblityOp.get_instance(self._cw).add_entity(self.entity)
 
 
+class AddedCardinalityCheckedRelationHook(hook.Hook):
+    """Some relations whose children should be checked for unhandled cardinality has
+    been added.
+    """
+    __regid__ = 'seda.transfer.checkcard.relations'
+    __select__ = hook.Hook.__select__ & hook.match_rtype_sets(set(CHECK_CHILDREN_CARD_RTYPES))
+    events = ('after_add_relation',)
+    category = 'integrity'
+
+    def __call__(self):
+        CheckChildrenUnhandledCardinalityOp.get_instance(self._cw).add_data(
+            (self.eidto, self.rtype, self.eidfrom))
+
+
+class UpdatedCardinalityCheckedEntityHook(hook.Hook):
+    """Some entity whose cardinality should be checked for unhandled cardinality has
+    been updated.
+    """
+    __regid__ = 'seda.transfer.checkcard.entities'
+    __select__ = hook.Hook.__select__ & is_instance(*CHECK_CARD_ETYPES)
+    events = ('after_update_entity',)
+    category = 'integrity'
+
+    def __call__(self):
+        # nothing to do if cardinality isn't edited or new value is '1'
+        if ('user_cardinality' not in self.entity.cw_edited
+                or self.entity.user_cardinality == '1'):
+            return
+        # back to its parent entity
+        icontained = self.entity.cw_adapt_to('IContained')
+        if icontained is None or not icontained.parent:
+            # entity is being created in the transaction, should be catched by
+            # AddedCardinalityCheckedRelationHook
+            return
+        parent = icontained.parent
+        rtype, role = icontained.parent_relation()
+        CheckChildrenUnhandledCardinalityOp.get_instance(self._cw).add_data(
+            (parent.eid, rtype, self.entity.eid))
+
+
+class CheckChildrenUnhandledCardinalityOp(hook.DataOperationMixIn, hook.Operation):
+    """Check we don't run into the case of an with more than one child through a
+    same relation with `user_cardinality != '1'` (otherwise it won't be
+    validable by RelaxNG validator like Jing).
+
+    Expect (parent_eid, rtype, error_entity_eid) to be added into the data container:
+
+    * eid of the parent whose child should be checked,
+
+    * name of the relation to retrieve its children (expected to be an object
+      relation),
+
+    * eid of the entity on which the `ValidationError` will be raised in case of
+      error.
+    """
+
+    def precommit_event(self):
+        for parent_eid, rtype, added_entity_eid in self.get_data():
+            parent = self.cnx.entity_from_eid(parent_eid)
+            has_non_single_cardinality = False
+            # sort to attempt to return the child with the greatest eid
+            for child in sorted(parent.related(rtype, 'object', entities=True),
+                                key=lambda x: x.eid):
+                if child.user_cardinality != '1':
+                    if has_non_single_cardinality:
+                        msg = _('a sibling entity already has a cardinality non '
+                                'equal to "1" and only one is supported')
+                        raise ValidationError(added_entity_eid,
+                                              {role_name('user_cardinality', 'subject'): msg})
+                    has_non_single_cardinality = True
+
+
 def registration_callback(vreg):
     from cubicweb.server import ON_COMMIT_ADD_RELATIONS
     from cubicweb_seda import seda_profile_container_def, iter_all_rdefs
--- a/cubicweb_seda/i18n/en.po	Fri Sep 01 11:03:28 2017 +0200
+++ b/cubicweb_seda/i18n/en.po	Fri Sep 01 11:19:42 2017 +0200
@@ -2877,6 +2877,11 @@
 msgid "XSD content type"
 msgstr ""
 
+msgid ""
+"a sibling entity already has a cardinality non equal to \"1\" and only one "
+"is supported"
+msgstr ""
+
 msgid "add a SEDAAccessRule"
 msgstr ""
 
--- a/cubicweb_seda/i18n/fr.po	Fri Sep 01 11:03:28 2017 +0200
+++ b/cubicweb_seda/i18n/fr.po	Fri Sep 01 11:19:42 2017 +0200
@@ -2891,6 +2891,13 @@
 msgid "XSD content type"
 msgstr "type de contenu"
 
+msgid ""
+"a sibling entity already has a cardinality non equal to \"1\" and only one "
+"is supported"
+msgstr ""
+"une entité de même type et au même niveau a déja une cardinalité autre que "
+"obligatoire, il ne peut en avoir plusieurs"
+
 msgid "add a SEDAAccessRule"
 msgstr ""
 
@@ -7854,7 +7861,9 @@
 msgstr "sélectionnez un type pour filtrer les vocabulaires correspondants"
 
 msgid "select first a vocabulary then type here to prompt auto-completion"
-msgstr "sélectionnez un vocabulaire avant de taper dans ce champ pour activer la complétion automatique"
+msgstr ""
+"sélectionnez un vocabulaire avant de taper dans ce champ pour activer la "
+"complétion automatique"
 
 msgid "service_level"
 msgstr "valeur"
--- a/test/data/seda_02_export.rng	Fri Sep 01 11:03:28 2017 +0200
+++ b/test/data/seda_02_export.rng	Fri Sep 01 11:19:42 2017 +0200
@@ -468,58 +468,34 @@
               </rng:element>
             </rng:element>
           </rng:oneOrMore>
-          <rng:oneOrMore>
-            <rng:element name="Contains">
-              <rng:element name="DescriptionLevel">
-                <rng:attribute name="listVersionID">
-                  <rng:value type="token">edition 2009</rng:value>
-                </rng:attribute>
-                <rng:value type="string">file</rng:value>
-              </rng:element>
-              <xsd:annotation>
-                <xsd:documentation>archive unit title</xsd:documentation>
-              </xsd:annotation>
+          <rng:element name="Contains">
+            <rng:element name="DescriptionLevel">
+              <rng:attribute name="listVersionID">
+                <rng:value type="token">edition 2009</rng:value>
+              </rng:attribute>
+              <rng:value type="string">file</rng:value>
+            </rng:element>
+            <xsd:annotation>
+              <xsd:documentation>archive unit title</xsd:documentation>
+            </xsd:annotation>
+            <rng:optional>
+              <rng:attribute name="Id">
+                <rng:data type="ID"/>
+              </rng:attribute>
+            </rng:optional>
+            <rng:element name="Name">
               <rng:optional>
-                <rng:attribute name="Id">
-                  <rng:data type="ID"/>
+                <rng:attribute name="languageID">
+                  <rng:data type="language"/>
                 </rng:attribute>
               </rng:optional>
-              <rng:element name="Name">
-                <rng:optional>
-                  <rng:attribute name="languageID">
-                    <rng:data type="language"/>
-                  </rng:attribute>
-                </rng:optional>
-                <rng:data type="string"/>
-              </rng:element>
-              <rng:optional>
-                <rng:element name="Appraisal">
-                  <xsd:annotation>
-                    <xsd:documentation>detruire le document</xsd:documentation>
-                  </xsd:annotation>
-                  <rng:optional>
-                    <rng:attribute name="Id">
-                      <rng:data type="ID"/>
-                    </rng:attribute>
-                  </rng:optional>
-                  <rng:element name="Code">
-                    <rng:attribute name="listVersionID">
-                      <rng:value type="token">edition 2009</rng:value>
-                    </rng:attribute>
-                    <rng:value type="string">detruire</rng:value>
-                  </rng:element>
-                  <rng:element name="Duration">
-                    <xsd:annotation>
-                      <xsd:documentation>C'est dans 10ans je m'en irai</xsd:documentation>
-                    </xsd:annotation>
-                    <rng:value type="string">P10Y</rng:value>
-                  </rng:element>
-                  <rng:element name="StartDate">
-                    <rng:data type="string"/>
-                  </rng:element>
-                </rng:element>
-              </rng:optional>
-              <rng:element name="AccessRestriction">
+              <rng:data type="string"/>
+            </rng:element>
+            <rng:optional>
+              <rng:element name="Appraisal">
+                <xsd:annotation>
+                  <xsd:documentation>detruire le document</xsd:documentation>
+                </xsd:annotation>
                 <rng:optional>
                   <rng:attribute name="Id">
                     <rng:data type="ID"/>
@@ -529,14 +505,36 @@
                   <rng:attribute name="listVersionID">
                     <rng:value type="token">edition 2009</rng:value>
                   </rng:attribute>
-                  <rng:data type="string"/>
+                  <rng:value type="string">detruire</rng:value>
+                </rng:element>
+                <rng:element name="Duration">
+                  <xsd:annotation>
+                    <xsd:documentation>C'est dans 10ans je m'en irai</xsd:documentation>
+                  </xsd:annotation>
+                  <rng:value type="string">P10Y</rng:value>
                 </rng:element>
                 <rng:element name="StartDate">
                   <rng:data type="string"/>
                 </rng:element>
               </rng:element>
+            </rng:optional>
+            <rng:element name="AccessRestriction">
+              <rng:optional>
+                <rng:attribute name="Id">
+                  <rng:data type="ID"/>
+                </rng:attribute>
+              </rng:optional>
+              <rng:element name="Code">
+                <rng:attribute name="listVersionID">
+                  <rng:value type="token">edition 2009</rng:value>
+                </rng:attribute>
+                <rng:data type="string"/>
+              </rng:element>
+              <rng:element name="StartDate">
+                <rng:data type="string"/>
+              </rng:element>
             </rng:element>
-          </rng:oneOrMore>
+          </rng:element>
         </rng:element>
       </rng:oneOrMore>
     </rng:element>
--- a/test/data/seda_02_export.xsd	Fri Sep 01 11:03:28 2017 +0200
+++ b/test/data/seda_02_export.xsd	Fri Sep 01 11:19:42 2017 +0200
@@ -373,7 +373,7 @@
                   <xsd:attribute name="Id" type="xsd:ID" use="optional"/>
                 </xsd:complexType>
               </xsd:element>
-              <xsd:element maxOccurs="unbounded" name="Contains">
+              <xsd:element name="Contains">
                 <xsd:annotation>
                   <xsd:documentation>archive unit title</xsd:documentation>
                 </xsd:annotation>
--- a/test/data/seda_1_export.rng	Fri Sep 01 11:03:28 2017 +0200
+++ b/test/data/seda_1_export.rng	Fri Sep 01 11:19:42 2017 +0200
@@ -439,71 +439,47 @@
               </rng:element>
             </rng:element>
           </rng:oneOrMore>
-          <rng:oneOrMore>
-            <rng:element name="ArchiveObject">
-              <xsd:annotation>
-                <xsd:documentation>archive unit title</xsd:documentation>
-              </xsd:annotation>
+          <rng:element name="ArchiveObject">
+            <xsd:annotation>
+              <xsd:documentation>archive unit title</xsd:documentation>
+            </xsd:annotation>
+            <rng:optional>
+              <rng:attribute name="Id">
+                <rng:data type="ID"/>
+              </rng:attribute>
+            </rng:optional>
+            <rng:element name="Name">
+              <rng:optional>
+                <rng:attribute name="languageID">
+                  <rng:data type="language"/>
+                </rng:attribute>
+              </rng:optional>
+              <rng:data type="string"/>
+            </rng:element>
+            <rng:element name="ContentDescription">
               <rng:optional>
                 <rng:attribute name="Id">
                   <rng:data type="ID"/>
                 </rng:attribute>
               </rng:optional>
-              <rng:element name="Name">
-                <rng:optional>
-                  <rng:attribute name="languageID">
-                    <rng:data type="language"/>
-                  </rng:attribute>
-                </rng:optional>
+              <rng:element name="DescriptionLevel">
+                <rng:attribute name="listVersionID">
+                  <rng:value type="token">edition 2009</rng:value>
+                </rng:attribute>
+                <rng:value type="string">file</rng:value>
+              </rng:element>
+              <rng:element name="Language">
+                <rng:attribute name="listVersionID">
+                  <rng:value type="token">edition 2009</rng:value>
+                </rng:attribute>
                 <rng:data type="string"/>
               </rng:element>
-              <rng:element name="ContentDescription">
-                <rng:optional>
-                  <rng:attribute name="Id">
-                    <rng:data type="ID"/>
-                  </rng:attribute>
-                </rng:optional>
-                <rng:element name="DescriptionLevel">
-                  <rng:attribute name="listVersionID">
-                    <rng:value type="token">edition 2009</rng:value>
-                  </rng:attribute>
-                  <rng:value type="string">file</rng:value>
-                </rng:element>
-                <rng:element name="Language">
-                  <rng:attribute name="listVersionID">
-                    <rng:value type="token">edition 2009</rng:value>
-                  </rng:attribute>
-                  <rng:data type="string"/>
-                </rng:element>
-              </rng:element>
-              <rng:optional>
-                <rng:element name="Appraisal">
-                  <xsd:annotation>
-                    <xsd:documentation>detruire le document</xsd:documentation>
-                  </xsd:annotation>
-                  <rng:optional>
-                    <rng:attribute name="Id">
-                      <rng:data type="ID"/>
-                    </rng:attribute>
-                  </rng:optional>
-                  <rng:element name="Code">
-                    <rng:attribute name="listVersionID">
-                      <rng:value type="token">edition 2009</rng:value>
-                    </rng:attribute>
-                    <rng:value type="string">detruire</rng:value>
-                  </rng:element>
-                  <rng:element name="Duration">
-                    <xsd:annotation>
-                      <xsd:documentation>C'est dans 10ans je m'en irai</xsd:documentation>
-                    </xsd:annotation>
-                    <rng:value type="string">P10Y</rng:value>
-                  </rng:element>
-                  <rng:element name="StartDate">
-                    <rng:data type="string"/>
-                  </rng:element>
-                </rng:element>
-              </rng:optional>
-              <rng:element name="AccessRestrictionRule">
+            </rng:element>
+            <rng:optional>
+              <rng:element name="Appraisal">
+                <xsd:annotation>
+                  <xsd:documentation>detruire le document</xsd:documentation>
+                </xsd:annotation>
                 <rng:optional>
                   <rng:attribute name="Id">
                     <rng:data type="ID"/>
@@ -513,14 +489,36 @@
                   <rng:attribute name="listVersionID">
                     <rng:value type="token">edition 2009</rng:value>
                   </rng:attribute>
-                  <rng:data type="string"/>
+                  <rng:value type="string">detruire</rng:value>
+                </rng:element>
+                <rng:element name="Duration">
+                  <xsd:annotation>
+                    <xsd:documentation>C'est dans 10ans je m'en irai</xsd:documentation>
+                  </xsd:annotation>
+                  <rng:value type="string">P10Y</rng:value>
                 </rng:element>
                 <rng:element name="StartDate">
                   <rng:data type="string"/>
                 </rng:element>
               </rng:element>
+            </rng:optional>
+            <rng:element name="AccessRestrictionRule">
+              <rng:optional>
+                <rng:attribute name="Id">
+                  <rng:data type="ID"/>
+                </rng:attribute>
+              </rng:optional>
+              <rng:element name="Code">
+                <rng:attribute name="listVersionID">
+                  <rng:value type="token">edition 2009</rng:value>
+                </rng:attribute>
+                <rng:data type="string"/>
+              </rng:element>
+              <rng:element name="StartDate">
+                <rng:data type="string"/>
+              </rng:element>
             </rng:element>
-          </rng:oneOrMore>
+          </rng:element>
           <rng:zeroOrMore>
             <rng:element name="Document">
               <xsd:annotation>
--- a/test/data/seda_1_export.xsd	Fri Sep 01 11:03:28 2017 +0200
+++ b/test/data/seda_1_export.xsd	Fri Sep 01 11:19:42 2017 +0200
@@ -333,7 +333,7 @@
                   <xsd:attribute name="Id" type="xsd:ID" use="optional"/>
                 </xsd:complexType>
               </xsd:element>
-              <xsd:element maxOccurs="unbounded" name="ArchiveObject">
+              <xsd:element name="ArchiveObject">
                 <xsd:annotation>
                   <xsd:documentation>archive unit title</xsd:documentation>
                 </xsd:annotation>
--- a/test/test_hooks.py	Fri Sep 01 11:03:28 2017 +0200
+++ b/test/test_hooks.py	Fri Sep 01 11:19:42 2017 +0200
@@ -122,6 +122,46 @@
                         {'rel_eid': rel.eid})
             cnx.commit()
 
+    def test_multiple_child_unhandled_cardinality_archive_unit(self):
+        with self.admin_access.cnx() as cnx:
+            transfer = cnx.create_entity('SEDAArchiveTransfer', title=u'test profile')
+            testutils.create_archive_unit(transfer)
+            cnx.commit()
+            testutils.create_archive_unit(transfer, user_cardinality=u'0..1')
+            cnx.commit()
+            unit2 = testutils.create_archive_unit(transfer, user_cardinality=u'1')[0]
+            cnx.commit()
+            with self.assertValidationError(cnx) as cm:
+                unit2.cw_set(user_cardinality=u'1..n')
+            self.assertEqual(cm.exception.entity, unit2.eid)
+            self.assertIn('user_cardinality-subject', cm.exception.errors)
+            with self.assertValidationError(cnx) as cm:
+                unit3 = testutils.create_archive_unit(transfer, user_cardinality=u'0..1')[0]
+            self.assertEqual(cm.exception.entity, unit3.eid)
+            self.assertIn('user_cardinality-subject', cm.exception.errors)
+
+    def test_multiple_child_unhandled_cardinality_document(self):
+        with self.admin_access.cnx() as cnx:
+            transfer = cnx.create_entity('SEDAArchiveTransfer', title=u'test profile')
+            unit, unit_alt, unit_alt_seq = testutils.create_archive_unit(transfer)
+            testutils.create_data_object(unit_alt_seq, user_cardinality=u'0..n',
+                                         seda_binary_data_object=transfer)
+            cnx.commit()
+            with self.assertValidationError(cnx):
+                testutils.create_data_object(unit_alt_seq, user_cardinality=u'1..n',
+                                             seda_binary_data_object=transfer)
+
+    def test_multiple_child_unhandled_cardinality_keyword(self):
+        with self.admin_access.cnx() as cnx:
+            transfer = cnx.create_entity('SEDAArchiveTransfer', title=u'test profile')
+            unit, unit_alt, unit_alt_seq = testutils.create_archive_unit(transfer)
+            cnx.create_entity('SEDAKeyword', seda_keyword=unit_alt_seq,
+                              user_cardinality=u'1..n')
+            cnx.commit()
+            with self.assertValidationError(cnx):
+                cnx.create_entity('SEDAKeyword', seda_keyword=unit_alt_seq,
+                                  user_cardinality=u'1..n')
+
 
 class SetDefaultHooksTC(CubicWebTC):
 
--- a/test/test_profile_generation.py	Fri Sep 01 11:03:28 2017 +0200
+++ b/test/test_profile_generation.py	Fri Sep 01 11:19:42 2017 +0200
@@ -821,7 +821,7 @@
 
             # Add another sub archive unit
             _, _, subunit2_alt_seq = testutils.create_archive_unit(unit_alt_seq,
-                                                                   user_cardinality=u'1..n')
+                                                                   user_cardinality=u'1')
 
             cnx.commit()