Closed Bug 223097 (xmlschema) Opened 17 years ago Closed 3 years ago

Interface for checking a Node against a XML Schema type

Categories

(Core Graveyard :: Web Services, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: ian, Assigned: doronr)

References

Details

Attachments

(4 files, 13 obsolete files)

18.50 KB, text/plain
Details
158.35 KB, patch
Details | Diff | Splinter Review
41.69 KB, text/plain
Details
8.37 KB, patch
Details | Diff | Splinter Review
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.
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.
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.
Blocks: xforms
This bug needs a new owner.  Sicking, Peter?

/be
I'll take this for now. No target milestone though.
Assignee: harishd → peterv
Summary: RFE: Interface for checking a Node against a Schema type → Interface for checking a Node against a Schema type
Assignee: peterv → doronr
*** Bug 261268 has been marked as a duplicate of this bug. ***
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.
Status: NEW → ASSIGNED
Depends on: 262188
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.
Attached patch work-in-progress (obsolete) — Splinter Review
Attaching work in progress.
Attached patch Cleaned up code. (obsolete) — Splinter Review
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
Attachment #164314 - Attachment is obsolete: true
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.
Attachment #165746 - Flags: review?(peterv)
Attached file notes
Attachment #165746 - Flags: review?(peterv)
Attachment #166886 - Flags: superreview?(peterv)
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.
Alias: xmlschema
Summary: Interface for checking a Node against a Schema type → Interface for checking a Node against a XML Schema type
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.

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
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.
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.
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
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 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.
Attachment #168325 - Attachment is obsolete: true
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?
Attachment #168341 - Flags: review?(darin)
Attachment #168341 - Attachment is obsolete: true
Attachment #168341 - Flags: review?(darin)
Comment on attachment 168352 [details] [diff] [review]
add allmakefiles.sh changes

r=jst for the build changes.
Attachment #168352 - Flags: superreview+
checked in, not built by default.

To build it, you need in your .mozconfig:
ac_add_options --enable-extensions="default,schema-validation"
(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...
you never got the email, sorry - you need the patch from
https://bugzilla.mozilla.org/show_bug.cgi?id=263384  to make it build.
Depends on: 263384
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
Attachment #166886 - Flags: superreview?(peterv) → superreview-
> - 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.
>+      // 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?
(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.
Depends on: 278449
Attached patch Addresses peterv's comments (obsolete) — Splinter Review
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.
Attachment #166886 - Attachment is obsolete: true
Comment on attachment 171299 [details] [diff] [review]
Addresses peterv's comments

sorry, this patch is missing some parts.
Attachment #171299 - Attachment is obsolete: true
Attachment #171545 - Flags: superreview?(peterv)
Attachment #171545 - Attachment is obsolete: true
Attachment #171545 - Flags: superreview?(peterv)
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.
Attachment #172012 - Flags: superreview?(peterv)
Blocks: 279187
Blocks: 281987
Attachment #172012 - Attachment is obsolete: true
Attachment #176607 - Flags: superreview?(peterv)
Attachment #172012 - Flags: superreview?(peterv)
Comment on attachment 176607 [details] [diff] [review]
With peterv's commentes addressed

new patch coming
Attachment #176607 - Attachment is obsolete: true
Attachment #176607 - Flags: superreview?(peterv)
Attachment #182530 - Flags: superreview?(peterv)
Attachment #182530 - Attachment is obsolete: true
Attachment #182530 - Flags: superreview?(peterv)
Attached patch more changes (obsolete) — Splinter Review
Includes changes needed by xforms and complex type validtion (actual complex
type validation code not included).
Attachment #184319 - Flags: superreview?(peterv)
Blocks: 296815
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
Attachment #187211 - Attachment description: a patch that correct some bugs → a patch that fix some bugs
Peterv gave rs= on irc for the patch to go in.

laurent: I'll incorporate your fixes as well, nice catch with gMonth there.
I think the AIX bustage fix was wrong... maxExclusive.EqualsLiteral("1"); is a
comparison, not an assignment; I think you want .AssignLiteral("1")
(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.
Assignee: doronr → web-services
Status: ASSIGNED → NEW
QA Contact: ashshbhatt → doronr
Assignee: web-services → doronr
I'm getting LONG_MAX and LONG_MIN aren't defined errors in
nsSchemaValidatorUtils.cpp now. I'm building using MSVS .NET 2003.
(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
Can you please file a new bug on that issue?
Status: NEW → ASSIGNED
I think the fix would be to #include <limits.h>
(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
Bustage patch checked in, thanks for catching that (my windows build was failing
in nss so never got to it).
(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 :)

Attachment #184319 - Flags: superreview?(peterv)
Attached patch more simple types (obsolete) — Splinter Review
- 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.
Attachment #184319 - Attachment is obsolete: true
Attachment #187954 - Flags: superreview?(peterv)
Comment on attachment 187954 [details] [diff] [review]
more simple types

went in as part of another patch
Attachment #187954 - Attachment is obsolete: true
Attachment #187954 - Flags: superreview?(peterv)
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.
QA Contact: doronr → web-services
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.