326413 - We should report when schemavalidation fails to look up a namespace

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[Allan Beaufour]

If a user creates a form using f.x. "xsd:integer" as a type, but forgets to bind "xsd" to the XML Schema namespace we should report that on the JS console. Right now we do nothing.

Comment 1

[Allan Beaufour]

Attachment: Testcase

Comment 2

[aaronr]

Good starter bug.

Comment 3

[Dexter]

It's my first time working on Mozilla, and I'd like to try my hand on this bug.

Comment 4

[aaronr]

Assigning to Dexter.  Let us know if you need any help.

Comment 5

[Dexter]

Just want to make sure:
1. By javascript console, you're referring to firefox's "error console"?
2. What error message string should be reported?
3. Does the xforms extension currently do any error reporting at all? I've been mangling the testcase here and there, but all I get are XML parse errors in the error console.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> Just want to make sure:
> 1. By javascript console, you're referring to firefox's "error console"?
Yes. Use is nsXFormsUtils::ReportError() or if the error reporting
must for some reason happen in schemavalidation module, do something
similar there.

> 2. What error message string should be reported?
Add some new error message to http://mxr.mozilla.org/seamonkey/source/extensions/xforms/resources/locale/en-US/xforms.properties
If it is XForms module which reports the error, maybe something like:
XForms Error (43): Couldn't bind prefix %S to any namespace.

> 3. Does the xforms extension currently do any error reporting at all? I've been
> mangling the testcase here and there, but all I get are XML parse errors in the
> error console.
Yes, via nsXFormsUtils::ReportError()

Comment 7

[Dexter]

Attachment: Proposed patch

Attached the patch.
It took a while for me to figure out where I should report the error, but I decided that's the best place to put it (when there's a prefix attached to the type, but it can't be looked up).

Comment 8

[alexander :surkov (:asurkov)]

is the patch ready for review?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 344467 [details] [diff] [review]
Proposed patch

> NS_IMETHODIMP nsXFormsModelElement::GetTypeForNode(nsIDOMNode     *aBoundNode,
>                                                    nsISVSchemaType **aType)
> {
>   nsAutoString schemaTypeName, schemaTypeNamespace;
>   nsresult rv = GetTypeFromNode(aBoundNode, schemaTypeName,
>                                 schemaTypeNamespace);
>-
>+    
Don't add extra space here,
...
>     if (schema)
>-      validator.LoadSchema(schema);
>+      validator.LoadSchema(schema);  
nor here
>   }
> 
>   if (validator.GetType(schemaTypeName, schemaTypeNamespace, aType))
>     rv = NS_OK;
>-  else
>+  else
nor here 
>     rv = NS_ERROR_FAILURE;
>-
>+  
nor here
>   return rv;
> }
> 
> /* static */ nsresult
> nsXFormsModelElement::GetTypeAndNSFromNode(nsIDOMNode *aInstanceData,
>                                            nsAString &aType, nsAString &aNSUri)
> {
>   // 6.2.1 1. see if the instance data has a schema type.
>@@ -2000,17 +2000,16 @@ nsXFormsModelElement::GetTypeFromNode(ns
>     prefix.Assign(Substring(*typeVal, 0, separator));
>     aType.Assign(Substring(*typeVal, ++separator, typeVal->Length()));
> 
>     if (prefix.IsEmpty()) {
>       aNSUri = EmptyString();
>       return NS_OK;
>     }
>   }
>-
Why to remove the newline.

>+  if (aNSUri.IsVoid()) {
>+    // if it's still not found, report the error (namespace not bound to any schema)
>+    const PRUnichar *strings[] = { prefix.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("namespaceBindingError"),
>+                 strings, 1, mElement, nsnull);
>+  }
>+
Align string right under NS_LITERAL_STRING


>-   *                         build a string array of datatype URIs and put them
>+   *                         build a string array of datatype URIs and put themm
???



>   nsRefPtrHashtable<nsISupportsHashKey, nsXFormsControlListItem> mControlListHash;
>-
>+  
Don't make these whitespace changes

>   PRInt32 mSchemaCount;
>   PRInt32 mSchemaTotal;
>   PRInt32 mPendingInstanceCount;
>-
>+ 
!

If you fix those, I could (re-)review.

Comment 10

[Dexter]

Attachment: Proposed patch #2

Sorry about that. Those changes were made in my first attempts to fix the bug, and I didn't revert exactly as it was. I'll be more careful next time.

This new patch is ready for review.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Usually you ask review by setting review flag to ? and add the bugmail address
of the reviewer.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 344556 [details] [diff] [review]
Proposed patch #2

>     rv = domNode3->LookupNamespaceURI(prefix, aNSUri);
>   }
> 
>+  if (aNSUri.IsVoid()) {
>+    // if it's still not found, report the error (namespace not bound to any schema)
>+    const PRUnichar *strings[] = { prefix.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("namespaceBindingError"),
>+                               strings, 1, mElement, nsnull);
>+  }
>+
Couldn't you move this up a bit to be right after rv = domNode3->LookupNamespaceURI(prefix, aNSUri);
Because otherwise if aNSUri already has some value, the second aNSUri.IsVoid()
check isn't needed.

Comment 13

[Dexter]

Attachment: Proposed patch #3

Review request method noted.

You're right. Sorry, force of habit. I've gotten used to minimize nesting loops.
Patch updated!

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 344626 [details] [diff] [review]
Proposed patch #3

And you could ask surkov to do "addl. review".

Comment 15

[Dexter]

Comment on attachment 344626 [details] [diff] [review]
Proposed patch #3

Requested additional review from surkov.

Comment 16

[alexander :surkov (:asurkov)]

I'm not sure GetTypeFromNode is right place to put ReportError. Is the better place GetTypeForNode method? Because we will report error if prefix type isn't bound to namespace but if it is and prefix type's namespace is not schema namespace then we won't fail.

Comment 17

[Dexter]

Originally, I intended to place it in the "else" block after:

[ In GetTypeForNode() ]
if ( schema ) 
   validator.LoadSchema(schema);
else
   // Report the error

But, I figured that it's already been determined in GetTypeFromNode() that a prefix was specified but no URI was found, so a warning should already be reported by then.

Do you think the first idea was better? Or am I way off on both ideas?

Comment 18

[alexander :surkov (:asurkov)]

I would guess it make sense to put ReportError in else block of:

if (validator.GetType(schemaTypeName, schemaTypeNamespace, aType))
    rv = NS_OK;
  else
    rv = NS_ERROR_FAILURE;

because !schema may be true for built-in datatypes.

Yes, we could report an error if prefix can't be looked up but we won't report error if there is no schema. Though I think you could report two errors. But I'm not sure. 

Olli, what do you think?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I think there could be 2 errors. It is useful to have the prefix-not-bound-to-any-namespace error, which the patch adds.

Comment 20

[Dexter]

I agree it might be good to report two errors. Or rather 1 error (if the datatype is not found at all) and 1 warning (if a prefix was specified but not bound to any namespace since the user might simply have forgotten to bind the namespace).

Comment 21

[alexander :surkov (:asurkov)]

Will you put new patch?

Comment 22

[Dexter]

Yes, I'll update the patch in a while.

Comment 23

[Dexter]

Attachment: Proposed patch #4

Added another error report.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 344883 [details] [diff] [review]
Proposed patch #4


>     if (schema)
>       validator.LoadSchema(schema);
>   }
> 
>   if (validator.GetType(schemaTypeName, schemaTypeNamespace, aType))
>     rv = NS_OK;

I would suggest to use "return NS_OK" here.

