Xml parser improved #18

Closed
wants to merge 4 commits into from

2 participants

@cricri

No description provided.

christophe added some commits Sep 12, 2013
@jmarca
Owner

As I said in the other, I've been busy. I check these out.

Also, I prefer atomic changes---easier for me to see what is what. If you can split these into two different patches I'd appreciate it.

@jmarca
Owner

So it looks like the core change is you switched from libxmljs to xml2js

Any reason for that? Is it faster? Was a test failing? Is there new functionality (I didn't see any new tests, so I'm guessing not).

I actually wanted libxmljs to allow for more flexibility with parsing the xml from the CAS server, which gets a little bit complicated if you are doing it right. Granted I'm not everything libxmljs has to offer, but I also trust libxml having used it for years, and I'm not so sure I trust sax-js (upon which xml2js is based).

Also, your comment says parsing was improved, but what was wrong with the prior? If something was breaking or failing, I'd like a test to show the difference so that it doesn't break again.

Obviously your code is easier to read than my sax parser, so I'm not arguing it isn't better in that way. Just want to be convinced that xml2js is somehow better than libxmljs. Maybe I'll take a whack at cleaning up my original and then we can compare?

@cricri

Hi James,

Sorry for answering so late. Too much hurry. And sorry for the non atomic commit.
It's "better" only because it's simpler to parse attributes. In my case cas returns a complicated list of attributes, and it was easier to use xml2js than the current implementation. Feel free to use it or not.
I will send you another patch which returns the attributes as non-array when not needed.

Christophe

@jmarca
Owner
@cricri

Hi James,

a sample of the XML returned:

<cas:serviceResponse xmlns:cas='http://www.yale.edu/tp/cas'>
  <cas:authenticationSuccess>
    <cas:user>bob</cas:user>
    <cas:attribute name="uid" value="b01234" />
    <cas:attribute name="mail" value="bob@mail.com" />
    <cas:attribute name="cn" value="smith" />
    <cas:attribute name="givenname" value="bob" />
    <cas:attribute name="service" value="other" />
    <cas:attribute name="permission" value="p1" />
    <cas:attribute name="permission" value="p2" />
    <cas:attribute name="permission" value="p3" />
    <cas:attribute name="uidnumber" value="123456789" />
  </cas:authenticationSuccess>
</cas:serviceResponse>

Christophe

@jmarca
Owner

Thanks. I'll try to put some time in on this today or tomorrow.

@jmarca
Owner

Sorry I have not gotten this done yet. I got stalled on the testing, and then work demand heated up. I will try to finish this soon...I will either switch to your approach, or simplify the use of libxmljs, so that the xml parsing is easier to understand and modify

@jmarca
Owner

I'm going to close this issue as I can't and won't merge the updated xml parser. I do not want to use the node-based parser. I might eventually switch to node-expat, but not right now.

I have recoded my xml parser in the latest version. I've added your example xml file, and a test that properly parses that file's attributes.

Apparently, there does not seem to be a standard way of including user attributes in the CAS spec, as it is not part of the spec at all, but rather an add-on that is somewhat dependent upon the back-end being used. So I'm doing a bit of duck typing here...if it looks like ..., do it the default way, but if it is like your case iwth just a list of elements, then do it a second way.

Check out my latest version if you want, or feel free to maintain your own fork...there haven't been many other changes with this library other than parsing attributes.

@jmarca jmarca closed this Feb 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment