Details

    • Type: New Feature New Feature
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.7, 1.1, 1.1.1, 1.1.2, 1.1.2.1, 1.2
    • Fix Version/s: 1.3 rc1
    • Component/s: XML code generator
    • Labels:
      None
    • Number of attachments :
      10

      Description

      "XML Schema Part 2: Datatypes Second Edition" document (http://www.w3.org/TR/xmlschema-2/) specifies validity constraints on facets. For example: "It is an error for both minInclusive and minExclusive to be specified for the same datatype."
      These constraints need to be checked during schema validation and generation must be aborted when the constraints are violated.

      1. Clean-up.patch
        5 kB
        Sergei Ivanov
      2. patch.c1107.20080302.txt
        103 kB
        Werner Guttmann
      3. patch.c1107.20080311.txt
        104 kB
        Werner Guttmann
      4. patch.c1107.tests.20061108.txt
        7 kB
        Werner Guttmann
      5. patch.txt
        66 kB
        Sergei Ivanov

        Issue Links

          Activity

          Hide
          Sergei Ivanov added a comment -

          I have prepared the first incomplete implementation, which checks validity constraints on minExclusive, minInclusive, maxInclusive, maxExclusive. I had to introduce facet factory and provide specific validation code in the implementation of the specific facet.
          The attached code is an update to 0.9.6 stable codebase.

          Show
          Sergei Ivanov added a comment - I have prepared the first incomplete implementation, which checks validity constraints on minExclusive, minInclusive, maxInclusive, maxExclusive. I had to introduce facet factory and provide specific validation code in the implementation of the specific facet. The attached code is an update to 0.9.6 stable codebase.
          Hide
          Sergei Ivanov added a comment -

          Attached is a collection of test cases for minExclusive facet.

          Show
          Sergei Ivanov added a comment - Attached is a collection of test cases for minExclusive facet.
          Hide
          Sergei Ivanov added a comment -

          The attached implementation also fixes "CASTOR-1106 minExclusive does not override minInclusive of the base simple type" bug. These two issues are linked in terms of implementation (in both cases a factory of facets was required).

          Show
          Sergei Ivanov added a comment - The attached implementation also fixes " CASTOR-1106 minExclusive does not override minInclusive of the base simple type" bug. These two issues are linked in terms of implementation (in both cases a factory of facets was required).
          Hide
          Sergei Ivanov added a comment -

          I am ready to continue the implementation (introduce checks for the remaining facets) if there's any interest among Castor principal developers.

          Show
          Sergei Ivanov added a comment - I am ready to continue the implementation (introduce checks for the remaining facets) if there's any interest among Castor principal developers.
          Hide
          Bruce Snyder added a comment -

          Unifed diffs (diff -u) are always welcome from the community. It's also nice to have JUnit test cases that fit into the CTF-XML to prove patches, otherwise we have to create these test cases ourselves which takes longer.

          Show
          Bruce Snyder added a comment - Unifed diffs (diff -u) are always welcome from the community. It's also nice to have JUnit test cases that fit into the CTF-XML to prove patches, otherwise we have to create these test cases ourselves which takes longer.
          Hide
          Werner Guttmann added a comment -

          Sergei, though the reply is a bit late, I hope you'd still be interested to see this your work integrated into the cpdebase. If that's the case, is there any chances you could provide us with a new patch against the current codebase ?

          Show
          Werner Guttmann added a comment - Sergei, though the reply is a bit late, I hope you'd still be interested to see this your work integrated into the cpdebase. If that's the case, is there any chances you could provide us with a new patch against the current codebase ?
          Hide
          Sergei Ivanov added a comment -

          Werner,
          I am no longer working on the original project (the one where we needed the patch), but I am ready to continue my contribution to Castor's codebase. Though I am a bit time-constrained at the moment, I'll try to rebase the patch as soon as I've got enough spare time.
          Regards,
          --Sergei

          Show
          Sergei Ivanov added a comment - Werner, I am no longer working on the original project (the one where we needed the patch), but I am ready to continue my contribution to Castor's codebase. Though I am a bit time-constrained at the moment, I'll try to rebase the patch as soon as I've got enough spare time. Regards, --Sergei
          Hide
          Werner Guttmann added a comment -

          Thanks, Sergei, in advance. And there's non need to rush .,,,

          Show
          Werner Guttmann added a comment - Thanks, Sergei, in advance. And there's non need to rush .,,,
          Hide
          Sergei Ivanov added a comment -

          Modifications against version 1.0.4

          Show
          Sergei Ivanov added a comment - Modifications against version 1.0.4
          Hide
          Sergei Ivanov added a comment -

          You may use the originally attached mini test suite (facets.zip). The XML source generator must generate valid code from test cases named 'pass*.xsd' and must fail with validation exception on cases named 'fail*.xsd'.

          Show
          Sergei Ivanov added a comment - You may use the originally attached mini test suite (facets.zip). The XML source generator must generate valid code from test cases named 'pass*.xsd' and must fail with validation exception on cases named 'fail*.xsd'.
          Hide
          Werner Guttmann added a comment -

          Sergei, how difficult would it be for you to supply me with a unified patch against SVN trunk ?

          Show
          Werner Guttmann added a comment - Sergei, how difficult would it be for you to supply me with a unified patch against SVN trunk ?
          Hide
          Sergei Ivanov added a comment -

          Hmmm... Never used SVN before. Probably it's time to try it out.
          I'll try to do that, but it may take time.

          Show
          Sergei Ivanov added a comment - Hmmm... Never used SVN before. Probably it's time to try it out. I'll try to do that, but it may take time.
          Hide
          Werner Guttmann added a comment -

          Simply install the svn client binaries on your computer, and in the Castor root directory (or even better in src), type

          > svn diff > some/path/patch.file

          Please note that for any new files you add as part of your patch, you should run >svn add file first, otherwise it won't be included in the patch.

          I hope this makes life easier for you ....

          Show
          Werner Guttmann added a comment - Simply install the svn client binaries on your computer, and in the Castor root directory (or even better in src), type > svn diff > some/path/patch.file Please note that for any new files you add as part of your patch, you should run >svn add file first, otherwise it won't be included in the patch. I hope this makes life easier for you ....
          Hide
          Sergei Ivanov added a comment -

          All right. It turned out to be much simpler than I expected.
          Please find attached a patch file aginst head revision (6384).

          Show
          Sergei Ivanov added a comment - All right. It turned out to be much simpler than I expected. Please find attached a patch file aginst head revision (6384).
          Hide
          Edward Kuns added a comment -

          I haven't tried the patch, but from eyeballing it it certainly looks like A Good Thing. I like the idea of classes to represent facets, which means that the simpletypes don't have to know as much about the facets and redundant logic can be moved to a single location.

          Show
          Edward Kuns added a comment - I haven't tried the patch, but from eyeballing it it certainly looks like A Good Thing. I like the idea of classes to represent facets, which means that the simpletypes don't have to know as much about the facets and redundant logic can be moved to a single location.
          Hide
          Sergei Ivanov added a comment -

          Thank you for your appreciation. I did not went that far to create classes for all facet types (mainly because I was not sure if the patch would be accepted and it took over a year indeed), but that was the intention. If there is interest, I'll dedicate some of my time to provide a complete implementation.

          Show
          Sergei Ivanov added a comment - Thank you for your appreciation. I did not went that far to create classes for all facet types (mainly because I was not sure if the patch would be accepted and it took over a year indeed), but that was the intention. If there is interest, I'll dedicate some of my time to provide a complete implementation.
          Hide
          Werner Guttmann added a comment -

          Sergei, the appreciation is really fully warranted. First off, I will try to integrate your patch this week (if time permits). Second, I think switching all factes would make sense indeed. In other words, your contribution would be appreciated.

          PS Castor (XML) is a different project now from what it used to be a few months ago. This should not be an excuse, as it is an open source project where committers tend to come and go. In other words, we'll do out best to integrate your contributions as quick as possible.

          Show
          Werner Guttmann added a comment - Sergei, the appreciation is really fully warranted. First off, I will try to integrate your patch this week (if time permits). Second, I think switching all factes would make sense indeed. In other words, your contribution would be appreciated. PS Castor (XML) is a different project now from what it used to be a few months ago. This should not be an excuse, as it is an open source project where committers tend to come and go. In other words, we'll do out best to integrate your contributions as quick as possible.
          Hide
          Werner Guttmann added a comment -

          Sergei, I just had a look at your patch, and I think it's a very good step forward. As the CTF suite still runs without any problems, I think this can be committed once all your tests have been converted to proper CTF tests.

          Show
          Werner Guttmann added a comment - Sergei, I just had a look at your patch, and I think it's a very good step forward. As the CTF suite still runs without any problems, I think this can be committed once all your tests have been converted to proper CTF tests.
          Hide
          Werner Guttmann added a comment -

          A few of your tests converted to the required format (relative to src/tests/xml/MasterTestSuite/sourcegenerator).

          Show
          Werner Guttmann added a comment - A few of your tests converted to the required format (relative to src/tests/xml/MasterTestSuite/sourcegenerator).
          Hide
          Sergei Ivanov added a comment -

          The provided tests cover perhaps only 10% of cases. The reason behind was the same - I did not want to spend more time and effort until the changes were accepted. Now I'll have a look at the converted tests and I'll try to come up with more test cases.

          Show
          Sergei Ivanov added a comment - The provided tests cover perhaps only 10% of cases. The reason behind was the same - I did not want to spend more time and effort until the changes were accepted. Now I'll have a look at the converted tests and I'll try to come up with more test cases.
          Hide
          Werner Guttmann added a comment -

          Thanks in advance, Sergei. Please have a look at my latest patch, which includes an addition to the Castor test suite, taking some of your original tests, and moving them to the required format. I hope that this is self-explanatory .. . If not, feel free to ask us any questions, and/or join us at the IRC channel.

          Show
          Werner Guttmann added a comment - Thanks in advance, Sergei. Please have a look at my latest patch, which includes an addition to the Castor test suite, taking some of your original tests, and moving them to the required format. I hope that this is self-explanatory .. . If not, feel free to ask us any questions, and/or join us at the IRC channel.
          Hide
          Werner Guttmann added a comment -

          Can I take it that there are no questions with regards to the tests attached by myself .. serving as a kind of recipe ?

          Show
          Werner Guttmann added a comment - Can I take it that there are no questions with regards to the tests attached by myself .. serving as a kind of recipe ?
          Hide
          Edward Kuns added a comment -

          Looks OK to me.

          Show
          Edward Kuns added a comment - Looks OK to me.
          Hide
          Sergei Ivanov added a comment -

          Had a look at them before... everything seems clear and simple.

          Show
          Sergei Ivanov added a comment - Had a look at them before... everything seems clear and simple.
          Hide
          Werner Guttmann added a comment -

          Sergei, just wondering whether you'd need any additional help ?

          Show
          Werner Guttmann added a comment - Sergei, just wondering whether you'd need any additional help ?
          Hide
          Sergei Ivanov added a comment -

          All I need is some spare time, which I haven't had yet
          I shall try to come back to this issue, but cannot guarantee that it is going to be soon. Sorry about that.

          Show
          Sergei Ivanov added a comment - All I need is some spare time, which I haven't had yet I shall try to come back to this issue, but cannot guarantee that it is going to be soon. Sorry about that.
          Hide
          Werner Guttmann added a comment -

          Just wondering how soon 'soon' really is .. ?

          Show
          Werner Guttmann added a comment - Just wondering how soon 'soon' really is .. ?
          Hide
          Sergei Ivanov added a comment -

          Hi Werner,

          'Soon' has finally arrived I've got some spare time now and I would like to clear my backlog. I've just pulled the complete trunk out of svn and I am now going to put the test cases in place and reapply the patch against the current head revision.
          Do I understand it right that the location of the test cases in the trunk should be:
          /trunk/xmlctf/tests/MasterTestSuite/sourcegenerator/Facets/...
          As I understand it, I will need to build the main module + xmlctf module and then use CTFRun script to run the test cases.

          Show
          Sergei Ivanov added a comment - Hi Werner, 'Soon' has finally arrived I've got some spare time now and I would like to clear my backlog. I've just pulled the complete trunk out of svn and I am now going to put the test cases in place and reapply the patch against the current head revision. Do I understand it right that the location of the test cases in the trunk should be: /trunk/xmlctf/tests/MasterTestSuite/sourcegenerator/Facets/... As I understand it, I will need to build the main module + xmlctf module and then use CTFRun script to run the test cases.
          Hide
          Werner Guttmann added a comment -

          Sergei, I am happy soon has finally arrived .. . Anyhow, to answer your questions ....

          a) yes, the xmlctf suite happens to live in the module xmlctf, within which the tests reside in tests/MasterTestSuite.
          b) to run the tests from the CTF suite, simply build the complete project issuing a 'build tests' from the bin directory, and then a 'CTFrun' from the same place

          If you happen to be using Maven, things are even simpler. Let me know whether you need any help.

          Show
          Werner Guttmann added a comment - Sergei, I am happy soon has finally arrived .. . Anyhow, to answer your questions .... a) yes, the xmlctf suite happens to live in the module xmlctf, within which the tests reside in tests/MasterTestSuite. b) to run the tests from the CTF suite, simply build the complete project issuing a 'build tests' from the bin directory, and then a 'CTFrun' from the same place If you happen to be using Maven, things are even simpler. Let me know whether you need any help.
          Hide
          Sergei Ivanov added a comment -

          Cool, got the whole thing up and running.
          Looking at the structure of the master test suite, I think the best place for the new test cases will be xmlctf/tests/MasterTestSuite/sourcegenerator/generationOnly/facets.
          It's a pity that I can't bundle a number of test cases with different schemas into OnlySourceGenerationTest (in a similar way to SchemaTest). That means I'll have to place related test cases into separate directories (one test schema per directory). Nevermind, I am on my way to convert them all and add more, based on the w3c schema specification.

          Show
          Sergei Ivanov added a comment - Cool, got the whole thing up and running. Looking at the structure of the master test suite, I think the best place for the new test cases will be xmlctf/tests/MasterTestSuite/sourcegenerator/generationOnly/facets. It's a pity that I can't bundle a number of test cases with different schemas into OnlySourceGenerationTest (in a similar way to SchemaTest). That means I'll have to place related test cases into separate directories (one test schema per directory). Nevermind, I am on my way to convert them all and add more, based on the w3c schema specification.
          Hide
          Werner Guttmann added a comment - - edited

          > It's a pity that I can't bundle a number of test cases with different schemas into OnlySourceGenerationTest (in a
          > similar way to SchemaTest). That means I'll have to place related test cases into separate
          > directories (one test schema per directory).
          Hmm, it should not be too hard to have this changed in the XML schema for the test descriptor and associated (generated) classes. Let me know whether we should have a look into this .. . Other than that, thanks for your effort.

          Show
          Werner Guttmann added a comment - - edited > It's a pity that I can't bundle a number of test cases with different schemas into OnlySourceGenerationTest (in a > similar way to SchemaTest). That means I'll have to place related test cases into separate > directories (one test schema per directory). Hmm, it should not be too hard to have this changed in the XML schema for the test descriptor and associated (generated) classes. Let me know whether we should have a look into this .. . Other than that, thanks for your effort.
          Hide
          Sergei Ivanov added a comment -

          Hi there,

          Here's a big patch, which contains:
          1. the original fix, rebased against the HEAD of the trunk;
          2. minor enhancements to the existing code (e.g. using declared constants instead of hard-coded strings);
          3. a bunch of test cases to validate schema constraints (they are named to match the corresponding sections in the schema spec).

          Please review and hopefully it gets integrated soon. I plan to add more tests to the suite and introduce validation for the remaining facets.

          NB You will have to insert a proper copyright header in each of the new files.

          Show
          Sergei Ivanov added a comment - Hi there, Here's a big patch, which contains: 1. the original fix, rebased against the HEAD of the trunk; 2. minor enhancements to the existing code (e.g. using declared constants instead of hard-coded strings); 3. a bunch of test cases to validate schema constraints (they are named to match the corresponding sections in the schema spec). Please review and hopefully it gets integrated soon. I plan to add more tests to the suite and introduce validation for the remaining facets. NB You will have to insert a proper copyright header in each of the new files.
          Hide
          Werner Guttmann added a comment -

          Thanks, Sergei. I will definitely be having a look at this in the next few days.

          Show
          Werner Guttmann added a comment - Thanks, Sergei. I will definitely be having a look at this in the next few days.
          Hide
          Werner Guttmann added a comment -

          Sergei, what's the reason behind using the generation only test case in the CMLCTF. Why not use the 'vanilla' source generation test case and include a few unit test cases that drive the generated and compiled classes against sample input documents, correct and incorrect ones ?

          Show
          Werner Guttmann added a comment - Sergei, what's the reason behind using the generation only test case in the CMLCTF. Why not use the 'vanilla' source generation test case and include a few unit test cases that drive the generated and compiled classes against sample input documents, correct and incorrect ones ?
          Hide
          Werner Guttmann added a comment -

          Actually, my last comment is only valid for those test cases where generation does succeed, indeed. For those cases, one could enhance the test suite by checking that valid documents complete successfully, and invalid ones cause the test case to fail. Just my 0.02 cents ....

          Show
          Werner Guttmann added a comment - Actually, my last comment is only valid for those test cases where generation does succeed, indeed. For those cases, one could enhance the test suite by checking that valid documents complete successfully, and invalid ones cause the test case to fail. Just my 0.02 cents ....
          Hide
          Sergei Ivanov added a comment -

          Hi Werner,

          Considering the current issue, we only need to verify the validity of the schema definition itself. If the schema is internally inconsistent, the generation must fail. Here we are not interested in validating XML documents against the schema (especially against an invalid schema).

          There could be another bunch of test cases, which tests XML document validation against various facet combinations, but it's a different story. We also need tests to check that facets are correctly overridden in subtype definitions (this functionality is included in the patch, but there are no test cases to cover it). I shall have a look into it.

          Show
          Sergei Ivanov added a comment - Hi Werner, Considering the current issue, we only need to verify the validity of the schema definition itself. If the schema is internally inconsistent, the generation must fail. Here we are not interested in validating XML documents against the schema (especially against an invalid schema). There could be another bunch of test cases, which tests XML document validation against various facet combinations, but it's a different story. We also need tests to check that facets are correctly overridden in subtype definitions (this functionality is included in the patch, but there are no test cases to cover it). I shall have a look into it.
          Hide
          Werner Guttmann added a comment -

          Sergei, it looks like there's a problem with constraint checking for non-number data types. Can you please run the complete test suite and have a look at the errors show. In particular, the test case in sourcegenerator/facets/DateTimeNoTimeZone has a schema that clearly has a problem with conversion to BigInteger/BigDecimal values where it should not.

          Show
          Werner Guttmann added a comment - Sergei, it looks like there's a problem with constraint checking for non-number data types. Can you please run the complete test suite and have a look at the errors show. In particular, the test case in sourcegenerator/facets/DateTimeNoTimeZone has a schema that clearly has a problem with conversion to BigInteger/BigDecimal values where it should not.
          Hide
          Werner Guttmann added a comment -

          Here's a stack trace as exhibited when trying to generate classes from that XML schema:

          java.lang.NumberFormatException: Illegal embedded minus sign
          	at java.math.BigInteger.<init>(BigInteger.java:287)
          	at java.math.BigInteger.<init>(BigInteger.java:447)
          	at java.math.BigDecimal.<init>(BigDecimal.java:216)
          	at org.exolab.castor.xml.schema.Facet.toBigDecimal(Facet.java:199)
          	at org.exolab.castor.xml.schema.facets.MaxInclusive.checkConstraints(MaxInclusive.java:95)
          	at org.exolab.castor.xml.schema.SimpleType.validate(SimpleType.java:494)
          	at org.exolab.castor.xml.schema.ElementDecl.validate(ElementDecl.java:836)
          	at org.exolab.castor.xml.schema.Schema.validate(Schema.java:2218)
          	at org.exolab.castor.builder.SourceGenerator.generateSource(SourceGenerator.java:646)
          	at xml.c1207.TestSourceGenerator.testGeneration(TestSourceGenerator.java:25)
          	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          	at java.lang.reflect.Method.invoke(Method.java:324)
          	at junit.framework.TestCase.runTest(TestCase.java:164)
          	at junit.framework.TestCase.runBare(TestCase.java:130)
          	at junit.framework.TestResult$1.protect(TestResult.java:106)
          	at junit.framework.TestResult.runProtected(TestResult.java:124)
          	at junit.framework.TestResult.run(TestResult.java:109)
          	at junit.framework.TestCase.run(TestCase.java:120)
          	at junit.framework.TestSuite.runTest(TestSuite.java:230)
          	at junit.framework.TestSuite.run(TestSuite.java:225)
          	at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
          	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
          
          
          Show
          Werner Guttmann added a comment - Here's a stack trace as exhibited when trying to generate classes from that XML schema: java.lang.NumberFormatException: Illegal embedded minus sign at java.math.BigInteger.<init>(BigInteger.java:287) at java.math.BigInteger.<init>(BigInteger.java:447) at java.math.BigDecimal.<init>(BigDecimal.java:216) at org.exolab.castor.xml.schema.Facet.toBigDecimal(Facet.java:199) at org.exolab.castor.xml.schema.facets.MaxInclusive.checkConstraints(MaxInclusive.java:95) at org.exolab.castor.xml.schema.SimpleType.validate(SimpleType.java:494) at org.exolab.castor.xml.schema.ElementDecl.validate(ElementDecl.java:836) at org.exolab.castor.xml.schema.Schema.validate(Schema.java:2218) at org.exolab.castor.builder.SourceGenerator.generateSource(SourceGenerator.java:646) at xml.c1207.TestSourceGenerator.testGeneration(TestSourceGenerator.java:25) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:324) at junit.framework.TestCase.runTest(TestCase.java:164) at junit.framework.TestCase.runBare(TestCase.java:130) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
          Hide
          Werner Guttmann added a comment -

          Personally, I think that storing the base type (name) of the 'enclosing' simple type|content definition with the Facet instance should provide you with enough information to distinguish between numeric and non-numeric types.

          Show
          Werner Guttmann added a comment - Personally, I think that storing the base type (name) of the 'enclosing' simple type|content definition with the Facet instance should provide you with enough information to distinguish between numeric and non-numeric types.
          Hide
          Sergei Ivanov added a comment -

          Hi Werner,

          Honestly speaking, I had a suspicion that date/time based types may become a problem. It looks like there is a serious regression indeed.
          I looked at the Facet implementation, and there was no hint about the base type of the element, for which the facet is specified. If we had this information, we should be able to use parse() methods of GDay, Duration and alike. Then we should be able to use DateTimeBase.compareTo() or Duration.isGreater() to compare values (similarly to e.g. DateTimeValidator).

          So, you are right again, we need some information about the enclosing base type in the Facet. Is it possible to add it there?

          Show
          Sergei Ivanov added a comment - Hi Werner, Honestly speaking, I had a suspicion that date/time based types may become a problem. It looks like there is a serious regression indeed. I looked at the Facet implementation, and there was no hint about the base type of the element, for which the facet is specified. If we had this information, we should be able to use parse() methods of GDay, Duration and alike. Then we should be able to use DateTimeBase.compareTo() or Duration.isGreater() to compare values (similarly to e.g. DateTimeValidator). So, you are right again, we need some information about the enclosing base type in the Facet. Is it possible to add it there?
          Hide
          Werner Guttmann added a comment -

          > we need some information about the enclosing base type in the Facet. Is it possible to add it there?
          Sure. I had a look at some code, and it is easy to add a setBaseType(Name) method to Facet, and change Simple[Type|Content]RestrictionUnmarshaller code to set the base type (name) on the Facet before adding it to the SimpleTypeDefinition/SimpleContentDedinition instances. Does this make sense ?

          Show
          Werner Guttmann added a comment - > we need some information about the enclosing base type in the Facet. Is it possible to add it there? Sure. I had a look at some code, and it is easy to add a setBaseType(Name) method to Facet, and change Simple [Type|Content] RestrictionUnmarshaller code to set the base type (name) on the Facet before adding it to the SimpleTypeDefinition/SimpleContentDedinition instances. Does this make sense ?
          Hide
          Sergei Ivanov added a comment -

          It probably does, but I shall leave the decision to you, simply because you know the library better. Can you see any potential adverse effects or design flaws here?

          Show
          Sergei Ivanov added a comment - It probably does, but I shall leave the decision to you, simply because you know the library better. Can you see any potential adverse effects or design flaws here?
          Hide
          Werner Guttmann added a comment -

          No, go ahead. I don't think there's side effects. If you need help, simply shout at me .. .

          Show
          Werner Guttmann added a comment - No, go ahead. I don't think there's side effects. If you need help, simply shout at me .. .
          Hide
          Sergei Ivanov added a comment -

          While I am trying to find a better solution to the problem, I propose the following scenario (to avoid wasting the effort completely):
          1. I will create a smaller patch with "clean-up" changes only, which will not affect the functionality. We check this patch in.
          2. I will temporarily back out the functional changes, update the failing test cases with <Skip>true</Skip> and we check them all in.
          3. Then I will restore the functional changes locally and will try to add date/duration handling.
          What do you think?

          Show
          Sergei Ivanov added a comment - While I am trying to find a better solution to the problem, I propose the following scenario (to avoid wasting the effort completely): 1. I will create a smaller patch with "clean-up" changes only, which will not affect the functionality. We check this patch in. 2. I will temporarily back out the functional changes, update the failing test cases with <Skip>true</Skip> and we check them all in. 3. Then I will restore the functional changes locally and will try to add date/duration handling. What do you think?
          Hide
          Sergei Ivanov added a comment -

          A follow-up to the last comment: two patches.
          1. Minor code clean-up (any existing functionality preserved)
          2. Test suite for facets, some tests marked with <Skip> for now, until facet constraint validation is implemented
          All this can be immediately committed and must not cause any regression.

          Show
          Sergei Ivanov added a comment - A follow-up to the last comment: two patches. 1. Minor code clean-up (any existing functionality preserved) 2. Test suite for facets, some tests marked with <Skip> for now, until facet constraint validation is implemented All this can be immediately committed and must not cause any regression.
          Hide
          Werner Guttmann added a comment -

          Just committed the clean-up patch as is.

          Show
          Werner Guttmann added a comment - Just committed the clean-up patch as is.
          Hide
          Werner Guttmann added a comment -

          Sergei, just committed the extension(s) to the XML CTF test suite as well. I think that - after some thinking - I agree with your step-by-step approach. And I just want to add that I really appreciate your efforts.

          Show
          Werner Guttmann added a comment - Sergei, just committed the extension(s) to the XML CTF test suite as well. I think that - after some thinking - I agree with your step-by-step approach. And I just want to add that I really appreciate your efforts.
          Hide
          Werner Guttmann added a comment -

          Sergei, I think I have got a new patch ready where I have added some code to the *Facet classes so that all tests pass, incl. the ones that define restriction of date/time types. I will attach this new patch early next week. I basically added some methods to XMLType so that one can check whether the type is derived from a numeric type or a datetime tyoe, and modified the *Facet.checkConstraints() methods to work with numeric types only (for the time being).

          Show
          Werner Guttmann added a comment - Sergei, I think I have got a new patch ready where I have added some code to the *Facet classes so that all tests pass, incl. the ones that define restriction of date/time types. I will attach this new patch early next week. I basically added some methods to XMLType so that one can check whether the type is derived from a numeric type or a datetime tyoe, and modified the *Facet.checkConstraints() methods to work with numeric types only (for the time being).
          Hide
          Werner Guttmann added a comment -

          Updated patch, as discussed. Please have a look, and don't hesitate to provide me with feedback. Please note that all CTF tests complete just fine.

          Show
          Werner Guttmann added a comment - Updated patch, as discussed. Please have a look, and don't hesitate to provide me with feedback. Please note that all CTF tests complete just fine.
          Hide
          Sergei Ivanov added a comment -

          Werner, I've just had some time to apply the proposed patch locally and review your changes (and run the entire test suite). They look absolutely fine and I would proceed with applying them.
          One suggestion:
          instead of adding two static methods to SimpleTypesFactory, I would make isNumericType() return false by default and override it to return true in classes DecimalType and RealType (same for isDateTimeType() and class DateTimeType). That would be more elegant and OO. Apart from that I am quite happy with the resulting patch and it provides a good starting point for further enhancements (e.g. possible inclusion of the date/time/duration data types in validation checks).

          Show
          Sergei Ivanov added a comment - Werner, I've just had some time to apply the proposed patch locally and review your changes (and run the entire test suite). They look absolutely fine and I would proceed with applying them. One suggestion: instead of adding two static methods to SimpleTypesFactory, I would make isNumericType() return false by default and override it to return true in classes DecimalType and RealType (same for isDateTimeType() and class DateTimeType). That would be more elegant and OO. Apart from that I am quite happy with the resulting patch and it provides a good starting point for further enhancements (e.g. possible inclusion of the date/time/duration data types in validation checks).
          Hide
          Werner Guttmann added a comment -

          > Instead of adding two static methods to SimpleTypesFactory, I would make isNumericType() return false by default ...

          Yes, you are right, and I shall introduce that change.

          Show
          Werner Guttmann added a comment - > Instead of adding two static methods to SimpleTypesFactory, I would make isNumericType() return false by default ... Yes, you are right, and I shall introduce that change.
          Hide
          Werner Guttmann added a comment -

          Looking at this again, how about the logic that exists within SimpleType.isNumericType() that deals with base types ?

              /**
               * Indicates whether this {@link SimpleType} is a numeric type.
               * @return True if this SimpleType is a numeric type
              **/
              public boolean isNumericType() {
                  SimpleType toStart = this;
                  while (!toStart.isBuiltInType()) {
                      toStart = (SimpleType) toStart.getBaseType();
                  }
                  return SimpleTypesFactory.isNumericType(toStart.getTypeCode());
              }
          

          Where should this go in your opinion ?

          Show
          Werner Guttmann added a comment - Looking at this again, how about the logic that exists within SimpleType.isNumericType() that deals with base types ? /** * Indicates whether this {@link SimpleType} is a numeric type. * @return True if this SimpleType is a numeric type **/ public boolean isNumericType() { SimpleType toStart = this; while (!toStart.isBuiltInType()) { toStart = (SimpleType) toStart.getBaseType(); } return SimpleTypesFactory.isNumericType(toStart.getTypeCode()); } Where should this go in your opinion ?
          Hide
          Sergei Ivanov added a comment -

          How about this:

          /* SimpleType */
              public boolean isNumericType() {
                  if (!isBuiltInType()) {
                      return ((SimpleType) getBaseType()).isNumericType();
                  }
                  return false;
              }
          

          overridden as

              public boolean isNumericType() {
                  return true;
              }
          

          in DecimalType and RealType.

          Show
          Sergei Ivanov added a comment - How about this: /* SimpleType */ public boolean isNumericType() { if (!isBuiltInType()) { return ((SimpleType) getBaseType()).isNumericType(); } return false ; } overridden as public boolean isNumericType() { return true ; } in DecimalType and RealType.
          Hide
          Werner Guttmann added a comment -

          Very reasonable .. and far better than pulling this logic outside of SimpleType, as I was considering. I guess I will commit this patch (post this small change) , and work with change requests from here.

          Show
          Werner Guttmann added a comment - Very reasonable .. and far better than pulling this logic outside of SimpleType, as I was considering. I guess I will commit this patch (post this small change) , and work with change requests from here.
          Hide
          Werner Guttmann added a comment -

          Updated and final patch.

          Show
          Werner Guttmann added a comment - Updated and final patch.
          Hide
          Sergei Ivanov added a comment -

          I am really glad that this change will finally go live (this defect probably has one of the longest lifespans in the project!). This is a good starting point for further enhancements in the area and I hope I'll be able to contribute again. Many thanks, Werner.

          Show
          Sergei Ivanov added a comment - I am really glad that this change will finally go live (this defect probably has one of the longest lifespans in the project!). This is a good starting point for further enhancements in the area and I hope I'll be able to contribute again. Many thanks, Werner.

            People

            • Assignee:
              Werner Guttmann
              Reporter:
              Sergei Ivanov
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: