263384 - add method to get base schema type

Status: RESOLVED WONTFIX

(Core Graveyard :: XForms, defect)

Description

[Darin Fisher]

add method to get base schema type.

for xforms submission, we need to be able to test if a schema type derives from
a base type, such as xsd:base64binary.  it might be nice if nsISchemaCollection
provided a method to do this for us.  something like:

  nsISchemaBuiltinType getBaseType(in AString name, in AString namespace);

Comment 1

[Doron Rosenberg (IBM)]

Attachment: first try

Comment 2

[Doron Rosenberg (IBM)]

Can the XPCOM experts look at the patch?

nsSchemaLoader::GetBaseType calls nsBuiltinSchemaCollection::GetBaseType if its
a schema namespace, which returns a nsISchemaBuiltinType and addrefs that
comptr.  nsSchemaLoader::GetBaseType then returns that nsISchemaBuiltinType
comptr as its return.  Should I addref that in nsSchemaLoader::GetBaseType as well?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

+  NS_ENSURE_ARG_POINTER(_retval);

that is not normally needed... also, shouldn't the patch contain a diff to an
idl file as well?

+  if (schemaType)
+    schemaBuiltinType = do_QueryInterface(schemaType);
+
+  *_retval = schemaBuiltinType;
+  NS_ADDREF(*_retval);
+
+  return rv;

maybe:
if (schemaType)
  return CallQueryInterface(schemaType, _retval);

>nsSchemaLoader::GetBaseType then returns that nsISchemaBuiltinType
>comptr as its return.  Should I addref that in nsSchemaLoader::GetBaseType as
>well?

yes (like it's done in this patch), because the comptr will Release() the object
when it goes out of scope.

Comment 4

[Doron Rosenberg (IBM)]

Attachment: With biesi's comments

I'm currently only supporting builtin types and simpletype restrictions.

Comment 5

[Doron Rosenberg (IBM)]

Attachment: change uuid, null check when addreffing.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 162018 [details] [diff] [review]
change uuid, null check when addreffing.

I'd really prefer it if someone would give r= who knows this code...

this patch again misses the IDL changes ;)

in nsBuiltinSchemaCollection::GetBaseType I'd move the rv decl to the first use

Comment 7

[Doron Rosenberg (IBM)]

Comment on attachment 162018 [details] [diff] [review]
change uuid, null check when addreffing.

darn it, I was sure I had added the IDL changes.  Consider the IDL with the
method added and a new UUID, I'm at home and can't create a new diff :)

Comment 8

[Doron Rosenberg (IBM)]

Attachment: with IDL changes.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 162078 [details] [diff] [review]
with IDL changes.

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

>+NS_IMETHODIMP
>+nsBuiltinSchemaCollection::GetBaseType(const nsAString& aName, 
>+                                       const nsAString & aNamespace, 
>+                                       nsISchemaBuiltinType **_retval)

Be consistent. &aName, &aNamespace and please make _retval aResult.

>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  rv = GetBuiltinType(aName, aNamespace, getter_AddRefs(schemaType));
>+  if (NS_FAILED(rv))
>+    return rv;

No need to return here if you return later.

>+  
>+  if (schemaType)
>+    return CallQueryInterface(schemaType, _retval);
>+
>+  return rv;

return schemaType ? CallQueryInterface(schemaType, aResult) : rv;

>+}
>+
> nsresult
> nsBuiltinSchemaCollection::GetBuiltinType(const nsAString& aName,
>                                           const nsAString& aNamespace,
>@@ -609,6 +627,75 @@
>   return NS_OK;
> }
> 
>+NS_IMETHODIMP 
>+nsSchemaLoader::GetBaseType(const nsAString & aName, 
>+                            const nsAString & aNamespace, 
>+                            nsISchemaBuiltinType **_retval)

Be consistent. &aName, &aNamespace and please make _retval aResult.

