335366 - Add support for <xsd:include>

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[Steve Speicher]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

In utilizing reword XML-based applications, their data models are most often represented in reusable modules utilizing the XML Schema construction <xsd:include>

This is the case of schemas I/we have seen from groups like US Center for Disease Control, etc.

(opening separate enhancement request for xsd:import and xsd:redefine, as they have different priorities and may require different amount of effort)

Reproducible: Always

Comment 1

[Steve Speicher]

See related bug 335367

Comment 2

[Doron Rosenberg (IBM)]

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

Comment 3

[Doron Rosenberg (IBM)]

Attachment: patch

I hacked this up today, it needs more testing with large schemas and all.  This does most of the required "is the included file a schema file" checks, other than the targetNamespace check (XXX: is in the patch for that).

Comment 4

[Doron Rosenberg (IBM)]

Attachment: patch that does relative urls and security checks

Comment 5

[aaronr]

Comment on attachment 220048 [details] [diff] [review]
patch that does relative urls and security checks

>Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/webservices/schema/src/nsSchemaLoader.cpp,v
>retrieving revision 1.48
>diff -u -r1.48 nsSchemaLoader.cpp
>--- extensions/webservices/schema/src/nsSchemaLoader.cpp	15 Mar 2006 17:13:09 -0000	1.48
>+++ extensions/webservices/schema/src/nsSchemaLoader.cpp	27 Apr 2006 19:47:54 -0000

>@@ -871,8 +841,144 @@
>       if (NS_SUCCEEDED(rv)) {
>         rv = schemaInst->AddModelGroup(modelGroup);
>       }
>+    }
>+    else if (tagName == nsSchemaAtoms::sInclude_atom) {
>+      /* http://www.w3.org/TR/2004/REC-xmlschema-1-20041028/structures.html#element-include
>+         If we include a schema, it must either
>+           (a) have the same targetNamespace as the including schema document or
>+           (b) no targetNamespace at all
>+
>+         If the uri to load doesn't resolve, it isn't a error.  It is if its an
>+         invalid XML document or not a schema file
>+       */
>+
>+      PRBool hasSchemaLocationAttr = PR_FALSE;
>+      childElement->HasAttribute(NS_LITERAL_STRING("schemaLocation"),
>+                                 &hasSchemaLocationAttr);
>+
>+      // no schema location attribute, skip it
>+      if (!hasSchemaLocationAttr)
>+        continue;
>+
>+      nsAutoString schemalocation;
>+      childElement->GetAttribute(NS_LITERAL_STRING("schemaLocation"),
>+                                 schemalocation);
>+

shouldn't you use a named literal string since you are (potentially) using the same string twice in the same function?  How about testing if schemalocation is an empty string before going on?

>+      nsCOMPtr<nsIIOService> ios = do_GetIOService();
>+      NS_ENSURE_STATE(ios);
>+
>+      nsCOMPtr<nsIDOMDocument> document;
>+      aElement->GetOwnerDocument(getter_AddRefs(document));
>+
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(document);
>+      NS_ENSURE_STATE(doc);
>+
>+      nsCOMPtr<nsIURI> uri;
>+
>+      ios->NewURI(NS_ConvertUTF16toUTF8(schemalocation),
>+                  doc->GetDocumentCharacterSet().get(),
>+                  doc->GetDocumentURI(),
>+                  getter_AddRefs(uri));
>+      NS_ENSURE_STATE(uri);
>+
>+      nsCAutoString spec;
>+      uri->GetSpec(spec);

GetSpec should probably be moved below the principal stuff.  No since getting the spec if the principals are just going to fail.

>+
>+      // make sure we can load it - do a principal same origin check.
>+      // get the base document's principal
>+      nsIPrincipal *basePrincipal = doc->GetNodePrincipal();
>+      NS_ENSURE_STATE(basePrincipal);
>+

I thought that they did away with GetNodePrincipal.  Shouldn't this be NodePrincipal()?

>+      // XXX: check the target namespace requirements

make sure that you open a bug on this so we don't forget to do it.


>@@ -3254,3 +3360,51 @@
> 
>   return rv;
> }
>+
>+nsresult
>+nsSchemaLoader::GetDocumentFromURI(const nsAString& aUri,
>+                                   nsIDOMDocument** aDocument)
>+{
>+  nsCOMPtr<nsIURI> resolvedURI;
>+  nsresult rv = GetResolvedURI(aUri, "load", getter_AddRefs(resolvedURI));
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

what, is NS_ENSURE_SUCCESS(rv, rv) too good for you?  Can use it mult. times below here, too.

>+  nsCAutoString spec;
>+  resolvedURI->GetSpec(spec);
>+  
>+  nsCOMPtr<nsIXMLHttpRequest> request(do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv));
>+  if (!request) {
>+    return rv;
>+  }

move spec get below this return otherwise it is wasted.  You could also use NS_ENSURE_TRUE(request, rv) instead of the if test.

Nothing major in my comments, so with those fixed, r=me

Comment 6

[Doron Rosenberg (IBM)]

