223097 - (xmlschema) Interface for checking a Node against a XML Schema type

Status: RESOLVED INVALID

(Core Graveyard :: Web Services, enhancement)

Description

[Hixie (not reading bugmail)]

Given:

   a Node
   zero or more URIs to XML Schemas
   a QName (a namespace and name) representing a Schema type

...I would like to be able to get to:

   a boolean representing whether the Node matches the given Schema type.

Would it be possible or easy to add this ability to Mozilla? We seem to already 
have the required Schema-handling code, but it doesn't seem to be able to just 
decide if something matches the type or not (it has to "encode" or "decode" it 
whenever it uses a Schema).

I don't mind if the script has to manually fetch the URIs and get nsISchemas out 
of them, or even if it then has to manually query each of the nsISchemas in turn 
to work out which one defines the specified Schema type.

Comment 1

[Peter Van der Beken [:peterv]]

It is not so simple, I've been working on this for a while now. You need to
validate the node to be able to know it's type and validation is hard.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

what i've been working on will not be able to do this. It can only validate
whole documents to see if they validate against a schema.

And afaik the schemacode we have in mozilla right now isn't very complete, so
i'm guessing it would be a lot of work to do this.

Comment 3

[Brendan Eich [:brendan]]

This bug needs a new owner.  Sicking, Peter?

/be

Comment 4

[Peter Van der Beken [:peterv]]

I'll take this for now. No target milestone though.

Comment 5

[Doron Rosenberg (IBM)]

*** Bug 261268 has been marked as a duplicate of this bug. ***

Comment 6

[Doron Rosenberg (IBM)]

The current work being done for Schema Validation is for XForms.  This means
simpletype validation against a DOMNode, and only support simpletype
restrictions (xsd:restriction) and several of the built-in types.

I have the infrastrucute running, and can validate xsd:integer right now with
all the possible restrictions (other than regexps, since there is no c++
interface atm).  Probably will write a JS xpcom compontent for regexps, as
trying to get a c++ impl to hook into the jseng is beyond me.

The class (SchemaValidator) is exposed to non-chrome JS, so hixie will be happy.

Comment 7

[Doron Rosenberg (IBM)]

Are there any feature requirements for this to initially land?  I currently have
simple types (restrictions and builtin) working for the xforms minimal schema
requirements.  I'd like to start landing it so that we can integrate it into xforms.

Questions are: should this be built by default or ifdef xforms once it goes in?

peterv, as pretty-much-owner-of-xml, any thoughts?

I'm currently cleaning up the code so that I can post something that people can
tear apart.

Comment 8

[Doron Rosenberg (IBM)]

Attachment: work-in-progress

Attaching work in progress.

Comment 9

[Doron Rosenberg (IBM)]

Attachment: Cleaned up code.

Cleaned up version of what I currently have.  Feel free to comment and diss it
to your hearts content - this is my first big c++ xpcom component and I am sure
I am making mistakes all around it.

Supported schema types:
string
boolean
gday
gmonth
gyear
gyearmonth
gmonthday
date
time
datetime
integer
nonPositiveInteger
negativeInteger
byte
float

Comment 10

[aaronr]

Comment on attachment 165746 [details] [diff] [review]
Cleaned up code.

requesting review for Doron who is out on vacation.  He'll be back in a couple
of days.  Thanks.

Comment 11

[timeless]

Attachment: notes

Comment 12

[Doron Rosenberg (IBM)]

Attachment: With timeless review comments addressed. Also added anyURI support and validation of a string value.

Comment 13

[Doron Rosenberg (IBM)]

Attachment: Minor cleanup and adding some more null checks, also added schema validation test files.

Comment 14

[Doron Rosenberg (IBM)]

peterv: do you have time to look at this?  If people are too busy, this could be
checked in with #ifdef XFORMS so that we can start integrating XForms with
validation.

I also have started working on complex type validation and have xsd:sequence
mostly working.

Comment 15

[Doron Rosenberg (IBM)]

Including my complex type work (xsd:sequence support kinda works), code size
change on a opt trunk linux build of libwebsrvcs.so is ~48k.  This will
obviously grow as more complex type support gets done. Ugh.

Comment 16

[Brendan Eich [:brendan]]

Doron: if only XForms needs this, can we factor it into a separate library that
can be bundled with XForms?  That kind of footprint hit for code not used by any
standard product build is going to get a thumbs-down from drivers, I bet.  My
own thumb is spontaneously turning down right now (as it is over my own
impending E4X changes to SpiderMonkey, currently configured off).

/be

Comment 17

[Doron Rosenberg (IBM)]

brendan: Most likely that will happen.  Keeping the code in extension/xforms
should work fine, though perhaps extensions/schemavalidation would make more
sense going forward?

