arishanapalli / wsdl2objc

Automatically exported from code.google.com/p/wsdl2objc
MIT License
0 stars 0 forks source link

Code review request #134

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Purpose of code changes on this branch:
Merging the fork at https://github.com/pmilosev/wsdl2objc back to the main 
project.

Added functionalities:
- Support for advanced authentication
  - Self-signed certificate support
  - Client certificate authentication (2-way SSL)

- Support for XMLSecurity
  - The current implementation only covers one special case (XMLSigning on the body of the message), but can serve as a template for further security work.

- Few enhancements and bugfixes

When reviewing my code changes, please focus on:
Please check that I have not broken some of the existing functionalities. There 
was quite some work done on the main project since I have forked, so the 
merging had a lot of conflicts that I needed to resolve manually.

I also need a second opinion on:
- I have changed the binding delegate to be retained instead of assigned.
  There was some memory issue that was crashing the application from time to time and I needed to break the rule of not retaining the delegate. I don't know if this issue was fixed with the later development.

- I didn't know how to differ between the classes for the requests, and other 
complex types (e.g. responses) in the templates. So I have added the SOAPSigner 
in every complex type. The signer however is only needed for the requests.  

I have tested the code after the merge against my mock server, on both iPhone 
and Mac, but I guess it should be checked against few more :)

Ah, and sorry for using spaces instead of tabs for indentation :)
It is a corporate standard, and all of this work was done at my workplace. I 
noticed the difference only when I started merging.
There might be some code that I contributed earlier, for which the indentation 
was fixed, that is now again rewritten, but it was simply easier to merge 
everything than to search trough history what was already contributed.

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by pmilosev on 5 Mar 2011 at 12:36

GoogleCodeExporter commented 9 years ago

Original comment by pmilosev on 5 Mar 2011 at 12:40

GoogleCodeExporter commented 9 years ago
Next week I hope to write a short tutorial on how to use the new features.
Should I do this directly on the wiki, or on some other place (so that the wiki 
page is written only after the code is reviewed and fixed if anything) ?

Original comment by pmilosev on 5 Mar 2011 at 12:52

GoogleCodeExporter commented 9 years ago
Put it in a new wiki page, not in the main page.
Then once all is good, we can link to its tutorial.

Original comment by hasse...@gmail.com on 5 Mar 2011 at 8:36

GoogleCodeExporter commented 9 years ago
Regarding changing the binding delegate to be retained, I am strongly against 
that.
I think the problem that you had with the application crashing is that you (and 
that's the user's responsibility) must ensure that the delegate is still alive 
when the async binding request finishes.
So please revert that behavior, and ensure in your own user code that the 
delegate is always alive when the async request finishes.

I'll look into the SOAPSigner, but beyond that it's going to need a lot of 
testing and walking through every change to the current revision that I have 
little time for.

Original comment by hasse...@gmail.com on 5 Mar 2011 at 9:06

GoogleCodeExporter commented 9 years ago
Regarding the delegate; I'm not sure if you are right, but I'll check our code 
once I'm back to the office.

For the rest; It's higher priority to check the diff than to find a solution 
for the signer not to be included in every class.
If you invest any time please check the diff first. It clearly states what was 
removed and what was added so you don't really need to go trough every commit 
since the fork. Assume that everything you have commited is OK and just check 
if I have removed/changed somethink that I shouldn't.
It's doable in less than an hour; drop me a comment or send me a mail with your 
remarks so that we can clear things faster.

Original comment by pmilosev on 5 Mar 2011 at 9:32

GoogleCodeExporter commented 9 years ago
I published my comments on the code review diffs. Thanks.

Original comment by hasse...@gmail.com on 5 Mar 2011 at 1:43

GoogleCodeExporter commented 9 years ago
Thanks for the review, I assume all diffs that are not commented are OK.
For your remarks:
- I'll check if the delegate issue is on our side once I'm back to the office.
- You are right about the USDecimal type, it can be handled by NSDecimalNumber.
   This is leftover from some company standard. I'll fix that before merging to trunk.
- About the SOAPSigner; As I said, I'm not familiar with the templating thing. 
If you say that no request class can appear on the commented places I'll remove 
it. I only need the signer for request classes.

- If you have basic authentication, everything is simple again. Instead of as 
different properties you provide the username and password into a dictionary.
One thing that I might change is the handling of the default persistence flag.
If it is not provided now it equals NONE (since the dictionary will return nil) 
.
I will add a check first and make the default persistence to be SESSION, as it 
was earlier.

Thanks for your quick review.
I'll stop commenting now as I don't have access on a computer, and is quite 
hard to type on the phone's soft keyboard :)

I'll check with you once again after I prepare the code for merging into trunk.

Original comment by pmilosev on 5 Mar 2011 at 7:58

GoogleCodeExporter commented 9 years ago
Good point regarding the properties. I suggest that you make the authentication 
dict property be a copy, and generate the setter so that if the persistence 
flag isn't passed in, you add it to the dict ivar.

For SOAPSigner, yes I am quite sure about the #ifdef AttributedString, I wrote 
that part. It only triggers on the special case of a string with attributes, 
which is certainly not a request.

Original comment by hasse...@gmail.com on 6 Mar 2011 at 7:44

GoogleCodeExporter commented 9 years ago
I have made the changes suggested with your code review (and few more).
Please confirm if the code is now OK to be merged with trunk.

In meantime I will write a short how-to for the new features.

regards

Original comment by pmilosev on 9 Mar 2011 at 10:44

GoogleCodeExporter commented 9 years ago
Looks good. Go for it.

Original comment by hasse...@gmail.com on 9 Mar 2011 at 2:39

GoogleCodeExporter commented 9 years ago
Done.

I have updated the UserInstructions to follow latest changes, and stared a new 
page for the advanced topics.
Merged the NCA branch into TRUNK.
Returned the delegate to be assigned (missed out previously into the branch).

Now I only need to finish the advanced topics page, which should happen in the 
next few days.

Thanks for the support

Original comment by pmilosev on 9 Mar 2011 at 6:42

GoogleCodeExporter commented 9 years ago

Original comment by pmilosev on 9 Mar 2011 at 6:42

GoogleCodeExporter commented 9 years ago
Why don't you compile a 0.8-pre1 or something, so people can start using it? (I 
assume you can make a new build, if not I'll make it)
Thanks for the SSL work!

Original comment by hasse...@gmail.com on 9 Mar 2011 at 6:46

GoogleCodeExporter commented 9 years ago
I'm not sure how to do that, this is my first project hosted on google code.
If it's not too much effort please do it.

Btw, do you have some kind of a checklist for each release ?
How do you decide that, lets say version 0.7 is ready ?
I might be able to help with something.

p.s. the last entry in the CHANGELOG is from 6/24/2009, I didn't add anything 
as I didn't know the criteria when something should be added in there.

Original comment by pmilosev on 9 Mar 2011 at 7:07