299173 - More than one schema per namespace allowed within <xf:model>

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[Stephen Pride]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050509 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050509 Firefox/1.0.4

From XForms spec ...

"3.3.1 (schema) Within each XForms Model, there is a limit of one Schema per
namespace declaration, including inline and linked Schemas."

However, using more than one schema per namespace does not cause a problem.

I will attach a test case.

Reproducible: Always

Comment 1

[Stephen Pride]

Attachment: testcase

Comment 2

[aaronr]

not really clear what this means or what the error should be.  We'll have to ask
the working group at some point.

Comment 3

[Steve Speicher]

Test case: 3.3.1.c3
still valid

Comment 4

[Merle Sterling]

Attachment: testcase2

Additional testcase that is slightly different than the first testcase.  This version uses the same name (simpleType name 'answer') but each schema element defines the name as a different type.

Comment 5

[Merle Sterling]

nsSchemaLoader maintains a hashtable of schemas keyed by targetNamespace. If processSchemaElement finds an existing schema with the same targetNamespace in the hashtable, it returns that schema. Thus we only process the types defined in the first instance of the schema.

Jan Kratky will get clarifictaion from the working group as to whether or not this should be considered a fatal error and if so we can display the fatal error dialog instead of returning the prior schema from the hashtable.

Comment 6

[Allan Beaufour]

(In reply to comment #5)
> Jan Kratky will get clarifictaion from the working group as to whether or not
> this should be considered a fatal error and if so we can display the fatal
> error dialog instead of returning the prior schema from the hashtable.

http://lists.w3.org/Archives/Member/w3c-forms/2006JanMar/0348.html

Comment 7

[Steve Speicher]

Based on some research, it appears that XSmiles and formsPlayer use the last schema defined and don't generate an error.

Also, looking at a similar feature in XML Schema called xsi:schemaLocation.  It turns out that usage of xsi:schemaLocation with multiple schemas of the same namespace, only the 1st one is used.  All the others are ignored (based on Xerces-J).  I've been digging through the XML Schema Structures spec to get confirmation on this and haven't located specific wording.

So I recommend that this be the course of action for this bug:
  1) leave the processing as-is (use the first schema found for namespace, like xsi:schemaLocation)
  2) register an error per 'extra' schema definition for the same namespace in the JS console
  3) dispatch a xforms-link-error per 'extra' schema definition

If the spec gets changed at some point regarding this, we can open a new bug for the behaviour.  If the author wants to merge schemas, then there is the xsi:include mechanism that is readily available.

Comment 8

[Merle Sterling]

Attachment: Check for duplicate schemas

If a duplicate schema is found (same target namespace), write an error to the JS Console and dispatch the LinkError event. This is not a fatal error and only the first such schema is processed.

Comment 9

[aaronr]

Comment on attachment 222439 [details] [diff] [review]
Check for duplicate schemas

>Index: nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.118
>diff -u -8 -p -r1.118 nsXFormsModelElement.cpp
>--- nsXFormsModelElement.cpp	17 May 2006 19:30:36 -0000	1.118
>+++ nsXFormsModelElement.cpp	18 May 2006 00:37:44 -0000

