DolphDev / pynationstates

Python API wrapper for NationStates
MIT License
15 stars 4 forks source link

Nation.command sometimes returns unparsed string #17

Closed bekaertruben closed 3 years ago

bekaertruben commented 3 years ago

As said in the title Nation.command may return a string instead of an NSDict. Here's the code (from https://github.com/bekaertruben/ns-census-maximizer) where I discovered this issue: image I'd had this occur for a while so I've been running the script with a debugger for a couple days to see what the string was. However, most of the time it works as expected. Now that I've finally caught it, here it is: https://pastebin.com/SFxK9srp. This simply seems like unparsed xml as the nationstates api returns it. The version I use is the latest from pypi (3.0.2.7).

DolphDev commented 3 years ago

Just a note, I cant find your code example in your repo

DolphDev commented 3 years ago

So issues are a bit of annoying thing to handle with there possible random errors - I created Nation.pick_issue that should cover this.

Now if that pastebin was the actually one, we may still have an issue. But pick_issue will give you a much better error message. Could you try swapping to it and see if that resolves it (or give better message)

bekaertruben commented 3 years ago

The sample is from here, but split for the sake of debugging. I'll switch to Nation.pick_issue and I'll get back to you within a couple of days on whether the issue persists.

bekaertruben commented 3 years ago

Immediately the issue has occurred. Issue is the exact same: nation.command returns a string:

Traceback (most recent call last):
  File "/home/kumiho/projects/ns-census-maximizer/puppetmaster.py", line 102, in <module>
    solver.solve_issues()
  File "/home/kumiho/projects/ns-census-maximizer/census_maximizer.py", line 145, in solve_issues
    option_picked, outcome = self.solve_issue(issue, log = log)
  File "/home/kumiho/projects/ns-census-maximizer/census_maximizer.py", line 96, in solve_issue
    response = self.nation.pick_issue(issue.id, best_option)
  File "/home/kumiho/.local/lib/python3.9/site-packages/nationstates/objects.py", line 261, in pick_issue
    if resp["data"][self.api_name]["issue"]["error"]:
TypeError: string indices must be integers

While trying to track down the origin of the issue, I should note that this should simply be

return self.request(shards=args, full_response=full_response, use_post=use_post)

However, as I can't exactly reproduce the error at will, I have no idea why the response isn't parsed.

DolphDev commented 3 years ago

return self.request(shards=args, full_response=full_response, use_post=use_post)

This originally because I may have special code for post, but that is no longer the case. I'll fix optimize should I make another release

DolphDev commented 3 years ago

The XML is failing to Parse. I'll see if I can find what it's erroring out about.

DolphDev commented 3 years ago

&egrave; is failing

DolphDev commented 3 years ago

https://github.com/DolphDev/pynationstates/pull/18 - Patch for this. As last resort we will process html entities to make xmltodict happy. This isn't an ideal solution unfortunately. Sometimes Nationstates likes to put in actual html, notable for the dispatchs, which make this theoretically risky, as the dict returned will have things like 'a' tags in them. I'll probably eventually dig into xmltodict and see what entities it doesn't like and just process those ones. it didn't seem to mind the html ones.

I'll post new patch tomorrow.

bekaertruben commented 3 years ago

Doing a little research reveals that xml only has five entities that all xml processors must honor. I suspect you could probably just replace all ampersands with &amp;. That would avoid the risks of processing html as you propose, but of course all strings will be full of &quot, &apos and whatnot. (Then again, most strings probably won't be looked at by a human, and it shouldn't be all that hard to process those that one does want to look at)

DolphDev commented 3 years ago

I added code to protect this entity by swapping them (and later swapping back post parse). This should protect HTML entities while not causing xmltodict to give up.

I will consider a minor major release (3.0.3.X) to swap some logic around and completely patch this whole. if the library can't parse it, It will probably just throw a bug message asking to create an issue on github rather than silencing it

bekaertruben commented 3 years ago

Alright, thanks for fixing this so quickly!

DolphDev commented 3 years ago

Released - https://pypi.org/project/nationstates/3.0.2.8/