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

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
10 months ago

People

(Reporter: Stephen Pride, Assigned: Merle Sterling)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Created attachment 187694 [details]
testcase

Comment 2

12 years ago
not really clear what this means or what the error should be.  We'll have to ask
the working group at some point.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

12 years ago
Blocks: 322255

Updated

11 years ago
Blocks: 326372

Updated

11 years ago
Blocks: 326373

Comment 3

11 years ago
Test case: 3.3.1.c3
still valid

Updated

11 years ago
Assignee: aaronr → msterlin
(Assignee)

Comment 4

11 years ago
Created attachment 216131 [details]
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.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

11 years ago
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

11 years ago
(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

11 years ago
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.
(Assignee)

Comment 8

11 years ago
Created attachment 222439 [details] [diff] [review]
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.
Attachment #222439 - Flags: review?(aaronr)

Comment 9

11 years ago
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.
Attachment #222439 - Flags: review?(aaronr) → review-
(Assignee)

Comment 10

11 years ago
(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.
(Assignee)

Comment 11

11 years ago
Created attachment 222622 [details] [diff] [review]
Trim targetNamespace; fix spacing nits
Attachment #222439 - Attachment is obsolete: true
Attachment #222622 - Flags: review?(aaronr)

Comment 12

11 years ago
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
Attachment #222622 - Flags: review?(aaronr) → review+
(Assignee)

Updated

11 years ago
Attachment #222622 - Flags: review?(Olli.Pettay)

Updated

11 years ago
Attachment #222622 - Flags: review?(Olli.Pettay) → review+

Updated

11 years ago
Attachment #216131 - Attachment mime type: text/plain → application/xhtml+xml

Comment 13

11 years ago
Created attachment 223134 [details] [diff] [review]
patch for checkin

patch for checkin.  Updated to the trunk
Attachment #222622 - Attachment is obsolete: true

Comment 14

11 years ago
checked into trunk for msterlin
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

Updated

11 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.