Closed Bug 243904 Opened 17 years ago Closed 16 years ago

SOAP code problem with null, unpositioned array item

Categories

(Core Graveyard :: Web Services, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: chantepie, Assigned: chantepie)

References

Details

Attachments

(17 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

Even if in the SOAP response from Webservice there is the attribute 'nil="true"'
for a param, this param will be an empty string rather than a null one in when
trying to use it in javascript code.

Reproducible: Always
Steps to Reproduce:
1. Call a SOAP operation that returns an object 'defaultLocale' with 'variant'
property null.
2. In javascript test whether '(defaultLocale.variant == null)' is true
Actual Results:  
defaultLocale.variant = ""

Expected Results:  
defaultLocale.variant = null
Attached patch untested patch for consideration (obsolete) — Splinter Review
Attachment #148739 - Flags: review?(keeda)
Comment on attachment 148739 [details] [diff] [review]
untested patch for consideration

It might take me upto a week to get to around to reviewing this.
Patch doesn't seems to work with firefox 0.8+ debug build
Comment on attachment 148739 [details] [diff] [review]
untested patch for consideration

Cancelling review request till we figure out why this doesn't work.
Attachment #148739 - Flags: review?(keeda)
Attached file testcase (obsolete) —
Please read included 'README' file

*** This bug has been marked as a duplicate of 241157 ***
Status: UNCONFIRMED → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Still have some case where SOAP null values aren't supported, see following
testcase
Attached file testcase (20040705) (obsolete) —
To give it a try please first add 213.41.158.81 for ipage-test.corsaire.fr in
your host file.
Attachment #151089 - Attachment is obsolete: true
Stacktrace for the NS_ERROR_CANNOT_CONVERT_DATA when null property should be string

#0  nsDebugImpl::Break(char const*, int) (this=0x1116fd0, aFile=0x1c9a27c
"nsVariant.cpp", aLine=804) at nsDebugImpl.cpp:283
#1  0x01c598c4 in nsDebug::Break(char const*, int) (aFile=0x1c9a27c
"nsVariant.cpp", aLine=804) at nsDebug.cpp:91
#2  0x01bc6c2c in ToString(nsDiscriminatedUnion const&, nsACString&)
(data=@0x10671d0c, outString=@0xbfffc890) at nsVariant.cpp:804
#3  0x01bc709c in nsVariant::ConvertToAString(nsDiscriminatedUnion const&,
nsAString&) (data=@0x10671d0c, _retval=@0xbfffcf80) at nsVariant.cpp:889
#4  0x01bca670 in nsVariant::GetAsAString(nsAString&) (this=0x10671d00,
_retval=@0xbfffcf80) at nsVariant.cpp:1805
#5  0x1e3e0048 in WSPProxy::VariantToValue(unsigned char, void*,
nsIInterfaceInfo*, nsIVariant*) (aTypeTag=15 '\017', aValue=0xbfffcf80,
aInterfaceInfo=0x0, aProperty=0x10671d00) at wspproxy.cpp:998
#6  0x1e3e92fc in WSPPropertyBagWrapper::CallMethod(unsigned short,
nsXPTMethodInfo const*, nsXPTCMiniVariant*) (this=0x10808f10, methodIndex=5,
info=0x2b84840, params=0xbfffcc00) at wsppropertybagwrapper.cpp:167
#7  0x01c494f0 in PrepareAndDispatch (self=0x10808f10, methodIndex=5,
args=0xbfffcdc8, gprData=0xbfffcd0c, fprData=0xbfffcd28) at
xptcstubs_ppc_rhapsody.cpp:234
#8  0x01c4a058 in SharedStub () at nsTObsoleteAStringThunk.cpp:51
#9  0x01c49818 in _XPTC_InvokeByIndex () at nsTObsoleteAStringThunk.cpp:51
#10 0x01c48b50 in XPTC_InvokeByIndex (that=0x10808f10, methodIndex=5,
paramCount=1, params=0xbfffcef0) at xptcinvoke_ppc_rhapsody.cpp:144
#11 0x030483fc in XPCWrappedNative::CallMethod(XPCCallContext&,
XPCWrappedNative::CallMode) (ccx=@0xbfffd1e0, mode=CALL_GETTER) at
xpcwrappednative.cpp:2027
Attached patch Would this fix it? (obsolete) — Splinter Review
seems reasonable. this should probably be testable from xpcshell...
(In reply to comment #10)
> Created an attachment (id=152628)
> Would this fix it?
> 

no :-)
Patch also fix bug for one dimension array from apache SOAP that are unnumbered
(no explicit "position" attribute).
Attachment #152328 - Attachment is obsolete: true
*** Bug 248923 has been marked as a duplicate of this bug. ***
Comment on attachment 152690 [details] [diff] [review]
Patch supporting xsi:nil for apache SOAP and working for null string