Attachment: with comments addressed

peterv, could you give this a look?  We'd like this for ff 2.0.  thanks!

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 221317 [details] [diff] [review]
with comments addressed

>Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
>===================================================================

>@@ -864,22 +834,164 @@ nsSchemaLoader::ProcessSchemaElement(nsI

>+         If the uri to load doesn't resolve, it isn't a error.  It is if its an

it's

>+      NS_NAMED_LITERAL_STRING(schemaLocationStr, "schemaLocation");
>+      PRBool hasSchemaLocationAttr = PR_FALSE;
>+      childElement->HasAttribute(schemaLocationStr, &hasSchemaLocationAttr);
>+
>+      // no schema location attribute, skip it
>+      if (!hasSchemaLocationAttr)
>+        continue;

No need for this block, the schemalocation.IsEmpty() check below will catch this.

>+      nsCOMPtr<nsIIOService> ios = do_GetIOService();
>+      NS_ENSURE_STATE(ios);

No need for this if you use NS_NewURI below.

>+      nsCOMPtr<nsIURI> uri;
>+
>+      ios->NewURI(NS_ConvertUTF16toUTF8(schemalocation),
>+                  doc->GetDocumentCharacterSet().get(),
>+                  doc->GetDocumentURI(),
>+                  getter_AddRefs(uri));
>+      NS_ENSURE_STATE(uri);

I'm confused how this fits with "If the uri to load doesn't resolve, it isn't a error."
Also do:
      rv = NS_NewURI(getter_AddRefs(uri), NS_ConvertUTF16toUTF8(schemalocation),
                     doc->GetDocumentCharacterSet().get(),
                     doc->GetDocumentURI());
      NS_ENSURE_SUCCESS(rv, rv);

>+      rv = GetDocumentFromURI(NS_ConvertUTF8toUTF16(spec), getter_AddRefs(includedDocument));

Long line.

>+          if (nextSibling) {
>+            rv = aElement->InsertBefore(importedNode, nextSibling,
>+                                        getter_AddRefs(dummy));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+          } else {
>+            rv = aElement->AppendChild(importedNode, getter_AddRefs(dummy));
>+            NS_ENSURE_SUCCESS(rv, rv);
>+          }

Just do:

          rv = aElement->InsertBefore(importedNode, nextSibling,
                                      getter_AddRefs(dummy));
          NS_ENSURE_SUCCESS(rv, rv);

InsertBefore can deal with a null refChild (it'll append).

>+nsSchemaLoader::GetDocumentFromURI(const nsAString& aUri,

>+  nsCOMPtr<nsIXMLHttpRequest> request(do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv));

Long line, I'd do:

  nsCOMPtr<nsIXMLHttpRequest> request =
    do_CreateInstance(NS_XMLHTTPREQUEST_CONTRACTID, &rv);

>+  NS_ENSURE_TRUE(request, rv);

NS_ENSURE_SUCCESS(rv, rv);

>+    document.swap(*aDocument);

You should set *aDocument to nsnull at the beginning of the method.

Comment 8

[Doron Rosenberg (IBM)]

checked into trunk

Comment 9

[Doron Rosenberg (IBM)]

Comment on attachment 221317 [details] [diff] [review]
with comments addressed

Would like this for ff 2.0 - xforms needs it,and this is low-risk (since only web services uses it).

Comment 10

[Darin Fisher]

What are risks with this patch?  It is pretty large, and that makes me nervous.  Unfortunately, this code can be reached w/o the XForms extensions, so we have to worry somewhat about regressions to Firefox (even though there are probably next to no websites making use of our webservices stack).  Thoughts?

I really wish there were some regression/unit tests for this code so we could have some extra confidence when making these kinds of changes :-)

Comment 11

[Doron Rosenberg (IBM)]

(In reply to comment #10)
> What are risks with this patch?  It is pretty large, and that makes me nervous.
>  Unfortunately, this code can be reached w/o the XForms extensions, so we have
> to worry somewhat about regressions to Firefox (even though there are probably
> next to no websites making use of our webservices stack).  Thoughts?
> 

A large part of the patch was moving code into a function so that it can be reused.  The rest is new functionality, and it complies with the Mozilla security guidelines (cross domain principal checking).

I did run working webservices tests we have on mozilla.org and they didn't show any regressions.

Comment 12

[Darin Fisher]

Comment on attachment 221317 [details] [diff] [review]
with comments addressed

a=darin on behalf of drivers

Comment 13

[Doron Rosenberg (IBM)]

thanks for approval!

Comment 14

[Doron Rosenberg (IBM)]

Comment on attachment 221317 [details] [diff] [review]
with comments addressed

Approved bug 335367 actually needs this patch to work, so asking for approval, though might be too late for 1.8.0.9?

Comment 15

[Jay Patel [:jay]]

Comment on attachment 221317 [details] [diff] [review]
with comments addressed

Approved for 1.8.0 branch, a=jay for drivers.  Please land this asap.  Thanks!

Comment 16

[aaronr]

checked into MOZILLA_1_8_0_BRANCH