>-  else
>+  else {
>+    const PRUnichar *strings[] = { schemaTypeName.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("typeNotFoundError"),
>+                               strings, 1, mElement, nsnull);

Should we add schemaTypeNamespace to error message also?

>     domNode3 = do_QueryInterface(instanceNode, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
>     rv = domNode3->LookupNamespaceURI(prefix, aNSUri);
>+
>+    if (aNSUri.IsVoid()) {
>+      // if it's still not found, report the error (namespace not bound to any schema)

technically prefix is not namespace and here actually prefix isn't bound to namespace if I get right. So it's more correct to write "(cannot look up the prefix)".

>+warnNamespaceBinding      = XForms Warning (10): Could not bind %S to any namespace.

possibly: "Can't resolve namespace of prefix %S"? or similar.

Comment 25

[Dexter]

> I would suggest to use "return NS_OK" here.
Done. Although the original code uses "rv =".

> Should we add schemaTypeNamespace to error message also?
At this point, the schemaTypeNamespace would be blank if GetTypeFromNode() can't look up the prefix. Should we go on ahead and include it?

> technically prefix is not namespace and here actually prefix isn't bound to
> namespace if I get right. So it's more correct to write "(cannot look up the
> prefix)".
Done.

> possibly: "Can't resolve namespace of prefix %S"? or similar.
Done.

I'll update the patch after you confirm #2.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)

> > Should we add schemaTypeNamespace to error message also?
> At this point, the schemaTypeNamespace would be blank if GetTypeFromNode()
> can't look up the prefix. Should we go on ahead and include it?

It can be blank but it can be not, right? Because we can successfully resolve prefix but it's possible there is no schema type for obtained prefix's namespace + type name, no?

Comment 27

[Dexter]

Yes, that's right. In this case, is it okay to check first whether the namespace is blank, and if it isn't, then display it together with the typename?
Example: "Can't resolve type xsd:integer" or "Can't resolve type integer" (if xsd isn't resolved)

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> Yes, that's right. In this case, is it okay to check first whether the
> namespace is blank, and if it isn't, then display it together with the
> typename?
> Example: "Can't resolve type xsd:integer" or "Can't resolve type integer" (if
> xsd isn't resolved)

I guess it make sense, just you don't able to write "xsd:integer" because "xsd" is prefix but we have namespace for "xsd" at this point only. Though it's possible we talk about different things :)

Comment 29

[Dexter]

Ah yes, I understand. I'm sorry, I got a little mixed up.
In this case, is it better to say "Can't resolve type xxx in namespace xxx"?

Would it be reasonable to create two different error strings?
Maybe:
- Can't resolve type %S.
- Can't resolve type %S in namespace %S.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> Ah yes, I understand. I'm sorry, I got a little mixed up.
> In this case, is it better to say "Can't resolve type xxx in namespace xxx"?
> 
> Would it be reasonable to create two different error strings?
> Maybe:
> - Can't resolve type %S.
> - Can't resolve type %S in namespace %S.

fine with me I think.

Comment 31

[Dexter]

Attachment: Proposed patch #5

Requested changes made!

Comment 32

[Dexter]

Attachment: Proposed patch #5

Sorry, typo!

Comment 33

[Dexter]

Attachment: Proposed patch #5

Some minor corrections to the indentation.

Comment 34

[alexander :surkov (:asurkov)]

Comment on attachment 345006 [details] [diff] [review]
Proposed patch #5

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: mozilla/extensions/xforms/nsXFormsModelElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v
>retrieving revision 1.164
>diff -u -8 -p -r1.164 nsXFormsModelElement.cpp
>--- mozilla/extensions/xforms/nsXFormsModelElement.cpp	26 Aug 2008 14:38:09 -0000	1.164
>+++ mozilla/extensions/xforms/nsXFormsModelElement.cpp	27 Oct 2008 22:59:01 -0000
>@@ -1593,19 +1593,30 @@ NS_IMETHODIMP nsXFormsModelElement::GetT
>     nsCOMPtr<nsISVSchema> schema;
>     schemaColl->GetSchema(schemaTypeNamespace, getter_AddRefs(schema));
>     // if no schema found, then we will only handle built-in types.
>     if (schema)
>       validator.LoadSchema(schema);
>   }
> 
>   if (validator.GetType(schemaTypeName, schemaTypeNamespace, aType))
>-    rv = NS_OK;
>-  else
>+    return NS_OK;
>+  else {

you don't need this 'else'

>+    if ( schemaTypeNamespace.IsVoid() ) {
>+      const PRUnichar *strings[] = { schemaTypeName.get() };
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("typeNotFoundError"),
>+                                 strings, 1, mElement, nsnull);
>+    }
>+    else {
>+      const PRUnichar *strings[] = { schemaTypeName.get(), schemaTypeNamespace.get() };
>+      nsXFormsUtils::ReportError(NS_LITERAL_STRING("typeNotFoundInNamespaceError"),
>+                                 strings, 2, mElement, nsnull);
>+    }
>     rv = NS_ERROR_FAILURE;

just return NS_ERROR_FAILURE

Comment 35

[Dexter]

Attachment: Proposed patch #6

Changes done!

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 345090 [details] [diff] [review]
Proposed patch #6


>-  return rv;
>+    return NS_OK;
>+  
>+  if ( schemaTypeNamespace.IsVoid() ) {

nit: remove extra whitespaces, should be if (schemaTypeNamespace.IsVoid()) {

>+    const PRUnichar *strings[] = { schemaTypeName.get() };
>+    nsXFormsUtils::ReportError(NS_LITERAL_STRING("typeNotFoundError"),
>+                               strings, 1, mElement, nsnull);
>+  }
>+  else {

nit: should be } else {

Comment 37

[alexander :surkov (:asurkov)]

Comment on attachment 345090 [details] [diff] [review]
Proposed patch #6

r=me, don't put new patch, I'll fix nits before checkin

Comment 38

[Dexter]

okay, thanks for the reviews! :)

Comment 39

[alexander :surkov (:asurkov)]

np, yw, thank you for the patience.

Checked in on 1.9.0

Checking in extensions/xforms/nsXFormsModelElement.cpp;
/cvsroot/mozilla/extensions/xforms/nsXFormsModelElement.cpp,v  <--  nsXFormsModelElement.cpp
new revision: 1.165; previous revision: 1.164
done
Checking in extensions/xforms/resources/locale/en-US/xforms.properties;
/cvsroot/mozilla/extensions/xforms/resources/locale/en-US/xforms.properties,v  <--  xforms.properties
new revision: 1.37; previous revision: 1.36
done