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

shouldn't each <constraintSpec> (or each <constraint>) map onto a *single* pattern? #1625

Closed
bansp opened this issue Mar 31, 2017 · 19 comments
Closed

Comments

@bansp
Copy link
Member

bansp commented Mar 31, 2017

I understand that this can't be an accidental omission but rather a conscious design decision, and I am seeking a rationale for it. I am also not asking this (or "making this", if we consider the present note a feature request) in the Stylesheets section, because it is primarily a modelling issue, and once the issue is solved, implementation might follow.

Schematron organizes its rules into patterns, with an important accompanying principle that once a rule inside a pattern matches, the rules that follow are not processed. This allows to group rules, among others, according to the increasing complexity of tests, but also along a logical cascade ("don't test for error X if error Y points at what should be handled first").

Until yesterday, I naively expected that a constraintSpec maps onto a pattern, and that this is why (modulo various unnecessary shortcuts, pointed out by Syd and Martin in #1524) it makes sense to sometimes create several constraintSpecs, but sometimes it's more sensible to extend the number of rules inside a single constraintSpec. But yesterday, I had to diff two schemas and noticed to my dismay that whatever the containing context, an <sch:rule> in an ODD receives its own containing <sch:pattern>, and odds/extract-isosch.xsl confirms this, at least as far as I can grok it.

So this is a question and/or a request: (a) can we please discuss the rationale for such an implementation and potentially have it stated in the Guidelines OR (b) may I suggest that for sheer transparency/coherence with the overall Schematron 'philosophy' and to ensure flexibility for users, for when they construct their own patterns, we adopt a principle whereby a single constraintSpec (which basically amounts to saying constraintSpec/constraint, given the current content model) always maps onto a single <sch:pattern>?

This way, if someone wants to multiply patterns to ensure that each of them is processed, they will be able to spawn <constraintSpec>s (or <constraint>s) at will, but it they want to have some internal logic to their rules (and/or want to save a bit on the processing, especially when all niche constraints have a chance to apply globally, as per #1417), they get a chance to do some proper organizing.

@sydb
Copy link
Member

sydb commented Nov 18, 2017

Council subgroup thinks this was, in fact, just an oversight. But in any case, we think it would be a dandy good idea for multiple <sch:rule> elements in a single <constraint> to be placed in the same <sch:pattern> in the output of odds/extract-isosch.xsl.
I volunteer to work on this. HOWEVER, note that Schematron is also processed into the RNG file, and there are so many issues to be addressed there, I’m not planning to work on that, at least not now.

@ebeshero
Copy link
Member

F2F (Victoria 2017): Council agrees with the subgroup: a single <constraintSpec> should resolve into a single <sch:pattern>.

@sydb
Copy link
Member

sydb commented Feb 10, 2020

OK. I said I was not going to do this in the RNG output, only the ISO Schematron output. However, on trying to reproduce the problem I found it does not seem to be a problem in the ISO Schematron output at all, only in the RNG output. I decided I would feel guilty closing the ticket without even looking to see if it could be fixed in the RNG reasonably easily.

So I have taken a crack at fixing it (with what I consider disgusting code, because the calling sequence is so incredibly convoluted) in Stylesheets c861337. I have tested my fix on a small customization ODD, and demonstrated to myself that it works in that it does what I expect in that one case (and the TEI constraint "targetorcontent1", which had formally been spit out as 2 <pattern>s). But I have not tested to see that it doesn’t break something else.

Therefore, @bansp, @hcayless. @raffazizzi, @ebeshero, and anyone else interested please test this ASAP! The thing to do is build your local schemas using this set of Stylesheets and test that the RNG file validates your documents as expected (particularly the Schematron bits, of course).

@ebeshero
Copy link
Member

ebeshero commented Feb 11, 2020

@sydb Okay...I'm going to attempt to test this right now as we're prepping for the release just to make sure all is well...

@ebeshero
Copy link
Member

@sydb @martinascholger Okay, I've tested this on an ODD file and can verify that Syd's fix works as advertised (that is, when I have a constraintSpec with one sch:pattern that contains multiple sch:rule elements inside, that I see in the output Relax NG just one sch:pattern and multiple rules inside. Also those rules are working for me. I'm testing on one of my students' projects that had an ODD with a couple of constraintSpecs that met the bill for this ticket. I think we're in the clear on this, so I'll close the ticket.

@ebeshero
Copy link
Member

Wait a second...reopening to make sure I've tested using the bleeding edge of the plugin...

@ebeshero ebeshero reopened this Feb 11, 2020
@ebeshero
Copy link
Member

Okay: @sydb Let me know if this is what we should expect:
I made sure I tested with the bleeding-edge plugin and all was well, so long as I use my own <sch:pattern> inside <constraint> in the ODD. Here's the code I'm using for the test (from a student project to prepare a TEI script of the anime Akira):

  <constraintSpec ident="human-or-super-powers" scheme="schematron">
 
               <constraint>
                  
                <sch:pattern>  
                   <sch:let name="superPowers" value="('#akira','#tetsuo', '#kiyoko', '#masaru', '#takashi', '#kei')"/> 
                 
                   <sch:rule context=" tei:spGrp[@subtype='super']">
                      <sch:let name="superWhos" value="for $i in tokenize(@who, ' ') return $i"/> 
                      <sch:assert test="every $superWho in $superWhos satisfies $superWho = $superPowers">The XML markup should reflect that only Akira, Tetsuo, Kiyoko, Masaru, Takashi, or Kei (or any combination of these characters) exercise superpowers.</sch:assert>                  
                  </sch:rule>
                  <sch:rule context="tei:spGrp[@subtype='human']">
                     <sch:assert test="@who = ('#kaneda','#yamagata', '#doctor', '#colonel', '#colonel #doctor', '#tetsuo #kaneda', '#kiyoko #tetsuo #masaru #takashi', '#nezu', '#ryu', '#army')">The XML markup should reflect that Kaneda, Yamagata, the doctor, the colonel exercise human powers. These combinations of characters together: Tetsuo and Kaneda, as well as Kiyoko, Tetsuo, Masaru, and Takashi also exercise distinctly human powers.</sch:assert>
                  </sch:rule></sch:pattern>
               </constraint>
            </constraintSpec>

This produces the following code in the Relax NG schema output compiled with the bleeding-edge version of Stylesheets: and we see just one <sch:pattern> wrapping around the rules: all good.

 <sch:pattern xmlns="http://www.tei-c.org/ns/1.0"
                xmlns:sch="http://purl.oclc.org/dsdl/schematron"
                xmlns:rng="http://relaxng.org/ns/structure/1.0">  
                   <sch:let name="superPowers"
               value="('#akira','#tetsuo', '#kiyoko', '#masaru', '#takashi', '#kei')"/> 
                 
                   <sch:rule context=" tei:spGrp[@subtype='super']">
                      <sch:let name="superWhos" value="for $i in tokenize(@who, ' ') return $i"/> 
                      <sch:assert test="every $superWho in $superWhos satisfies $superWho = $superPowers">The XML markup should reflect that only Akira, Tetsuo, Kiyoko, Masaru, Takashi, or Kei (or any combination of these characters) exercise superpowers.</sch:assert>                  
                    
                  </sch:rule>
                  <sch:rule context="tei:spGrp[@subtype='human']">
                     <sch:assert test="@who = ('#kaneda','#yamagata', '#doctor', '#colonel', '#colonel #doctor', '#tetsuo #kaneda', '#kiyoko #tetsuo #masaru #takashi', '#nezu', '#ryu', '#army')">The XML markup should reflect that Kaneda, Yamagata, the doctor, the colonel exercise human powers. These combinations of characters together: Tetsuo and Kaneda, as well as Kiyoko, Tetsuo, Masaru, and Takashi also exercise distinctly human powers.</sch:assert>
                  </sch:rule>
   </sch:pattern>

Now, this time I also removed my own <sch:pattern> that wrapped around the pair of <sch:rule> elements, so that they were only surrounded by a <constraint>. The results were not good this time: My <sch:let> variable was not declared and my schema didn't work. It was important, then, for me to use <sch:pattern>. Is this expected behavior?

@ebeshero
Copy link
Member

ebeshero commented Feb 11, 2020

I had one more constraintSpec to test in the Akira ODD that's maybe a little more conventional, as it didn't rely on a <sch:let> definition of a variable. It's defined within an elementSpec for <milestone>. This had nosch:pattern` and just looked like this in the ODD. I think it matches the issue raised in this ticket b/c it has a couple of rules inside:

<constraintSpec ident="powers-and-notes" scheme="schematron">
                   <constraint>
                        <sch:report test="@type='powers' and not(@who)">A who attribute must be present when using type="powers" on the milestone element, in order to identify the source of the power being exerted. 
                      </sch:report>
                         <sch:assert test="following-sibling::*[1] = //tei:note">Every milestone element must be followed by a note element to describe what action is occuring at this point in the script. 
                      </sch:assert> 
                   </constraint>
                </constraintSpec>

And it comes out like this in the bleeding-edge compilation of RNG:

 <pattern xmlns="http://purl.oclc.org/dsdl/schematron"
                  id="akiraODD-milestone-powers-and-notes-constraint-report-9">
            <rule context="tei:milestone">
               <sch:report xmlns="http://www.tei-c.org/ns/1.0"
                           xmlns:sch="http://purl.oclc.org/dsdl/schematron"
                           xmlns:rng="http://relaxng.org/ns/structure/1.0"
                           test="@type='powers' and not(@who)">A who attribute must be present when using type="powers" on the milestone element, in order to identify the source of the power being exerted. 
                      </sch:report>
               <sch:assert xmlns="http://www.tei-c.org/ns/1.0"
                           xmlns:sch="http://purl.oclc.org/dsdl/schematron"
                           xmlns:rng="http://relaxng.org/ns/structure/1.0"
                           test="following-sibling::*[1] = //tei:note">Every milestone element must be followed by a note element to describe what action is occuring at this point in the script. 
                      </sch:assert>
            </rule>
         </pattern>

And yes, both of these rules are working just fine to validate the markup, and respond quite correctly when violated.

@ebeshero
Copy link
Member

@sydb So, tldr: the only issue I see is that if you need to define a variable with <sch:let> in a <constraint>, you need to use <sch:pattern> as a child of <constraint> for the ODD to properly associate it with the rules that use the variable. Is that an issue? Or are we okay? (I apparently already knew to use <sch:pattern> in helping my students with this ODD, so I'm imagining this is not an issue.)

@sydb
Copy link
Member

sydb commented Feb 11, 2020

Well, it might be expected, but it isn’t good or right.

The main test we want is does your ODD file produce proper Schematron in the RELAX NG if you make no changes? Mainly testing for “did something else get messed up by this fix?” E.g., if that <sch:let> was working before and is getting dropped now with no changes to your ODD, then that’s a problem.

The secondary test (less important, because I’m pretty confident it works) is to test that something of the form

  <constraintSpec scheme="schematron" ident="FOO1">
    <constraint>
      <sch:rule context="context1a">
        <sch:assert test="test1a">1A</sch:assert>
      </sch:rule>
      <sch:rule context="context1b">
        <sch:report test="test1b">1B</sch:report>
      </sch:rule>
    </constraint>
  </constraintSpec>
  
  <constraintSpec scheme="schematron" ident="FOO2">
    <constraint>
      <sch:assert test="test2a">2A</sch:assert>
      <sch:assert test="test2b">2B</sch:assert>
    </constraint>
  </constraintSpec>

produces 2 <sch:pattern>s on output, not 3 or 4.

@ebeshero
Copy link
Member

@sydb I can verify that yes, indeed, my ODD is testing those very conditions (two constraints containing two rules each respectively), and I see just two patterns in the RNG as compiled.

In my special case with a <sch:let> that needed to be referenced by two rules, I had a <sch:pattern> already wrapping around it, so I suspect it didn't work before and I figured out how to get it to work. Because we wanted to test your new code here, I decided to remove the <sch:pattern> to see what would happen, and the results weren't right. I'll pull up the RNG code for that to share here in a sec once I re-run it.

@ebeshero
Copy link
Member

ebeshero commented Feb 11, 2020

@sydb Right: here's what happens: ODD code has a <sch:let> defined as a sibling of two rules. There is also another <sch:let> as a child of a <sch:rule>. The output RNG missed the "higher" <sch:let> that's the sibling of the rules, and only processes the <sch:let> child of a rule. Does that make sense?

ODD Source:

 <constraintSpec ident="human-or-super-powers" scheme="schematron">
 
               <constraint>
                  
              
                   <sch:let name="superPowers" value="('#akira','#tetsuo', '#kiyoko', '#masaru', '#takashi', '#kei')"/> 
                 
                   <sch:rule context=" tei:spGrp[@subtype='super']">
                      <sch:let name="superWhos" value="for $i in tokenize(@who, ' ') return $i"/> 
                      <sch:assert test="every $superWho in $superWhos satisfies $superWho = $superPowers">The XML markup should reflect that only Akira, Tetsuo, Kiyoko, Masaru, Takashi, or Kei (or any combination of these characters) exercise superpowers.</sch:assert>                  
                  </sch:rule>
                  <sch:rule context="tei:spGrp[@subtype='human']">
                     <sch:assert test="@who = ('#kaneda','#yamagata', '#doctor', '#colonel', '#colonel #doctor', '#tetsuo #kaneda', '#kiyoko #tetsuo #masaru #takashi', '#nezu', '#ryu', '#army')">The XML markup should reflect that Kaneda, Yamagata, the doctor, the colonel exercise human powers. These combinations of characters together: Tetsuo and Kaneda, as well as Kiyoko, Tetsuo, Masaru, and Takashi also exercise distinctly human powers.</sch:assert>
                  </sch:rule>
               </constraint>
            </constraintSpec>

RNG output loses the "upper" <sch:let> but does properly put the two rules together in one <sch:pattern>:

 <pattern xmlns="http://purl.oclc.org/dsdl/schematron"
            id="akiraODD-human-or-super-powers-constraint-rule-12">
      <sch:rule xmlns="http://www.tei-c.org/ns/1.0"
                xmlns:sch="http://purl.oclc.org/dsdl/schematron"
                xmlns:rng="http://relaxng.org/ns/structure/1.0"
                context=" tei:spGrp[@subtype='super']">
                      <sch:let name="superWhos" value="for $i in tokenize(@who, ' ') return $i"/> 
                      <sch:assert test="every $superWho in $superWhos satisfies $superWho = $superPowers">The XML markup should reflect that only Akira, Tetsuo, Kiyoko, Masaru, Takashi, or Kei (or any combination of these characters) exercise superpowers.</sch:assert>                  
                    
                  </sch:rule>
      <sch:rule xmlns="http://www.tei-c.org/ns/1.0"
                xmlns:sch="http://purl.oclc.org/dsdl/schematron"
                xmlns:rng="http://relaxng.org/ns/structure/1.0"
                context="tei:spGrp[@subtype='human']">
                     <sch:assert test="@who = ('#kaneda','#yamagata', '#doctor', '#colonel', '#colonel #doctor', '#tetsuo #kaneda', '#kiyoko #tetsuo #masaru #takashi', '#nezu', '#ryu', '#army')">The XML markup should reflect that Kaneda, Yamagata, the doctor, the colonel exercise human powers. These combinations of characters together: Tetsuo and Kaneda, as well as Kiyoko, Tetsuo, Masaru, and Takashi also exercise distinctly human powers.</sch:assert>
                  </sch:rule>
   </pattern> 

But everything is properly output when I wrap <sch:pattern> around the "upper" <sch:let> and the two <sch:rule> elements. The output RNG is quite correct and works in that case. I'd be inclined to just add some documentation somewhere about this.

@ebeshero
Copy link
Member

@sydb So what's good is that my student ODD had all of the test cases you envisioned in it, plus one more slightly annoying one... :-)

@sydb
Copy link
Member

sydb commented Feb 11, 2020

I have scanned your recent posts, and if I got it right, yes, your ODD tested what was important, and it seems to be working now. There is the other problem that an <sch:let> that is not inside an <sch:pattern> is sometimes lost, but that is not what I was fixing in this ticket. (It is one of the reasons I tell people not to use the Schematron in the RELAX NG, but rather the Schematron created by extract-isoschematron.xsl, which I think is much better code — of course I’m biased, I wrote most of it.)

@ebeshero
Copy link
Member

So, it seems like we're okay, and we can close this ticket, right?

@ebeshero
Copy link
Member

(We can always open a new ticket for weirdos like me who use <sch:let> in strange ways...) ;-)

@ebeshero
Copy link
Member

(Closing b/c I'm being a release tech now, and we do know that this particular fix is working as @sydb expects and we need to get ready for the release.)

@sydb
Copy link
Member

sydb commented Feb 12, 2020

@ebeshero: I want to take issue with your closing this. Not that you should not have closed it — absolutely no reason you should not have closed it. But it has nothing to do with your being a release tech for the upcoming release. Release techs (IMHO) have no more or less authority nor any more or less incentive to close issues.

@ebeshero
Copy link
Member

Fair enough @sydb . And sorry. I do have problems with justifying closure, but was also concerned to move this along.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants