check constraints on facets

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.

Attached is a collection of test cases for minExclusive facet.

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).

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

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.

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

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

Modifications against version 1.0.4

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'.

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

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

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 ....

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

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.

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.

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.

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.

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

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.

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.

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

Looks OK to me.

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

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

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.

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

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.

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.

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.

> 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.

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.

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

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 ?

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 ....

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.

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.

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)

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.

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?

> 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 ?

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?

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

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?

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.

Just committed the clean-up patch as is.

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.

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).

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.

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).

> 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.

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 ?

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.

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.

Updated and final patch.

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.