search

jandrew / Spreadsheet-XLSX-Reader-LibXML

Read spreadsheet files with xlsx extentions
Other
4 stars 2 forks source link

_build_out_the_cell isn't reentrant proof #26

Closed Frank071 closed 9 years ago

Frank071 commented 9 years ago

Depending on the contents of a cell a call to _build_out_the_cell changes the source object in such a way that a second retrieval of the cell value results in a Moose error.

While devising a patch I found that there is another peculiar thing with the function. The mail body consists of an if that tests if the supplied object is an hashref or not. However, on every occassion the function is called, that test is already done in the callilng code as well.

I am not sure if this is as it should be, but if it isn't a patch should probably also address this.

Frank071 commented 9 years ago

I need to add to this that I found out that the existence of different group_format options will result is a more complicated patch than I had in mind... :-)

jandrew commented 9 years ago

Sorry for the delay in responding, I checked out for the weekend. Let me look over your pull request today and I'll respond by this evening.

jandrew commented 9 years ago

I did look at the pull requests and they seem to be focused elsewhere? In any case let me look at it and see what I can do.

Meanwhile, could you be more specific on which contents of a cell put the source object at risk for modification?

Frank071 commented 9 years ago

With respect to the pull requests: the open pull request should be on the same codebase.

As for the modification: a good example is the conversion from attribute 'row' (Excel) to 'cell_row' (attribute to S::X::R::L::Cell). This is done by adding the {cell_row} attribute and ... deleting the {row} element. The next time that specific cell is reprocessed and since {cell_row} is filled with the (now non existent) {row} the attribute is undef to which Moose throws an error. Same goes for 'col' and some other elements deeper in the code

jandrew commented 9 years ago

Sorry, when I meant that the pull requests are focused elsewhere they seem to be fixing other issues in this code base not this one is that correct?

Thanks for the pointer on where the code fails. I didn't mean for it to do that so let me see what I can do.

jandrew commented 9 years ago

Ok it looks like this was another case of the lazies since I had a build variable available in line 353 but I actually manipulate the old variable instead. I will move all collations and manipulations to the new variable. I will also quit messing with the old source variable.

You don't happen to have a test case for this do you?

I will update this in the next release as well.

Frank071 commented 9 years ago

Sorry, no testcase programmed. However, I think it probably suffices to rerequest the same cell a second time (in 'instance' mode).

jandrew commented 9 years ago

OK, I'll add that to the 10-get_cell.t test file.