I'm going to try to see if I can reduce the size.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, a extensions/schemavalidation would be the best solution IMHO. I dislike
schema as much as the next guy, but W3C seems hellbent on using it so I think
that we'll end up having to too.

Comment 19

[Brendan Eich [:brendan]]

Whatever became of the readable jwz rule in favor of longish-hyphenated names
for directories?  Oh well, it's way late to try to promote this:

$ ls ../../extensions/
access-builtin  gnomevfs      manticore      sroaming          webdav
content-packs   help          negotiateauth  tasks             webservices
cookie          inspector     p3p            transformiix      xforms
cview           irc           permissions    tridentprofile    xmlextras
CVS             java          pref           typeaheadfind     xml-rpc
datetime        layout-debug  python         universalchardet  xmlterm
editor          Makefile      spellcheck     venkman
finger          Makefile.in   sql            wallet

xml-rpc kind of sticks out, but that's how it's spelled in the books and specs.

/be

Comment 20

[Doron Rosenberg (IBM)]

To expedite XForms work, I'll be checking this into extensions/schema-validation
tomorrow probably and not built by default.  I'll post the patch with those
changes for a quick build review tomorrow.

Comment 21

[Doron Rosenberg (IBM)]

Attachment: standalone schema validation (extensions/schema-validator) to be checked in, not built by default.

Comment 22

[Doron Rosenberg (IBM)]

Comment on attachment 168325 [details] [diff] [review]
standalone schema validation (extensions/schema-validator) to be checked in, not built by default.

my tree wasn't updated enough and the patch won't apply.  Rebuilding and then
posting a new one.

Comment 23

[Doron Rosenberg (IBM)]

Attachment: standalone schema validation (extensions/schema-validator) to be checked in, not built by default.

Comment 24

[Doron Rosenberg (IBM)]

Comment on attachment 168341 [details] [diff] [review]
standalone schema validation (extensions/schema-validator) to be checked in, not built by default.

darin, can you review the build changes?  Or anyone for that matter?

Comment 25

[Doron Rosenberg (IBM)]

Attachment: add allmakefiles.sh changes

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 168352 [details] [diff] [review]
add allmakefiles.sh changes

r=jst for the build changes.

Comment 27

[Doron Rosenberg (IBM)]

checked in, not built by default.

To build it, you need in your .mozconfig:
ac_add_options --enable-extensions="default,schema-validation"

Comment 28

[Allan Beaufour]