Also fix unnumbered one dimension array (item without "position" attribute)
Attachment #152690 - Flags: review?(jst)
Whiteboard: needed-aviary1.0?
wait'll we have reviews on this before we look at putting it on the branch.
Whiteboard: needed-aviary1.0?
Summary: Webservice doesn't support null SOAP param → SOAP code problem with null, inheritance, unpositioned array item
Fixes null value problem testing apache SOAP xsi:nil and supportings soap null
string as empty string. Fixes problem about unpositionned items of one
dimension array so that it works. Temporarily fix problem about inheritance
getting explicit type from soap response DOM element (xsi:type), inheritance
support should be enhanced. Maid from AVIARY, I'll submit a patch for trunk as
soon as I can.
Attachment #152690 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #152690 - Flags: review?(jst)
Attached patch Patch for trunk (obsolete) — Splinter Review
Attachment #156386 - Flags: review?(jst)
OS: Windows 2000 → All
Hardware: PC → All
Attachment #154407 - Attachment is obsolete: true
Attachment #156386 - Attachment is obsolete: true
Attachment #156386 - Flags: review?(jst)
Changes nsVariant::ToString to supports null string.
Assignee: web-services → chantepie
Status: NEW → ASSIGNED
Attachment #156586 - Flags: review?(jst)
Comment on attachment 156586 [details] [diff] [review]
Patch including changes for nsVariant::ToString [trunk]

- In HandleNull():

+  if (aNullAttr.Equals(gSOAPStrings->kTrue)
+      || aNullAttr.Equals(gSOAPStrings->kTrueA)) {
...
+    if (typeName.Equals(NS_LITERAL_STRING("string")) ||
+	 typeName.Equals(NS_LITERAL_STRING("normalizedString"))) {

Inconsistent operator placement, pick one (I'd prefer operators at the end of
the line in stead of at the beginning).

+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(strVar);
+	 
+	 *_retval = variant;
+	 
+	 NS_ADDREF(*_retval);
+	 
+	 return NS_OK;

How about replacing the above with the single line:

+	 return CallQueryInterface(strVar, _retval);

... and

+      }
+      else {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }

remove the else-after-return. How about:

+      nsCOMPtr<nsIWritableVariant>
strVar(do_CreateInstance("@mozilla.org/variant;1"));
+      
+      if (!strVar) {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }
+
+      return CallQueryInterface(strVar, _retval);

... and then...

+    }
+    
+    else {

One more else-after-return.

+      if (nullVariant) {
+	 nullVariant->SetAsISupports(nsnull);
+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(nullVariant);
+	 *_retval = variant;
+	 
+	 NS_ADDREF(*_retval);
+	 
+	 return NS_OK;

Same thing:

+	 return CallQueryInterface(varian, _retval);

...
+      }
+      
+      return NS_ERROR_OUT_OF_MEMORY;
+    }

... and flip this around too to return the error in the exception case, and let
the normal flow go to the end of this scope (like I suggested above).

