402250 - Need to build validation into the query mechanism

Status: RESOLVED FIXED

(Toolkit Graveyard :: Microformats, defect)

Description

[Mike Kaply [:mkaply]]

Currently all DOM nodes that have the appropriate microformat class are returned even if they are not valid microformats. It's up to the implementor to figure out what to do if they are not valid.

This isn't the right way to do it.

I should have validation in the API so that only valid microformats come back, and an implementor can choose to get invalid ones if they want to use them for debug purposes.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Patch for problem

I'll cover the entire patch separately

Comment 2

[Mike Kaply [:mkaply]]

Comment on attachment 287135 [details] [diff] [review]
Patch for problem

Please see the original patch for full context.


>-      targetArray.push(new Microformats[name].mfObject(microformatNodes[i]));
>+      try {
>+        targetArray.push(new Microformats[name].mfObject(microformatNodes[i]));
>+      } catch (ex) {
>+        /* Creation of individual object probably failed because it is invalid. */
>+        /* This isn't a problem, because the page might have invalid microformats */
>+      }

This section puts the creation of the semantic Object nodes in a try catch since now we throw errors when creation fails.

>   count: function(name, rootElement, recurseFrames) {

Count has been completely replaced by a call to get and a count. This was necessary because in the past, count didn't create actual semantic objects so it couldn't validate them.

>+    /* If Microformats.parser.validation is false, we do not attempt to validate */
>+    /* the node. This is useful for debugging microformats. */
>+    validation: true,

Parser attribute to allow extensions to do their own validation.


>         replace: function (a, b) {
>-          return this.toString().replace(a,b);
>+          if (this.toString()) {
>+            return this.toString().replace(a,b);
>+          } else {
>+            return this;
>+          }
>         },
>         match: function (a) {
>-          return this.toString().match(a);
>+          if (this.toString()) {
>+            return this.toString().match(a);
>+          } else {
>+            return this;
>+          }

Unrelated fix - this fixes an edge case where toString() didn't work on some objects.

>+      
>+
>+      if (Microformats.parser.validation) {
>+        Microformats.parser.validate(object, microformat);
>+      }

Do the actual validation during object creation.


>-            includes[i].parentNode.replaceChild(in_mfnode.ownerDocument.getElementById(includeId).cloneNode(true), includes[i]);
>+            if (in_mfnode.ownerDocument.getElementById(includeId)) {
>+              includes[i].parentNode.replaceChild(in_mfnode.ownerDocument.getElementById(includeId).cloneNode(true), includes[i]);
>+            }

Fix bug where an invalid microformat include broke parsing.
 var headerNode = in_mfnode.ownerDocument.getElementById(headers[i]);
>-            tempNode.innerHTML = headerNode.innerHTML;
>-            tempNode.className = headerNode.className;
>-            mfnode.appendChild(tempNode);
>+            if (headerNode) {
>+              tempNode.innerHTML = headerNode.innerHTML;
>+              tempNode.className = headerNode.className;
>+              mfnode.appendChild(tempNode);
>+            }

Fix bug where no headerNode broke parsing.


>-    validate: function validate(mfnode, mfname, error) {
>+    validate: function validate(mfobject, mfname) {

validate rewritten primarily to do throw's instead of just passing error messages and not to use "toString()" to decide if a microformat was invalid. There are only two ways a microformat can be invalid. Either it doesn't have a "required" item, or a call to the internal validate function for a microformat failed.


>-    if ((rootNode.ownerDocument || rootNode).getElementsByClassName) {
>+    if ((content.document.getElementsByClassName) && (content.document.getElementsByClassName.toString().match("native code"))) {

Better checking for Firefox 3 getElementsByClassName

>+  },
>+  validate: function(mfobject) {
>+    var node = mfobject.node;
>+    var xpathExpression = "count(descendant::*[" +
>+                                              "contains(concat(' ', @class, ' '), ' post-office-box ')" +
>+                                              " or contains(concat(' ', @class, ' '), ' street-address ')" +
>+                                              " or contains(concat(' ', @class, ' '), ' extended-address ')" +
>+                                              " or contains(concat(' ', @class, ' '), ' locality ')" +
>+                                              " or contains(concat(' ', @class, ' '), ' region ')" +
>+                                              " or contains(concat(' ', @class, ' '), ' postal-code ')" +
>+                                              " or contains(concat(' ', @class, ' '), ' country-name')" +
>+                                              "])";
>+    var xpathResult = (node.ownerDocument || node).evaluate(xpathExpression, node, null,  Components.interfaces.nsIDOMXPathResult.ANY_TYPE, null).numberValue;
>+    if (xpathResult == 0) {
>+      throw("Unable to create microformat");
>+    }
>+    return true;
>   }
> };

Add a custom validate function for the adr microformat. Mich more efficient



> 
>@@ -1631,24 +1633,18 @@
>     }
>     return returnTag;
>   },
>-  validate: function(node, error) {
>-    var tag = Microformats.parser.getMicroformatProperty(node, "tag", "tag");
>+  validate: function(mfobject) {
>+    var tag = Microformats.parser.getMicroformatProperty(mfobject.node, "tag", "tag");
>     if (!tag) {
>-      if (node.href) {
>-        var url_array = node.getAttribute("href").split("/");
>+      if (mfobject.node.href) {
>+        var url_array = mfobject.node.getAttribute("href").split("/");
>         for(let i=url_array.length-1; i > 0; i--) {
>           if (url_array[i] !== "") {
>-            if (error) {
>-              error.message = "Invalid tag name (" + url_array[i] + ")";
>-            }
>-            return false;
>+            throw("Invalid tag name (" + url_array[i] + ")");;
>           }
>         }
>       } else {
>-        if (error) {
>-          error.message = "No href specified on tag";
>-        }
>-        return false;
>+        throw("No href specified on tag");
>       }
>     }
>     return true;

validate now takes an object, not a node so this impl had to be changed.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Better patch with testcase

This is a better patch including a testcase.

Here's what I've done.

The microformats code now throws if you try to explicitly create a microformat using a node that is invalid.

When you do a .get or .count on a page to do stuff with all the microformats, by default invalid microformats are NOT in the list. We catch the exceptions.

If you add a new parameter (debug: true) in options, you get all microformats in the list including invalid ones. This is so an application can allow you to see all microformats.

To make this happen, I had to a a new parameter to the creation of all individual microformats that tells whether validate should be called or not.

Comment 4

[Damon Sicore (:damons)]

Comment on attachment 298305 [details] [diff] [review]
Better patch with testcase

a1.9+=damons

Comment 5

[Mike Kaply [:mkaply]]

checked in.