graetzer / GDataXML-HTML

HTML and XML parser for iOS and OSX, supports XPath
Apache License 2.0
289 stars 121 forks source link

1.2 changes root node for XPath #11

Closed artemyarulin closed 9 years ago

artemyarulin commented 10 years ago

Starting from v.1.2 xpath stopped returning current node after document creation. Well, it is easier to explain here:

    GDataXMLDocument* doc = [[GDataXMLDocument alloc] initWithXMLString:@"<doc/>" error:NULL];
    XCTAssertEqual([doc nodesForXPath:@"//doc" error:NULL].count,(NSUInteger)1,@"1.1: Works, 1.2: Works");
    XCTAssertEqual([doc nodesForXPath:@"/doc" error:NULL].count,(NSUInteger)1, @"1.1: Works, 1.2: Works");
    XCTAssertEqual([doc nodesForXPath:@"doc" error:NULL].count,(NSUInteger)1,  @"1.1: Works, 1.2: Fails");

Most probably it happened because of this commit: https://github.com/graetzer/GDataXML-HTML/commit/87ae11b09b7041195016d46f1a88f5368d23cd8e. As far as I understand it changes default root element.

graetzer commented 10 years ago

The commit sets the beginning of the xpath lookup to the root element of the document. https://github.com/graetzer/GDataXML-HTML/blob/master/Pod/Classes/GDataXMLNode.m#L745

Not sure what to do about that, looks like we would need to reimplement that method for GDataXMLDocument, instead of reusing the implementation from GDataXMLNode.

artemyarulin commented 9 years ago

@graetzer Hi again, any progress on this?

I'm not quite familiar with all these pointer to pointer magic, but I could probably try to fix it if you can explain what exactly did you mean by "reimplement that method for GDataXMLDocument"

Or maybe @master-nevi has a better idea how can we fix this?

graetzer commented 9 years ago

Hey I tried to fix it in the branch xpath-fix, I just ran a few tests and it seems to work. Essentially I copied the 'nodeForXpath' method from GDataXMLNode. I am not sure if there is a better way with less redundant code. I don't get any errors on x64 (as in https://github.com/graetzer/GDataXML-HTML/issues/10), but your third example doesn't work without this line and I am not sure if this cast might cause any issues

artemyarulin commented 9 years ago

Hm, well to be honest I have no idea, for me it looks OK, at least in the original version of the lib there was a cast like that.

Well, let put it this way - I'm going to update my application in a couple of weeks and I would use the code from your xpath-fix branch, let see how many crash reports I would get :)

master-nevi commented 9 years ago

Hi there. Sorry I'm late to the party. It's unfortunate that you couldn't get the third example to work without casting _xmlDoc to xmlNodePtr. I did a bit of investigating myself and found several libraries that perform this cast roughly the same way this branch is doing it. I also found some open source Apple code which uses xmlDocGetRootElement() when the xpath context node reference is the doc element itself, but only when handling the namespaces, again, roughly the same way this branch is working. This looks good to me.

artemyarulin commented 9 years ago

@master-nevi Thank you for the investigation! @graetzer What do you think? Can we merge a branch to master or you would like to wait until I check this update in real life to be sure that it doesn't cause any crashes?

artemyarulin commented 9 years ago

Hi again, any chance to get this merged to the master and release a new version to CocoaPods? I'm now creating react-native-xml and having a new version in CocoaPods would make my life easier!

Thanks!

graetzer commented 9 years ago

Just pushed 1.3.0 to cocoapods, should be fixed now