+  NS_ERROR("How have you get here ?");

"How did you get here?"

- In GetExplicitType():

...
+    rc = schemaLoader->GetType(name, ns, getter_AddRefs(type));
+
+    if (NS_SUCCEEDED(rc) && type) {
+      *_retval = type;
+
+      NS_ADDREF(*_retval);
+
+      return NS_OK;
+    }
+
+    else {
+      *_retval = nsnull;
+
+      return rc;
+    }

else-after-return, and this could be simplified to:

+    NS_IF_ADDREF(*_retval = type);
+
+    return rc;

Note the _IF_ in NS_IF_ADDREF().

+  NS_ERROR("::GetExplicitType: Wow how do you get here");

See above.

- In nsVariant.cpp

+	 outString.Assign("");
+	 outString.Truncate();

No need to assign an empty string into a string just before truncating it.

sr=jst with those changes, please fix that, and request review from
keeda@hotpop.com
Attachment #156586 - Flags: review?(jst) → superreview+
Will all loaders past and present be required to provide a schema collection?
Just wondering if interface inheritance is the right solution. Admittedly I'm
not familiar with the WSDL object structure, so take that into consideration.
I'm more trying to gain knowledge than suggest an alternative. Maybe the naming
is messing me up, as I wouldn't expect a loader to be a collection and QI to the
collection, I would have expected to ask it via a method for a collection of things.
Attached patch Fixed trunk patch (obsolete) — Splinter Review
Attachment #157000 - Attachment is obsolete: true
Attached patch Fixed trunk patch (obsolete) — Splinter Review
Attachment #157001 - Flags: review?(keeda)
Attachment #157001 - Attachment is obsolete: true
Attachment #157001 - Flags: review?(keeda)
Attachment #157967 - Flags: review?(keeda)
Attached patch Updated patch for trunk (obsolete) — Splinter Review
Attachment #157967 - Attachment is obsolete: true
Attachment #172048 - Flags: review?(keeda)
Attachment #157967 - Flags: review?(keeda)
Comment on attachment 172048 [details] [diff] [review]
Updated patch for trunk

With modifications from patch of bug #254897
*** Bug 254897 has been marked as a duplicate of this bug. ***
Comment on attachment 172048 [details] [diff] [review]
Updated patch for trunk

+#include "nsIWSPInterfaceInfoService.h"
Is not used for this patch
Attachment #172048 - Flags: review?(keeda) → review?(peterv)
Attachment #172048 - Flags: review?(peterv)
Attachment #172048 - Attachment is obsolete: true
Attachment #172506 - Flags: superreview?(jst)
Comment on attachment 172506 [details] [diff] [review]
Patch without unneeded include [trunk]

- In HandleNull():

+      nsAutoString strVal;
+      strVal.Truncate();

No need to truncate that string, it's already empty.

+      nsCOMPtr<nsIWritableVariant>
strVar(do_CreateInstance("@mozilla.org/variant;1"));
+     
+      if (strVar) {
+	 strVar->SetAsAString(strVal);
+
+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(strVar);
+
+	 *aResult = variant;
+
+	 NS_ADDREF(*aResult);
+
+	 return NS_OK;
+      }
+      else {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }

else-after-return. And how about flipping that around to returning early if
we're out of memory... i.e.:

+      nsCOMPtr<nsIWritableVariant>
strVar(do_CreateInstance("@mozilla.org/variant;1"));
+     
+      if (!strVar) {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }
+
+      strVar->SetAsAString(strVal);
...

+      nsCOMPtr<nsIWritableVariant>
nullVariant(do_CreateInstance("@mozilla.org/variant;1"));
+     
+      if (nullVariant) {
+	 nullVariant->SetAsISupports(nsnull);
+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(nullVariant);
+	 *aResult = variant;
+
+	 NS_ADDREF(*aResult);
+
+	 return NS_OK;
+      }
+     
+      return NS_ERROR_OUT_OF_MEMORY;

