Last Comment Bug 308500 - need to write hexBinary schema validation code
: need to write hexBinary schema validation code
Status: RESOLVED FIXED
: fixed1.8.0.2, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Doron Rosenberg (IBM)
: Stephen Pride
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-14 08:41 PDT by jhp (no longer active)
Modified: 2016-07-15 14:46 PDT (History)
0 users
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
le patch (6.92 KB, patch)
2005-11-01 15:11 PST, Doron Rosenberg (IBM)
jhpedemonte: review+
Details | Diff | Splinter Review
fixes pedemonte's comments (7.08 KB, patch)
2005-11-02 12:16 PST, Doron Rosenberg (IBM)
aaronr: review-
Details | Diff | Splinter Review
better patch (7.92 KB, patch)
2005-11-04 08:22 PST, Doron Rosenberg (IBM)
aaronr: review+
Details | Diff | Splinter Review

Description jhp (no longer active) 2005-09-14 08:41:41 PDT
Offshoot of bug 275453.
Comment 1 Doron Rosenberg (IBM) 2005-11-01 15:11:24 PST
Created attachment 201567 [details] [diff] [review]
le patch
Comment 2 jhp (no longer active) 2005-11-02 10:24:54 PST
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) 2005-11-02 12:16:14 PST
Created attachment 201660 [details] [diff] [review]
fixes pedemonte's comments
Comment 4 aaronr 2005-11-03 16:56:19 PST
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) 2005-11-04 08:22:48 PST
Created attachment 201854 [details] [diff] [review]
better patch

aResult will always be present, since nsSchemaValidator is the only one calling those methods.
Comment 6 aaronr 2005-11-04 11:40:54 PST
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) 2005-11-07 08:36:19 PST
checked into trunk
Comment 8 aaronr 2006-02-02 17:32:10 PST
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0
Comment 9 aaronr 2006-07-07 11:46:29 PDT
verified fixed in MOZILLA_1_8_BRANCH

Note You need to log in before you can comment on or make changes to this bug.