>@@ -2592,16 +2597,53 @@ nsXFormsModelElement::HandleLoad(nsIDOME
> nsresult
> nsXFormsModelElement::HandleUnload(nsIDOMEvent* aEvent)
> {
>   // due to fastback changes, had to move this notification out from under
>   // model's WillChangeDocument override.
>   return nsXFormsUtils::DispatchEvent(mElement, eEvent_ModelDestruct);
> }
> 
>+/**
>+ * Returns true if a duplicate schema (same targetNamespace) has already
>+ * been processed. Per section 3.3.1 of the XForms spec: Within each XForms
>+ * Model, there is a limit of one Schema per namespace declaration, including
>+ * inline and linked Schemas.
>+ *
>+ * @param aSchemaElement The schema element
>+ */

nit: You should just have this comment in the header, not both places.

>+PRBool
>+nsXFormsModelElement::IsDuplicateSchema(nsIDOMElement *aSchemaElement)
>+{
>+  PRBool isDuplicate = PR_FALSE;
>+  nsCOMPtr<nsISchemaCollection> schemaColl = do_QueryInterface(mSchemas);
>+  if (schemaColl) {

nit: I'd exit here if !schemaColl.  You'll see that pretty much all over the XForms code we return as soon as we see a result that we know can't change.  Saves a lot of indentation :-)  Also if you exit like this you won't need the isDuplicate variable.

>+    const nsAFlatString& empty = EmptyString();
>+    nsAutoString targetNamespace;
>+    aSchemaElement->GetAttributeNS(empty,
>+                                   NS_LITERAL_STRING("targetNamespace"),
>+                                   targetNamespace);

Don't you then need to trim the namespace and then get the URI like they do in nsSchema::nsSchema?

>+    nsCOMPtr<nsISchema> schema;
>+    schemaColl->GetSchema(targetNamespace, getter_AddRefs(schema));
>+    if (schema) {

nit: I'd exit here if !schema.  Also, please add a newline in between the end of this function and the next function.

>   NS_ADDREF(*aResult);
>Index: nsXFormsModelElement.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.h,v
>retrieving revision 1.46
>diff -u -8 -p -r1.46 nsXFormsModelElement.h
>--- nsXFormsModelElement.h	15 May 2006 14:14:52 -0000	1.46
>+++ nsXFormsModelElement.h	18 May 2006 00:37:45 -0000
>@@ -354,16 +354,25 @@ private:
>    */
>   NS_HIDDEN_(nsresult) HandleUnload(nsIDOMEvent *aEvent);
> 
>   NS_HIDDEN_(nsresult) RefreshSubTree(nsXFormsControlListItem *aCurrent,
>                                       PRBool                   aForceRebind);
> 
>   NS_HIDDEN_(nsresult) ValidateDocument(nsIDOMDocument *aInstanceDocument,
>                                         PRBool         *aResult);
>+  /**
>+   * Returns true if a duplicate schema (same targetNamespace) has already
>+   * been processed. Per section 3.3.1 of the XForms spec: Within each XForms
>+   * Model, there is a limit of one Schema per namespace declaration, including
>+   * inline and linked Schemas.
>+   *
>+   * @param aSchemaElement The schema element
>+   */
>+  PRBool IsDuplicateSchema(nsIDOMElement *aSchemaElement);

nit: I'd change this to "Returns true if a schema has already been registered to address the same namespace as aSchemaElement".  I think that is clearer than saying 'duplicate schema'

>Index: resources/locale/en-US/xforms.properties
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.properties,v
>retrieving revision 1.27
>diff -u -8 -p -r1.27 xforms.properties
>--- resources/locale/en-US/xforms.properties	17 May 2006 08:06:12 -0000	1.27
>+++ resources/locale/en-US/xforms.properties	18 May 2006 00:37:45 -0000
>@@ -69,16 +69,17 @@ externalLink1Error   = XForms Error (30)
> externalLink2Error   = XForms Error (31): Failed to load %S element from external file: %S
> externalLinkLoadOrigin = XForms Error (32): Security check failed! Trying to load %S data from a different domain than document
> instanceNotFound     = XForms Error (33): Could not find instance with id == '%S'
> defInstanceNotFound  = XForms Error (34): Could not find default instance
> MDGLoopError         = XForms Error (35): There are loops in the bindings of the model!
> invalidExtFunction   = XForms Error (36): Non-existant extension functions listed in this model's function attribute
> inlineInstanceNoChildError = XForms Error (37): Inline instance has no child elements. This is illegal.
> inlineInstanceMultipleElementsError = XForms Error (38): Inline instance has multiple child elements. This is illegal.
>+duplicateSchema = XForms Error (39): Ignoring duplicate schema with target namespace: %S

nit: please line up the '= XForms Error....' with MDGLoopError and invalidExtFunction


r-'ing since you don't get the namespace URI, which I think that you need.  Other than that, just nits.

Comment 10

[Merle Sterling]

(In reply to comment #9)
>+    const nsAFlatString& empty = EmptyString();
> >+    nsAutoString targetNamespace;
> >+    aSchemaElement->GetAttributeNS(empty,
> >+                                   NS_LITERAL_STRING("targetNamespace"),
> >+                                   targetNamespace);
> Don't you then need to trim the namespace and then get the URI like they do in
> nsSchema::nsSchema?

Trim can't hurt but no I do not need to get the namespace URI. The purpose of IsDuplicateSchema is to determine if we have already processed a schema with the same target namespace.  The first time the schema is encountered, there will not be an nsSchema object in the collection and we will proceed to call nsSchemaLoader::ProcessSchemaElement which will create the nsSchema object, initialize it (including the namespace URI), and process it. The collection maintains a hashtable that maps target namespaces to nsSchema instances and that is what we care about to check if the schema already exists. The Get from the hashtable won't even look at the namespace uri.

Fixed the other spacing and indentation nits.

Comment 11

[Merle Sterling]

Attachment: Trim targetNamespace; fix spacing nits

Comment 12

[aaronr]

Comment on attachment 222622 [details] [diff] [review]
Trim targetNamespace; fix spacing nits

Yep, you are right.  You don't need the GetNamespaceURI like I thought.  I misread nsSchema::nsSchema.  I thought it was using GetNamespaceURI to assign into the target namespace that it used, but it is actually assigning into another variable, not mTargetNamespace.

looks good.  r=me

Comment 13

[aaronr]

Attachment: patch for checkin

patch for checkin.  Updated to the trunk

Comment 14

[aaronr]

checked into trunk for msterlin