>+{
>+  nsresult rv = NS_OK;
>+
>+  nsCOMPtr<nsISchemaBuiltinType> schemaBuiltinType;

I don't think you need this.

>+
>+  if (IsSchemaNamespace(aNamespace)) {
>+    // if its a schema namespace, check if its a built in type
>+    rv = mBuiltinCollection->GetBaseType(aName, aNamespace, _retval);

Just return here.

>+  } else {

No need for the else then.

>+    nsCOMPtr<nsISchemaType> schemaType;
>+
>+    rv = GetType(aName, aNamespace, getter_AddRefs(schemaType));
>+    if (NS_FAILED(rv))
>+      return rv;
>+      
>+    // figure out the base type
>+    PRUint16 typevalue;
>+
>+    rv = schemaType->GetSchemaType(&typevalue);
>+    if (NS_FAILED(rv))
>+      return rv;
>+
>+    switch (typevalue) {
>+      case nsISchemaType::SCHEMA_TYPE_SIMPLE: {

No need for a switch if there's only two cases, just use if/else.

>+
>+        nsCOMPtr<nsISchemaSimpleType> simpleType = do_QueryInterface(schemaType);
>+
>+        if (simpleType) {
>+          PRUint16 simpleTypeValue;
>+
>+          rv = simpleType->GetSimpleType(&simpleTypeValue);
>+          if (NS_FAILED(rv))
>+            return rv;
>+
>+          switch (simpleTypeValue) {
>+            case nsISchemaSimpleType::SIMPLE_TYPE_RESTRICTION: {

No need for a switch if there's only one case, just use an if.

>+              nsCOMPtr<nsISchemaRestrictionType> restrictionType = 
>+                  do_QueryInterface(simpleType);
>+              nsCOMPtr<nsISchemaSimpleType> simpleBaseType;
>+
>+              rv = restrictionType->GetBaseType(getter_AddRefs(simpleBaseType));
>+              if (NS_FAILED(rv))
>+                return rv;
>+
>+              schemaBuiltinType = do_QueryInterface(simpleBaseType);

Just CallQueryInterface(simpleBaseType, aResult).

>+              break;
>+            } 
>+          }
>+        }
>+
>+        break;
>+      }
>+
>+      default: {
>+        rv = NS_ERROR_NOT_IMPLEMENTED;

For complex types with DERIVATION_RESTRICTION_SIMPLE or
DERIVATION_EXTENSION_SIMPLE it should be fairly easy (GetSimpleBaseType). For
SELF_CONTAINED you probably want the anyType. For the others we'll probably
have to recurse.

Don't adding trailing whitespace.
I'd use NS_ENSURE_SUCCESS(rv, rv); instead of if (NS_FAILED(rv)) return rv;

Comment 10

[Doron Rosenberg (IBM)]

Attachment: With review comments, also support simple type lists.

Comment 11

[Doron Rosenberg (IBM)]

I am not sure this needs to handle complex types, but I need the current code
for schema validation to work :)

Comment 12

[Doron Rosenberg (IBM)]

Attachment: propogate CallQueryInterface rv correctly, support some complex types.

Comment 13

[Darin Fisher]

Comment on attachment 168780 [details] [diff] [review]
propogate CallQueryInterface rv correctly, support some complex types.

some nit picks:


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

>+NS_IMETHODIMP
>+nsBuiltinSchemaCollection::GetBaseType(const nsAString& aName, 
>+                                       const nsAString & aNamespace, 
>+                                       nsISchemaBuiltinType **aResult)
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  rv = GetBuiltinType(aName, aNamespace, getter_AddRefs(schemaType));
>+
>+  return schemaType ? CallQueryInterface(schemaType, aResult) : rv;

It's more common to write this like so:

  nsCOMPtr<nsISchemaType> schemaType;
  rv = GetBuiltinType(aName, aNamespace, getter_AddRefs(schemaType));
  if (NS_FAILED(rv))
    return rv;

  return CallQueryInterface(schemaType, aResult);

The argument being that the out param will never be null if the function
returns a success code, and moreover that the out param should be ignored
if the function returns a failure code.


>+  nsCOMPtr<nsISchemaType> schemaType;
>+  rv = GetType(aName, aNamespace, getter_AddRefs(schemaType));
>+  if (NS_FAILED(rv) || !schemaType)
>+    return rv;

perhaps it would be better to check only |rv| or only |schemaType|.
again, it should never be the case that NS_SUCCEEDED(rv) && !schemaType.

>+  if (typevalue == nsISchemaType::SCHEMA_TYPE_SIMPLE) {
>+    nsCOMPtr<nsISchemaSimpleType> simpleType = do_QueryInterface(schemaType);
>+
>+    if (simpleType) {
>+      rv = GetBuiltinTypeFromSimpleType(simpleType, aResult);
>+    }

else, what happens?  should you add an NS_ASSERTION here in case
simpleType is somehow null?


>+  } else {
>+    nsCOMPtr<nsISchemaComplexType> complexType = do_QueryInterface(schemaType);
>+
>+    if (complexType) {

ditto for this check.  seems like you'd want to scream loudly if the QI
ever failed.


>+      } else {
>+        rv = NS_ERROR_NOT_IMPLEMENTED;

Can you add a XXX comment here explaining what needs to be done in this
case.


>+    nsCOMPtr<nsISchemaRestrictionType> restrictionType =
>+        do_QueryInterface(aSimpleType);
>+    nsCOMPtr<nsISchemaSimpleType> simpleBaseType;
>+
>+    rv = restrictionType->GetBaseType(getter_AddRefs(simpleBaseType));

up above, you null checked the return values of QI.  here you do not.
perhaps you should not in the above code as well?  either the QI is
assumed to always work (and assert that it should) or add runtime
checks.


>+    nsCOMPtr<nsISchemaListType> listType =
>+        do_QueryInterface(aSimpleType);
>+    nsCOMPtr<nsISchemaSimpleType> simpleBaseType;
>+
>+    rv = listType->GetListType(getter_AddRefs(simpleBaseType));

same here.


>+    rv = CallQueryInterface(simpleBaseType, aBuiltinType);

CallQueryInterface does not null check the first parameter, so you
are assuming that it will never be null.  is that a valid assumption?

Comment 14

[Doron Rosenberg (IBM)]

Should I assert or just return NS_ERROR_UNEXPECTED?

>>+      } else {
>>+        rv = NS_ERROR_NOT_IMPLEMENTED;
>
>Can you add a XXX comment here explaining what needs to be done in this
>case.

I'm not sure if we want this method to work on complex types.  It might make
sense for DERIVATION_RESTRICTION_SIMPLE or DERIVATION_EXTENSION_SIMPLE, but
perhaps it makes more sense to return anyType for all complex ones?

Comment 15

[Darin Fisher]

(In reply to comment #14)
> Should I assert or just return NS_ERROR_UNEXPECTED?

Up to you.  You can use NS_ASSERTION (and then crash afterwards) if you know you
are dealing with your own code and know that the bad condition is just really
really stupid and likely to mean that other things are entirely wrong such that
crashing is no worse than trying to continue.  But, when in doubt, go ahead and
code a |return rv| statement.  Add a NS_ERROR or NS_WARNING depending on how bad
the situation is.  You can use NS_ENSURE_TRUE (or one of the many variants) if
you want to output a NS_WARNING and |return rv| all in one quick line of code.


> >>+      } else {
> >>+        rv = NS_ERROR_NOT_IMPLEMENTED;

NS_ERROR_NOT_IMPLEMENTED implies more code to come once we figure out how
to implement it.  If that is so, then it means that a comment explaining
what code should be here is always a good idea.  Otherwise, I'd add a 
NS_NOTREACHED assertion so that people know right away that they are misusing
this code.


> I'm not sure if we want this method to work on complex types.  It might make
> sense for DERIVATION_RESTRICTION_SIMPLE or DERIVATION_EXTENSION_SIMPLE, but
> perhaps it makes more sense to return anyType for all complex ones?

I don't know XML schema well enough to say.

Comment 16

[Doron Rosenberg (IBM)]

>up above, you null checked the return values of QI.  here you do not.
>perhaps you should not in the above code as well?  either the QI is
>assumed to always work (and assert that it should) or add runtime
>checks.

I'll add runtime checks, but its up to peterv really, he knows the code better.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 168780 [details] [diff] [review]
propogate CallQueryInterface rv correctly, support some complex types.

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

> +nsBuiltinSchemaCollection::GetBaseType(const nsAString& aName, 
> +                                       const nsAString & aNamespace, 
> +                                       nsISchemaBuiltinType **aResult)
> +{
> +  nsresult rv;
> +
> +  nsCOMPtr<nsISchemaType> schemaType;
> +  rv = GetBuiltinType(aName, aNamespace, getter_AddRefs(schemaType));

I'm fine with darin's suggestion. Small nit: I personally prefer variables to
be declared where they're first used, so I'd do:

  nsresult rv = GetBuiltinType(aName, aNamespace, getter_AddRefs(schemaType));

> +  } else {
> +    nsCOMPtr<nsISchemaComplexType> complexType = do_QueryInterface(schemaType);
> +
> +    if (complexType) {
> +      // check the deriviation type
> +      PRUint16 derivation;
> +      rv = complexType->GetDerivation(&derivation);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      if ((derivation == nsISchemaComplexType::DERIVATION_RESTRICTION_SIMPLE) ||
> +          (derivation == nsISchemaComplexType::DERIVATION_EXTENSION_SIMPLE)) {

No need for the inner parentheses.

> +        nsCOMPtr<nsISchemaSimpleType> simpleType;
> +        rv = complexType->GetSimpleBaseType(getter_AddRefs(simpleType));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = GetBuiltinTypeFromSimpleType(simpleType, aResult);
> +      } else {
> +        rv = NS_ERROR_NOT_IMPLEMENTED;

There's more to be done here, so NS_ERROR_NOT_IMPLEMENTED is correct imo.

I'm fine with dropping some of the null-checks. The QI's shouldn't fail once
you've checked the types of the objects.

Can you make a new patch, get darin to r= it and then I'll sr.

Comment 18

[Doron Rosenberg (IBM)]

peterv: what do you think about doing this for all other complex types:

  *aResult = new nsSchemaBuiltinType(nsISchemaBuiltinType::BUILTIN_TYPE_ANYTYPE);

Seems to me that in this context, it would be a valid thing (I only use it for
simple types anyways).

Comment 19

[Doron Rosenberg (IBM)]

Attachment: new attempt

Comment 20

[Darin Fisher]

Comment on attachment 168893 [details] [diff] [review]
new attempt

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

>+    } else {
>+      rv = NS_ERROR_NOT_IMPLEMENTED;

I would still like to see a XXX comment accompanying this error
condition.  A NS_NOTREACHED or NS_WARNING would be nice too.

Comment 21

[Doron Rosenberg (IBM)]

ok, make it:

// XXX unclear what we should do for other complex types.  Return anyType,
// an NS_ERROR?
NS_NOTREACHED("Support for complextypes (other than simpletype
restriction/extension) is not yet implemented");
rv = NS_ERROR_NOT_IMPLEMENTED;

Comment 22

[Darin Fisher]

Comment on attachment 168893 [details] [diff] [review]
new attempt

r=darin with that change

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 168893 [details] [diff] [review]
new attempt

I've re-read some parts of the schema spec, and I seem to have misremembered
some stuff. I'm not sure what the exact behaviour of the function is expected
to be, but I assume that it is 'get the base type of a specified schema type'
with base type being used as the schema spec defines it.

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

>+NS_IMETHODIMP
>+nsBuiltinSchemaCollection::GetBaseType(const nsAString& aName, 
>+                                       const nsAString & aNamespace, 
>+                                       nsISchemaBuiltinType **aResult)
>+{
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  nsresult rv = GetBuiltinType(aName, aNamespace, getter_AddRefs(schemaType));

This isn't quite right, there are built-in types that are derived by
restriction and thus have a base type.

>+NS_IMETHODIMP
>+nsSchemaLoader::GetBaseType(const nsAString & aName,
>+                            const nsAString & aNamespace,
>+                            nsISchemaBuiltinType **aResult)
>+{
>+  if (IsSchemaNamespace(aNamespace)) {
>+    // if its a schema namespace, check if its a built-in type

it's

>+    return mBuiltinCollection->GetBaseType(aName, aNamespace, aResult);
>+  }
>+
>+  nsCOMPtr<nsISchemaType> schemaType;
>+  nsresult rv = GetType(aName, aNamespace, getter_AddRefs(schemaType));
>+  if (!schemaType)
>+    return rv;

NS_ENSURE_SUCCESS(rv, rv);

>+    if (derivation == nsISchemaComplexType::DERIVATION_RESTRICTION_SIMPLE ||
>+        derivation == nsISchemaComplexType::DERIVATION_EXTENSION_SIMPLE) {

We probably don't need to handle DERIVATION_EXTENSION_SIMPLE.

>+nsresult 
>+nsSchemaLoader::GetBuiltinTypeFromSimpleType(nsISchemaSimpleType* aSimpleType,
>+  nsISchemaBuiltinType** aBuiltinType)

Line up the second argument

nsSchemaLoader::GetBuiltinTypeFromSimpleType(nsISchemaSimpleType* aSimpleType,
					     nsISchemaBuiltinType**
aBuiltinType)

>+  } else if (simpleTypeValue == nsISchemaSimpleType::SIMPLE_TYPE_LIST) {

Base types only apply to derivation by restriction. You need to return an error
for _UNION at least and probably for _LIST too.

Comment 24

[Doron Rosenberg (IBM)]

(In reply to comment #23)
> (From update of attachment 168893 [details] [diff] [review] [edit])
> I've re-read some parts of the schema spec, and I seem to have misremembered
> some stuff. I'm not sure what the exact behaviour of the function is expected
> to be, but I assume that it is 'get the base type of a specified schema type'
> with base type being used as the schema spec defines it.

So "nonNegativeInteger" would return integer?  That is not what I need really
then.  I was using this to get the simpletype a type restricts for example, so
the name I choose for the method is probably is probably misleading.  Validation
needs to figure if the simpletype foo is actually a integer, nonNegativeInteger,
etc.  Since I don't need it to do complex types, perhaps its better to put a
simplified version I need into the validation code then?

Comment 25

[Doron Rosenberg (IBM)]

this ended up not being needed it seems, so wontfix