308500 - need to write hexBinary schema validation code

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[jhp (no longer active)]

Offshoot of bug 275453.

Comment 1

[Doron Rosenberg (IBM)]

Attachment: le patch

Comment 2

[jhp (no longer active)]

Comment on attachment 201567 [details] [diff] [review]
le patch

+    // For hexBinary, length is measured in octets (8 bits) of binary data.

To this, add another line about "one byte of binary data is represented by two hex digits, so divide by 2 to get binary data length."  Or something like that.  You may even want to rename "length" to "binaryDataLength" to make it more obvious.

+  if (length % 2 == 0) {

Rather than encapsulating the main code of the funtion in this if statement, just do an early return for "!= 0".  Then you won't need to have two PRBools ('isValid' and 'foundError').

+        validate("0FB7", "hexbinary-test-1", true);
+        validate("023-", "hexbinary-test-1", false);
+        validate("0HB7", "hexbinary-test-1", false);

Add another test for an odd length string.

With that, r+.

Comment 3

[Doron Rosenberg (IBM)]

Attachment: fixes pedemonte's comments

Comment 4

[aaronr]

Comment on attachment 201660 [details] [diff] [review]
fixes pedemonte's comments

>? extensions/schema-validation/f
>Index: extensions/schema-validation/src/nsSchemaValidator.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/schema-validation/src/nsSchemaValidator.cpp,v
>retrieving revision 1.15
>diff -w -u -r1.15 nsSchemaValidator.cpp
>--- extensions/schema-validation/src/nsSchemaValidator.cpp	14 Oct 2005 16:49:47 -0000	1.15
>+++ extensions/schema-validation/src/nsSchemaValidator.cpp	2 Nov 2005 20:13:28 -0000

>@@ -2777,6 +2791,84 @@
>   return isValid;
> }
> 
>+// http://www.w3.org/TR/xmlschema-2/#hexBinary
>+nsresult
>+nsSchemaValidator::ValidateBuiltinTypeHexBinary(const nsAString & aNodeValue,
>+                                                PRUint32 aLength,
>+                                                PRBool aLengthDefined,
>+                                                PRUint32 aMinLength,
>+                                                PRBool aMinLengthDefined,
>+                                                PRUint32 aMaxLength,
>+                                                PRBool aMaxLengthDefined,
>+                                                nsStringArray *aEnumerationList,
>+                                                PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;

Do you want to test if aResult even exists before going through the pain of figuring out if the value is valid?

>+
>+  isValid = IsValidSchemaHexBinary(aNodeValue);
>+
>+  if (isValid) {
>+    // For hexBinary, length is measured in octets (8 bits) of binary data.  So
>+    // one byte of binary data is represented by two hex digits, so we
>+    // divide by 2 to get the binary data length.
>+
>+    PRUint32 binaryDataLength = aNodeValue.Length() / 2;
>+
>+    if (aLengthDefined && (binaryDataLength != aLength)) {
>+      isValid = PR_FALSE;
>+      LOG(("  Not valid: Not the right length (%d)", binaryDataLength));
>+    }
>+
>+    if (aMinLengthDefined && (binaryDataLength < aMinLength)) {
>+      isValid = PR_FALSE;
>+      LOG(("  Not valid: Length (%d) is too small", binaryDataLength));
>+    }
>+
>+    if (aMaxLengthDefined && (binaryDataLength > aMaxLength)) {
>+      isValid = PR_FALSE;
>+      LOG(("  Not valid: Length (%d) is too large", binaryDataLength));
>+    }
>+  }

Shouldn't you return if isValid = PR_FALSE?  Why keep testing?  It can't go back to true and the continued logging doesn't seem to make sense in some cases.  Like how can the length be too small AND too large?

>+
>+  if (isValid && aEnumerationList && (aEnumerationList->Count() > 0)) {
>+    isValid = nsSchemaValidatorUtils::HandleEnumeration(aNodeValue, *aEnumerationList);
>+  }
>+
>+#ifdef PR_LOGGING
>+  LOG((isValid ? ("  Value is valid!") : ("  Value is not valid!")));
>+#endif
>+

Why is this log ifdef'd and the others aren't?

>+  *aResult = isValid;
>+  return NS_OK;
>+}
>+
>+PRBool
>+nsSchemaValidator::IsValidSchemaHexBinary(const nsAString & aString)
>+{
>+  // hex binary length has to be even
>+  PRUint32 length = aString.Length();
>+
>+  if (length % 2 != 0)
>+    return PR_FALSE;
>+
>+  nsAString::const_iterator start, end;
>+  aString.BeginReading(start);
>+  aString.EndReading(end);
>+
>+  PRBool isValid = PR_TRUE;
>+
>+  // each character has to be in [0-9a-fA-F]
>+  while ((start != end) && isValid) {
>+    PRUnichar temp = *start++;
>+
>+    if (!isxdigit(temp)) {
>+      isValid = PR_FALSE;
>+    }
>+  }
>+
>+  return isValid;
>+}
>+

Wouldn't it be more efficient to break out of the while loop as soon as you set isValid is false instead of checking for isValid on every iteration?

Comment 5

[Doron Rosenberg (IBM)]

Attachment: better patch

aResult will always be present, since nsSchemaValidator is the only one calling those methods.

Comment 6

[aaronr]

Comment on attachment 201854 [details] [diff] [review]
better patch


>+// http://www.w3.org/TR/xmlschema-2/#hexBinary
>+nsresult
>+nsSchemaValidator::ValidateBuiltinTypeHexBinary(const nsAString & aNodeValue,
>+                                                PRUint32 aLength,
>+                                                PRBool aLengthDefined,
>+                                                PRUint32 aMinLength,
>+                                                PRBool aMinLengthDefined,
>+                                                PRUint32 aMaxLength,
>+                                                PRBool aMaxLengthDefined,
>+                                                nsStringArray *aEnumerationList,
>+                                                PRBool *aResult)
>+{
>+  PRBool isValid = PR_FALSE;
>+
>+  isValid = IsValidSchemaHexBinary(aNodeValue);
>+

should move the IsValidSchemaHexBinary up to where isValid is initialized.

with that r=me.

Comment 7

[Doron Rosenberg (IBM)]

checked into trunk

Comment 8

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Comment 9

[aaronr]

verified fixed in MOZILLA_1_8_BRANCH