Same thing, return NS_ERROR_OUT_OF_MEMORY if (!nullVariant).

sr=jst wiht that changed.
Attachment #172506 - Flags: superreview?(jst) → superreview+
Attached patch Changed patch (obsolete) — Splinter Review
With changes from jst comment
If is ok with jst changes could anyone commit that
Blocks: 255624
If is ok with jst changes could anyone commit that
Comment on attachment 174744 [details] [diff] [review]
Changed patch

This still needs a review, requsting from Harshal...
Attachment #174744 - Flags: review?(keeda)
Comment on attachment 174744 [details] [diff] [review]
Changed patch

>Index: extensions/webservices/public/nsISchemaLoader.idl

>-[scriptable, uuid(9B2F0B4A-8F00-4a78-961A-7E84ED49B0B6)]
>-interface nsISchemaLoader : nsISupports {
>+[scriptable, uuid(77061d1d-e191-11d8-a3cc-000393b6661a)]
>+interface nsISchemaLoader : nsISchemaCollection {
>   nsISchema load(in AString schemaURI);
>   void loadAsync(in AString schemaURI, 
>                  in nsISchemaLoadListener listener);
>   nsISchema processSchemaElement(in nsIDOMElement element,
>                                  in nsIWebServiceErrorHandler aErrorHandler);
> };
> 

I have reservations about this change. We already have
a somewhat broken object model in the schema code. I
don't think that making nsISchemaLoader inherit from
nsISchemaCollection makes things any better. Some of the
functions that nsSchemaLoader performs are quite orthogonal
to each other. If someone wanted to clean that up in the
future, this change would make things more difficult.
Also, I am told there are also external users of this interface
in the wild, so we have to be careful when extending its
functionality.

Besides, as far as I can see, the only reason for this
change is to save one QI in the SOAP code. I don't think
that this is enough justifaction. Just do the extra QI to
nsISchemaCollection where you need it. (Or am I missing
something here ?)


>Index: extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp

>+static nsresult HandleNull(nsISOAPEncoding* aEncoding,
>+                           nsIDOMElement* aSource,
>+                           nsISchemaType* aSchemaType,
>+                           nsISOAPAttachments* aAttachments,
>+                           nsAutoString aNullAttr,
>+                           nsIVariant** aResult)
>+{

[snip]

>+   
>+    if (aSchemaType &&
>+        (typeName.Equals(NS_LITERAL_STRING("string")) ||
>+         typeName.Equals(NS_LITERAL_STRING("normalizedString")))) {
>+     
>+      nsAutoString strVal;
>+      strVal.SetIsVoid(true);
>+     
>+      nsCOMPtr<nsIWritableVariant> strVar(do_CreateInstance("@mozilla.org/variant;1"));
>+     
>+      if (!strVar) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+       
>+      strVar->SetAsAString(strVal);
>+
>+      nsCOMPtr<nsIVariant> variant = do_QueryInterface(strVar);
>+      
>+      NS_ADDREF(*aResult = variant);
>+
>+      return NS_OK;
>+    } else {
>+      nsCOMPtr<nsIWritableVariant> nullVariant(do_CreateInstance("@mozilla.org/variant;1"));
>+     
>+      if (!nullVariant) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+       
>+      nullVariant->SetAsISupports(nsnull);
>+      nsCOMPtr<nsIVariant> variant = do_QueryInterface(nullVariant);
>+      
>+      NS_ADDREF(*aResult = variant);
>+      
>+      return NS_OK;


Move the construction of the variant out of the if so that you
only do it once. Also since nsIVariant inherits from nsIWritableVariant
I suspect that you dont need to QI it when you assign it to an nsIVariant.

So, basically something like 

      nsCOMPtr<nsIWritableVariant>
var(do_CreateInstance("@mozilla.org/variant;1"));
      if (!var) {
	return NS_ERROR_OUT_OF_MEMORY;
      }
      if (stringtype) {
	do_string_stuff;
      } else {
	interface_stuff;
      }
      NS_ADDREF(*aResult = variant);

This will be somewhat smaller code.

[snip]

>+static nsresult DecodeStructParticle(nsISOAPEncoding* aEncoding, 
>+                                     nsIDOMElement* aElement, 
>+                                     nsISchemaParticle* aParticle, 
>+                                     nsISOAPAttachments * aAttachments, 
>+                                     nsISOAPPropertyBagMutator* aDestination,
>+                                     nsIDOMElement** aResult)
> {

[snip]


>           nsCOMPtr<nsIDOMElement> next = aElement;
>           while (particleCount > 0) {
>             for (i = 0; i < particleCount; i++) {
>-              child = dont_AddRef(NS_STATIC_CAST
>-                    (nsISchemaParticle*, all->ElementAt(i)));
>+              nsAutoString name;
>+              rc = next->GetTagName(name);
>+              NS_ENSURE_SUCCESS(rc, rc);
>+
>+              // Get schema particle according SOAP element
>+              groupParticles.Get(name, getter_AddRefs(child));
>+              
>+              if (NS_FAILED(rc) || !child) {
>+                nsAutoString msg(NS_LITERAL_STRING("::DecodeStructParticle: "));
>+                msg.AppendLiteral("Cannot find schema particle for \"");
>+                msg.Append(name);
>+                msg.AppendLiteral("\"");
>+                NS_ERROR(NS_ConvertUTF16toUTF8(msg).get());

Return this error. Or if you really want to ignore it, then put it in
a #ifdef DEBUG. There is no point in paying for the extra codesize in
release builds.


r=me if you revert the schema interface change and fix above nits.

The changes in the SOAP decoder are somewhat scary. Unfortunatly, I don't
even have a build right now and am unable to test anything. You have been
carrying this patch for a while, so I am going to assume that it must
have been tested, and my r= above is based on this. Please make
sure that you do sufficient testing with different some live servers 
before this gets checked in.

And finally, thanks for your patience on this bug .... I'm sorry for the
long wait on reviews for your patches.
Attachment #174744 - Flags: review?(keeda) → review+
Attachment #152329 - Attachment is obsolete: true
Summary: SOAP code problem with null, inheritance, unpositioned array item → SOAP code problem with null, unpositioned array item
Attachment #172506 - Attachment is obsolete: true
Attachment #148739 - Attachment is obsolete: true
Attachment #152628 - Attachment is obsolete: true
Attachment #156586 - Attachment is obsolete: true
Attachment #174744 - Attachment is obsolete: true
Attachment #177257 - Flags: review?(keeda)
Attachment #177257 - Flags: review?(keeda) → review?(doronr)
I still see schemaLoader changes in the patch.
Attached patch Without schemaloader change (obsolete) — Splinter Review
Attachment #177257 - Attachment is obsolete: true
Attachment #181912 - Flags: review?(doronr)
Attachment #177257 - Flags: review?(doronr)
Comment on attachment 181912 [details] [diff] [review]
Without schemaloader change

I tested a bit and doesn't seem to break anything.
Attachment #181912 - Flags: review?(doronr) → review+
Attachment #181912 - Flags: approval-aviary1.1a?
Comment on attachment 181912 [details] [diff] [review]
Without schemaloader change

a=asa
Attachment #181912 - Flags: approval-aviary1.1a? → approval-aviary1.1a+
Comment on attachment 181912 [details] [diff] [review]
Without schemaloader change

2005-05-02 12:42:
mozilla/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp	1.93
mozilla/extensions/webservices/soap/src/nsSOAPUtils.cpp 	1.39
mozilla/extensions/webservices/soap/src/nsSOAPUtils.h	1.26
mozilla/xpcom/ds/nsVariant.cpp	1.27
Attachment #181912 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 17 years ago16 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.