(In reply to comment #27)
> checked in, not built by default.
> 
> To build it, you need in your .mozconfig:
> ac_add_options --enable-extensions="default,schema-validation"

My compile stops here:
nsSchemaValidator.cpp:286: error: `GetBaseType' undeclared (first use this
   function)

Haven't looked at it at all...

Comment 29

[Doron Rosenberg (IBM)]

you never got the email, sorry - you need the patch from
https://bugzilla.mozilla.org/show_bug.cgi?id=263384  to make it build.

Comment 30

[Peter Van der Beken [:peterv]]

Comment on attachment 166886 [details] [diff] [review]
Minor cleanup and adding some more null checks, also added schema validation test files.

I've stopped reviewing for now.
Some general comments:
- You need to use our string APIs more. I've commented in some functions about
it (see IsValidSchemaGDay for example), but it applies in a lot of others as
well.
- You need to be more consistent about formatting (whitespace, position of
braces, lining up of arguments, ...). Right now it's all very messy. Functions
should be like:

ReturnType
FunctionName(Arguments)
{
  ...
}

- Please switch to PR_LOG instead of all the debug-only printf's.

> --- /dev/null	2004-11-23 07:52:31.995525448 -0600
>+++ extensions/webservices/schema/src/nsSchemaValidator.cpp	2004-11-23 16:30:58.267208928 -0600

>+NS_NAMED_LITERAL_STRING(EmptyLiteralString, "");

Emptystring() isn't good enough?

>+nsSchemaValidator::~nsSchemaValidator()
>+{
>+  /* destructor code */

No need for comments like this one.

>+NS_IMETHODIMP nsSchemaValidator::ValidateString(const nsAString & aValue,
>+                                                const nsAString & aType,
>+                                                const nsAString & aNamespace,
>+                                                PRBool *aResult)

>+  // get the nsISchemaCollection from the nsISchema
>+  nsCOMPtr<nsISchemaCollection> schemaCollection;
>+  rv = mSchema->GetCollection(getter_AddRefs(schemaCollection));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!schemaCollection)
>+    return NS_ERROR_UNEXPECTED;

Redundant.

>+
>+  // figure out if its a simple or complex type
>+  nsCOMPtr<nsISchemaType> type;
>+  rv = GetType(aType, aNamespace, getter_AddRefs(type));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!type)
>+    return NS_ERROR_UNEXPECTED;

Redundant.

>+  // get the nsISchemaCollection from the nsISchema
>+  nsCOMPtr<nsISchemaCollection> schemaCollection;
>+  rv = mSchema->GetCollection(getter_AddRefs(schemaCollection));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!schemaCollection) 
>+    return NS_ERROR_UNEXPECTED;
>+
>+  // figure out if its a simple or complex type
>+  nsCOMPtr<nsISchemaType> type;
>+  rv = GetType(elementType, elementNamespace, getter_AddRefs(type));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  if (!type)
>+    return NS_ERROR_UNEXPECTED;
>+
>+  PRUint16 typevalue;
>+  rv = type->GetSchemaType(&typevalue);
>+  NS_ENSURE_SUCCESS(rv, rv);

Can we refactor this (see earlier code).

>+
>+  if (typevalue == nsISchemaType::SCHEMA_TYPE_SIMPLE) {
>+    nsCOMPtr<nsISchemaSimpleType> simpleType = do_QueryInterface(type);
>+
>+    if (simpleType) {

Can this fail?

>+      // get the nodevalue
>+      nsAutoString nodeValue;
>+      nsCOMPtr<nsIDOMNode> childNode;
>+
>+      rv = aElement->GetFirstChild(getter_AddRefs(childNode));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      
>+      if (!childNode)
>+        return NS_ERROR_UNEXPECTED;
>+
>+      childNode->GetNodeValue(nodeValue);

Shouldn't you use the nodevalue of aElement (not just of the first child)?

>+nsresult nsSchemaValidator::GetType(const nsAString & aType,
>+                                    const nsAString & aNamespace,
>+                                    nsISchemaType ** aSchemaType)
>+{
>+  nsresult rv;
>+
>+  // get the nsISchemaCollection from the nsISchema
>+  nsCOMPtr<nsISchemaCollection> schemaCollection;
>+  mSchema->GetCollection(getter_AddRefs(schemaCollection));
>+  if (!schemaCollection) 
>+    return NS_ERROR_NULL_POINTER;
>+
>+  // figure out if its a simple or complex type
>+  rv = schemaCollection->GetType(aType, aNamespace, aSchemaType);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!aSchemaType) {
>+    // check if it is a built-in type
>+    nsCOMPtr<nsISchemaBuiltinType> builtinType;
>+    rv = schemaCollection->GetBaseType(aType, 
>+           NS_LITERAL_STRING(NS_SCHEMA_1999_NAMESPACE), 
>+           getter_AddRefs(builtinType));

I thought nsISchemaCollection::GetType returned base types too?

>+nsresult nsSchemaValidator::ValidateRestrictionSimpletype(const nsAString & aNodeValue,
>+  nsISchemaSimpleType *aSchemaSimpleType, PRBool *aResult)

>+  // handle all defined facets
>+  for (facetCounter = 0; facetCounter < facetCount; facetCounter++){

++facetCounter, space before brace.

>+    rv = restrictionType->GetFacet(facetCounter, getter_AddRefs(facet));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    facet->GetFacetType(&facetType);
>+
>+    switch (facetType) {

>+      case nsISchemaFacet::FACET_TYPE_ENUMERATION: {
>+        // not supported

It should be, no? Make this |XXX to be implemented| if so.

>+  switch(builtinTypeValue) {
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_STRING: {
>+      rv = ValidateBuiltinTypeString(aNodeValue, length, minLength,
>+                                maxLength, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_BOOLEAN: {
>+      rv = ValidateBuiltinTypeBoolean(aNodeValue, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_GDAY: {
>+      rv = ValidateBuiltinTypeGDay(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_GMONTH: {
>+      rv = ValidateBuiltinTypeGMonth(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_GYEAR: {
>+      rv = ValidateBuiltinTypeGYear(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_GYEARMONTH: {
>+      rv = ValidateBuiltinTypeGYearMonth(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_GMONTHDAY: {
>+      rv = ValidateBuiltinTypeGMonthDay(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DATE: {
>+      rv = ValidateBuiltinTypeDate(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_TIME: {
>+      rv = ValidateBuiltinTypeTime(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_DATETIME: {
>+      rv = ValidateBuiltinTypeDateTime(aNodeValue, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_INTEGER: {
>+      rv = ValidateBuiltinTypeInteger(aNodeValue, totalDigits, maxExclusive,
>+             minExclusive, maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    /* http://w3.org/TR/xmlschema-2/#nonPositiveInteger */
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_NONPOSITIVEINTEGER: {
>+      rv = ValidateBuiltinTypeInteger(aNodeValue, totalDigits,
>+             maxExclusive.IsEmpty() ? NS_LITERAL_STRING("1") : maxExclusive, 
>+             minExclusive,
>+             maxInclusive.IsEmpty() ? NS_LITERAL_STRING("0") : maxInclusive, 
>+             minInclusive, &isValid);
>+      break;
>+    }
>+
>+    /* http://www.w3.org/TR/xmlschema-2/#negativeInteger */
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_NEGATIVEINTEGER: {
>+      rv = ValidateBuiltinTypeInteger(aNodeValue, totalDigits, 
>+        maxExclusive.IsEmpty() ? NS_LITERAL_STRING("0") : maxExclusive, minExclusive, 
>+        maxInclusive.IsEmpty() ? NS_LITERAL_STRING("-1") : maxInclusive, minInclusive, 
>+        &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_BYTE: {
>+      rv = ValidateBuiltinTypeByte(aNodeValue, totalDigits, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_FLOAT: {
>+      rv = ValidateBuiltinTypeFloat(aNodeValue, totalDigits, maxExclusive, minExclusive,
>+             maxInclusive, minInclusive, &isValid);
>+      break;
>+    }
>+
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_ANYURI: {
>+      rv = ValidateBuiltinTypeAnyURI(aNodeValue, length, minLength,
>+             maxLength, &isValid);
>+      break;
>+    }
>+
>+    default:
>+      rv = SCHEMA_EXCEPTION(NS_ERROR_ILLEGAL_VALUE, "SCHEMA_TYPE_NOT_KNOWN", 
>+                            "Schema type not supported or invalid.");
>+      break;
>+  }

Maybe change this switch to a table of function pointers?
There's a lot more to be done here (checking all those facets), you should note
that in a comment.

>+nsresult nsSchemaValidator::ValidateBuiltinType(const nsAString & aNodeValue, 
>+  nsISchemaSimpleType *aSchemaSimpleType, PRBool *aResult)

>+  switch(builtinTypeValue) {
>+    case nsISchemaBuiltinType::BUILTIN_TYPE_STRING: {
>+      isValid = PR_TRUE;

We probably need to check that all characters are a Char
(http://www.w3.org/TR/2000/WD-xml-2e-20000814#NT-Char).

>+/* http://www.w3.org/TR/xmlschema-2/#string */
>+nsresult nsSchemaValidator::ValidateBuiltinTypeString(const nsAString & aNodeValue,
>+  PRUint32 aLength, PRUint32 aMinLength, PRUint32 aMaxLength, PRBool *aResult)

We probably need to check that all characters are a Char
(http://www.w3.org/TR/2000/WD-xml-2e-20000814#NT-Char).

>+nsresult nsSchemaValidator::ValidateBuiltinTypeBoolean(const nsAString & aNodeValue,
>+  PRBool *aResult)

>+  // possible values are 0, 1, true, false.  Spec makes no mention if TRUE is
>+  // valid, so performing a case insenstive comparision to be safe

"insensitive", but I don't think a case-insensitive comparison is correct.

>+PRBool nsSchemaValidator::IsValidSchemaGDay(const nsAString & aNodeValue, Schema_GDay *aResult){
>+  //---DD(Z|(+|-)hh:mm)
>+
>+  int strLength = aNodeValue.Length();

PRUint32

>+  //   ---DD               ---DDZ              ---DD(+|-)hh:mm
>+  if ((strLength != 5) && (strLength != 6) && (strLength != 11))
>+    return PR_FALSE;
>+
>+  PRBool isValid = PR_FALSE;
>+  NS_ConvertUTF16toUTF8 str(aNodeValue);
>+
>+  char day[3] = "";
>+  char timezoneHour[3] = "";
>+  char timezoneMinute[3] = "";
>+  int dayInt;
>+
>+  char delimeter[4] = "";
>+  strncpy(delimeter, str.get(), 3);
>+  delimeter[sizeof(delimeter)-1] = '\0';
>+
>+  // check for delimeter
>+  if (strcmp(delimeter, "---") == 0) {
>+    strncpy(day, str.get()+3, 2);
>+    day[sizeof(day)-1] = '\0';
>+
>+    // validate the day
>+    isValid = IsValidSchemaGType(NS_ConvertASCIItoUTF16(day), 1, 30, &dayInt);

31?

>+
>+    // validate the timezone - Z or (+|-)hh:mm
>+    if (isValid && (strLength > 5)) {
>+      if (strLength == 6) {
>+        // timezone is one character, so has to be 'Z'
>+        if (str[5] != 'Z')
>+          isValid = PR_FALSE;
>+      } else {
>+        // ---DD(+|-)hh:mm
>+
>+        if ((str[5] == '+') || (str[5] == '-')) {
>+          char timezone[6] = "";
>+          strncpy(timezone, str.get()+6, 5);
>+          timezone[sizeof(timezone)-1] = '\0';
>+
>+          isValid = nsSchemaValidatorUtils::ParseSchemaTimeZone(timezone,
>+                      timezoneHour, timezoneMinute);
>+        } else {
>+          // timezone does not start with +/-
>+          isValid = PR_FALSE;
>+        }
>+      }
>+    }
>+  }

You can do all that with much less copying:

  if (StringBeginsWith(aNodeValue, NS_LITERAL_STRING("---"))) {
    // validate the day
    isValid = IsValidSchemaGType(Substring(aNodeValue, 3, 2), 1, 30, &dayInt);

    if (isValid && strLength > 5) {
      if (strLength == 6) {
	// timezone is one character, so has to be 'Z'
	isValid = aNodeValue.CharAt(5) == 'Z';
      } else {
	// ---DD(+|-)hh:mm
	PRUnichar sign = aNodeValue.CharAt(5);
	if (sign == '+' || sign == '-') {
	  isValid =
nsSchemaValidatorUtils::ParseSchemaTimeZone(Substring(aNodeValue, 5, 5).get(),
		      timezoneHour, timezoneMinute);
	} else {
	  // timezone does not start with +/-
	  isValid = PR_FALSE;
	}
      }
    }
  }

>+PRBool nsSchemaValidator::IsValidSchemaGType(const nsAString & aNodeValue, 
>+    long aMinValue, long aMaxValue, int *aResult){
>+
>+  PRBool isValid = PR_FALSE;
>+  long intValue;
>+  isValid = IsValidSchemaInteger(aNodeValue, &intValue);
>+
>+  if (isValid && ( (intValue < aMinValue) || (intValue > aMaxValue) ))
>+    isValid = PR_FALSE;
>+
>+  *aResult = intValue;
>+  return isValid;

  long intValue;
  return IsValidSchemaInteger(aNodeValue, &intValue) &&
	 intValue >= aMinValue &&
	 intValue <= aMaxValue;

>+}
>+
>+/* http://www.w3.org/TR/xmlschema-2/#gMonth */
>+nsresult nsSchemaValidator::ValidateBuiltinTypeGMonth(const nsAString & aNodeValue, 
>+  const nsAString & aMaxExclusive, const nsAString & aMinExclusive,
>+  const nsAString & aMaxInclusive, const nsAString & aMinInclusive,
>+  PRBool *aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aResult);
>+  PRBool isValidGMonth;
>+  Schema_GMonth gmonth;
>+  PRBool isValid = IsValidSchemaGMonth(aNodeValue, &gmonth);
>+
>+  if (isValid && !aMaxExclusive.IsEmpty()) {
>+    Schema_GMonth maxExclusive;
>+    isValidGMonth = IsValidSchemaGMonth(aMaxExclusive, &maxExclusive);
>+    
>+    if (isValidGMonth) {
>+      if (gmonth.month >= maxExclusive.month) {

Combine the if's.
There's no real need for isValidGMonth.

  if (isValid && !aMaxExclusive.IsEmpty()) {
    Schema_GMonth maxExclusive;
    if (IsValidSchemaGMonth(aMaxExclusive, &maxExclusive) &&
	gmonth.month >= maxExclusive.month) {
      isValid = PR_FALSE;
    }
  }

>+        isValid = PR_FALSE;
>+#ifdef DEBUG
>+        printf("\n  Not valid: Value (%d) is too large", gmonth.month);
>+#endif
>+      }
>+    }
>+  }
>+
>+  if (isValid && !aMinExclusive.IsEmpty()) {
>+    Schema_GMonth minExclusive;
>+    isValidGMonth = IsValidSchemaGMonth(aMinExclusive, &minExclusive);
>+    
>+    if (isValidGMonth) {
>+      if (gmonth.month <= minExclusive.month) {
>+        isValid = PR_FALSE;
>+#ifdef DEBUG
>+        printf("\n  Not valid: Value (%d) is too small", gmonth.month);
>+#endif
>+      }
>+    }
>+  }

See above.

>+
>+  if (isValid && !aMaxInclusive.IsEmpty()) {
>+    Schema_GMonth maxInclusive;
>+    isValidGMonth = IsValidSchemaGMonth(aMaxInclusive, &maxInclusive);
>+    
>+    if (isValidGMonth) {
>+      if (gmonth.month > maxInclusive.month) {
>+        isValid = PR_FALSE;
>+#ifdef DEBUG
>+        printf("\n  Not valid: Value (%d) is too large", gmonth.month);
>+#endif
>+      }
>+    }
>+  }

See above.

>+
>+  if (isValid && !aMinInclusive.IsEmpty()) {
>+    Schema_GMonth minInclusive;
>+    isValidGMonth = IsValidSchemaGMonth(aMinInclusive, &minInclusive);
>+    
>+    if (isValidGMonth) {
>+      if (gmonth.month < minInclusive.month) {
>+        isValid = PR_FALSE;
>+#ifdef DEBUG
>+        printf("\n  Not valid: Value (%d) is too small", gmonth.month);
>+#endif
>+      }
>+    }
>+  }

See above.

>+PRBool nsSchemaValidator::IsValidSchemaGMonth(const nsAString & aNodeValue, 
>+  Schema_GMonth *aResult){
>+  // --MM--(Z|(+|-)hh:mm)

Same comment as for GDay, copy less.

>+PRBool nsSchemaValidator::IsValidSchemaGYear(const nsAString & aNodeValue, Schema_GYear *aResult){
>+  // (-)CCYY(Z|(+|-)hh:mm)

Same comment as for GDay, copy less.

>+
>+  long yearNum;
>+  PRBool isNegativeYear = (aNodeValue.First() == '-');
>+
>+  // figure out where the year ends, taking negative years
>+  // into account.
>+  int yearend = strcspn(isNegativeYear ? str.get()+1 : str.get(), "Z+-");

This can probably use FindCharInSet

>+/* compares 2 strings that contain integers.
>+   Schema Integers have no limit, thus converting the strings
>+   into numbers won't work.
>+ */
>+int nsSchemaValidator::CompareStrings(const nsAString & aString1, const nsAString & aString2){

Should this be in nsSchemaValidatorUtils?

>+  nsAutoString compareString1(aString1);
>+  nsAutoString compareString2(aString2);
>+
>+  // remove negative sign
>+  if (isNegative1)
>+    compareString1.Cut(0,1);
>+
>+  if (isNegative2)
>+    compareString2.Cut(0,1);
>+
>+  // strip leading 0s
>+  nsSchemaValidatorUtils::RemoveLeadingZeros(compareString1);
>+  nsSchemaValidatorUtils::RemoveLeadingZeros(compareString2);
>+
>+  // after removing leading 0s, check if they are the same
>+  if (compareString1.Equals(compareString2)) {
>+    return 0;
>+  }
>+
>+  if (compareString1.Length() > compareString2.Length())
>+    rv = 1;
>+  else if (compareString1.Length() < compareString2.Length())
>+    rv = -1;
>+  else
>+    rv = strcmp(NS_ConvertUTF16toUTF8(compareString1).get(), NS_ConvertUTF16toUTF8(compareString2).get());

Instead of copying and removing characters, you should look for the first
character that is not '-' or '0', get a Substring and compare the Substring's.


>+PRBool nsSchemaValidator::IsValidSchemaFloat(const nsAString & aNodeValue, float *aResult){
>+  PRBool isValid = PR_FALSE;
>+  NS_ConvertUTF16toUTF8 temp(aNodeValue);
>+  char * pEnd;
>+  float floatValue = strtod(temp.get(), &pEnd);
>+
>+  if (*pEnd == '\0')
>+    isValid = PR_TRUE;
>+
>+  // convert back to string and compare
>+  char floatStr[50];
>+  PR_snprintf(floatStr, sizeof(floatStr), "%f", floatValue);

Just use PR_sscanf?

>+PRBool nsSchemaValidator::IsValidSchemaAnyURI(const nsAString & aString)
>+{
>+  PRBool isValid = PR_FALSE;
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIIOService> ioService = do_GetIOService();
>+  if (ioService) {
>+    nsCOMPtr<nsIURI> uri;
>+    rv = ioService->NewURI(NS_ConvertUTF16toUTF8(aString), nsnull, nsnull, getter_AddRefs(uri));

Use NS_NewURI

>+void nsSchemaValidator::DumpBaseType(nsISchemaBuiltinType *aBuiltInType)
>+{
>+  nsAutoString typeName;
>+  aBuiltInType->GetName(typeName);
>+  PRUint16 foo;
>+  aBuiltInType->GetBuiltinType(&foo);
>+
>+#ifdef DEBUG
>+  printf("\n  Base Type is %s (%d)", NS_ConvertUTF16toUTF8(typeName).get(),foo);
>+#endif
>+}

This should be completely inside ifdef DEBUG.

>--- /dev/null	2004-11-23 07:52:31.995525448 -0600
>+++ extensions/webservices/schema/src/nsSchemaValidator.h	2004-11-23 15:57:15.936649904 -0600

>+ typedef struct Schema_GDay {

Use consistent names (nsSchemaGDay).

>+   int day;            // day represented (1-31)
>+   PRBool tz_negative; // is timezone negative
>+   int tz_hour;        // timezone - hour (0-23) - null if not specified
>+   int tz_minute;      // timezone - minute (0-59) - null if not specified

PRUint32

Comment 31

[Doron Rosenberg (IBM)]

> - You need to be more consistent about formatting (whitespace, position of
> braces, lining up of arguments, ...). Right now it's all very messy. Functions
> should be like:
> 
> ReturnType
> FunctionName(Arguments)
> {
>   ...
> }

a style question - if I have a method that has a long name, but there is space
for one argument before the 80 char cutoff, but the next argument is long as
well, do I do:

             80 char cutoff
             |
method (arg1,
  longarg2)
{
}

Or should I line all the args on a new line?

> >+  // get the nsISchemaCollection from the nsISchema
> >+  nsCOMPtr<nsISchemaCollection> schemaCollection;
> >+  rv = mSchema->GetCollection(getter_AddRefs(schemaCollection));
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  if (!schemaCollection)
> >+    return NS_ERROR_UNEXPECTED;
> 
> Redundant.
do we really never return NULL if rv is success?


> >+  if (!aSchemaType) {
> >+    // check if it is a built-in type
> >+    nsCOMPtr<nsISchemaBuiltinType> builtinType;
> >+    rv = schemaCollection->GetBaseType(aType, 
> >+           NS_LITERAL_STRING(NS_SCHEMA_1999_NAMESPACE), 
> >+           getter_AddRefs(builtinType));
> 
> I thought nsISchemaCollection::GetType returned base types too?

I think this part is not needed.  This would get hit if I passed in "myNS" and
"integer", and then would return xsd:integer, which is wrong.  If someone wants
to validate against xsd:integer, they should pass in the right namespace.  I
removed this part from cvs so that I don't need to rely on GetBaseType.

Now, perhaps I need to handle this case:

<test xmlns="http://www.mozilla.org/schema/test"
xmlns:xsi="http://www.w3.org/1999/XMLSchema" xsi:type="xsi:integer">

basically check if the type value itself has a namespace.  That is probably what
I should be doing here, right?  nsIDOM3Node::lookupNamespaceURI would do the trick.

> Maybe change this switch to a table of function pointers?
> There's a lot more to be done here (checking all those facets), you should note
> that in a comment.

Facets are checked in the methods (ValidateBuiltinType*) already.

Thanks for the comments, working on them right now.

Comment 32

[Doron Rosenberg (IBM)]

>+      // get the nodevalue
>+      nsAutoString nodeValue;
>>+      nsCOMPtr<nsIDOMNode> childNode;
>>+
>>+      rv = aElement->GetFirstChild(getter_AddRefs(childNode));
>>+      NS_ENSURE_SUCCESS(rv, rv);
>>+      
>>+      if (!childNode)
>>+        return NS_ERROR_UNEXPECTED;
>>+
>>+      childNode->GetNodeValue(nodeValue);
>
>Shouldn't you use the nodevalue of aElement (not just of the first child)?

isn't nodeValue of an domNode always null?  Hmm, wouldn't it be better to QI to
nsIDOM3Node and use ::GetTextContent?

Comment 33

[Peter Van der Beken [:peterv]]

(In reply to comment #32)
> >Shouldn't you use the nodevalue of aElement (not just of the first child)?
> 
> isn't nodeValue of an domNode always null?  Hmm, wouldn't it be better to QI to
> nsIDOM3Node and use ::GetTextContent?

Er, yes, textContent is what I meant.

Comment 34

[Doron Rosenberg (IBM)]

Attachment: Addresses peterv's comments

This addresses the review comments by peterv.  It now uses xpcom strings for
pretty much everything and the validation code has been moved to using string
iterators for performance.  Also moves to using nspr logging.

This also adds xsd:duration parsing.

Comment 35

[Doron Rosenberg (IBM)]

Comment on attachment 171299 [details] [diff] [review]
Addresses peterv's comments

sorry, this patch is missing some parts.

Comment 36

[Doron Rosenberg (IBM)]

Attachment: with peterv's comments as well as making DOM Node validation do the right thing.

Comment 37

[Doron Rosenberg (IBM)]

Attachment: Doesn't require a loaded schema file if the type to validate against is a built-in type.

Doesn't require a loaded schema file if the type to validate against is a
built-in type.

Also expands xsd:duration support.  nsSchemaDuration became a full class rather
than a struct to make code using it cleaner.  Reason is that -1 values on a
duration member (meaning it wasn't set) have to become 0 for duration
validation.  Because we need to keep track of what was set or not (as xforms
types restrict xsd:duration by only allowing some duration facets), we store if
its a negative duration as a memeber of the class.  When accessing a duration
value (say years), teh class code checks if its -1 (and returns 0), else checks
if the duration is negative and returns the negative value that is stored.

Comment 38

[Peter Van der Beken [:peterv]]

Attachment: Some review comments

Comment 39

[Doron Rosenberg (IBM)]

Attachment: With peterv's commentes addressed

Comment 40

[Doron Rosenberg (IBM)]

Comment on attachment 176607 [details] [diff] [review]
With peterv's commentes addressed

new patch coming

Comment 41

[Doron Rosenberg (IBM)]

Attachment: Fixes a few bugs, reduce code duplication.

Comment 42

[Doron Rosenberg (IBM)]

Attachment: more changes

Includes changes needed by xforms and complex type validtion (actual complex
type validation code not included).

Comment 43

[Laurent Jouanneau]

Attachment: a patch that fix some bugs

This patch should be apply after the lastest doron's patch
(https://bugzilla.mozilla.org/attachment.cgi?id=184319)

This patch fix some bugs that i have discovered :

* the declaration of nsISchemaDuration.idl in the public/Makefile.in file was
forgotten in the doron's patch
* gYear : according to the specification 
http://www.w3.org/TR/xmlschema-2/#gYear, 
  - a year number must have four digit : "No left truncation is allowed".
  - timezone is optional
* gMonth : according to the specification
http://www.w3.org/TR/xmlschema-2/#gMonth
  - the format of gMonth is --MM or --MMZ or --MM(+|-)hh:mm and not --MM-- or
--MM--Z or --MM--(+|-)hh:mm 
  - timezone is optional
* time : when timezone was Z, it doesn't validate (because it added an other Z)

* Decimal : if we call IsValidSchemaDecimal with 000123.4560000 the returned
fraction part should be 456, and not 456000 (to make equality test easier).
Same issue with the whole part.
* nsSchemaValidatorUtils::IsValidSchemaInteger : no verification on a value
overflow
* re-introduce the old nsSchemaValidatorUtils::RemoveLeadingZeros method for
the IsValidSchemaDecimal method (and it check now if there is a sign at the
begining or not)
* add nsSchemaValidatorUtils::RemoveTrailingZeros method for the
IsValidSchemaDecimal method

Comment 44

[Doron Rosenberg (IBM)]

Peterv gave rs= on irc for the patch to go in.

laurent: I'll incorporate your fixes as well, nice catch with gMonth there.

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

I think the AIX bustage fix was wrong... maxExclusive.EqualsLiteral("1"); is a
comparison, not an assignment; I think you want .AssignLiteral("1")

Comment 46

[Doron Rosenberg (IBM)]

(In reply to comment #45)
> I think the AIX bustage fix was wrong... maxExclusive.EqualsLiteral("1"); is a
> comparison, not an assignment; I think you want .AssignLiteral("1")

You are right, I wasn't thinking straight.

Comment 47

[Jonathan Watt [:jwatt]]

I'm getting LONG_MAX and LONG_MIN aren't defined errors in
nsSchemaValidatorUtils.cpp now. I'm building using MSVS .NET 2003.

Comment 48

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #47)
> I'm getting LONG_MAX and LONG_MIN aren't defined errors in
> nsSchemaValidatorUtils.cpp now. I'm building using MSVS .NET 2003.

I'm getting the same errors with MSVC6.

d:/cvs-1.11.5/mozilla/extensions/schema-validation/src/nsSchemaValidatorUtils.cpp(87)
: error C2065: 'LONG_MAX' : undeclared identifier
d:/cvs-1.11.5/mozilla/extensions/schema-validation/src/nsSchemaValidatorUtils.cpp(87)
: error C2065: 'LONG_MIN' : undeclared identifier

Comment 49

[Doron Rosenberg (IBM)]

Can you please file a new bug on that issue?

Comment 50

[Doron Rosenberg (IBM)]

I think the fix would be to #include <limits.h>

Comment 51

[Simon Montagu :smontagu]

(In reply to comment #50)
> I think the fix would be to #include <limits.h>

That fixes the build bustage for me with MSVC 6

Comment 52

[Doron Rosenberg (IBM)]

Bustage patch checked in, thanks for catching that (my windows build was failing
in nss so never got to it).

Comment 53

[Doron Rosenberg (IBM)]

(In reply to comment #43)
> Created an attachment (id=187211) [edit]
> a patch that correct some bugs

I am probably going to remove the LONG checks because Schema's Integer for
example has no bounds, and I have code that compares them as strings because of
that :)

Comment 54

[Doron Rosenberg (IBM)]

Attachment: more simple types

- More simpletypes, all the ones needed by the XForms minimal requirements.
- Fixed laurent's changes when it came to sizes (Schema's limits are not always
the "usual" numerical limits from C.
- Fixed leading 0 removal to not return "" for "000", but rather just "0".
- Mistake in a header function's argument position.

Comment 55

[Doron Rosenberg (IBM)]

Comment on attachment 187954 [details] [diff] [review]
more simple types

went in as part of another patch

Comment 56

[Joao Eiras]

You should use this work and develop it as part of the DOM 3 Validation support. If you develop another interface, then interoperability will be a problem, besides spending unneeded effort to come up with a new valid and flexible interface.