scrapy/scrapy — pull request #722 by Digenis (2014-05-17 14:36:55+00:00)
Title: ItemLoader.load_item: iterate over copy of fields

ItemLoader._values, as a defaultdict(list), has the side-effect of setting non existent values even when they are not assigned. As an outcome, writing a processor that reads from its loader state causes cryptic errors such as the one in the following snippet.

```python
class ImagesItem(Item):
images = Field()
page_url = Field()

class ImagesLoader(ItemLoader):
default_item_class = ImagesItem
page_url_out = TakeFirst()

def images_out(self, values):
return tuple(
urljoin(self.get_output_value('page_url'), url )
for url in values)

imgloader = ImagesLoader(response=response)
imgloader.add_xpath('images', '//img/@src')
imgloader.load_item()
```
The last line will raise a ```RuntimeError: dictionary changed size during iteration``` exception which confuses a lot. If I try ```load_item()``` once more, this time it will load resulting in relative urls (since ```urljoin``` doesn't raise an Exception when base url is ```None```). For some loaders, depending on the calls to ```get_output_value()``` for uninitialised fields, ```load_item()``` will fail an unpredictable number of times until it succeeds.
Calling ```get_output_value()``` for the uninitialised field before ```load_item()```, even in the spider, will also hide the bug since getting the value implies setting it.
If an input processor, or a processor among the ones passed in the parameters to ```add_value``` filters it out, the person who wrote them may hardly determine the presence of this field in ```_values```. The loader has conditionals for bailing out further processing:

Iterating over an immutable copy of ```_values``` in ```load_item()``` solves this bug.

Now beyond the purpose of this pull request I want to bring some attention to the following related issues:

Deeper problems persist since the presence of a field key in ```_values``` is tangled with its boolean value and its type (None or not). The loader and its processors inconsistently treat values as empty (and as reasons to bail out processing):
```TakeFirst()``` considers ```None``` and ```''``` as empty.
```Compose()``` (by default) stops chaining calls to the rest of its wrapped functions when it encounters ```None```
```MapCompose()``` does not stop like ```Compose()```, nor it can through an argument
```ItemLoader.get_value()``` stops processing on ```None```
```ItemLoader.get_collected_values()``` attempts to retrieve the field from ```_values``` no matter what
```ItemLoader.get_output_value()``` as the above, and also process it
```ItemLoader.replace_value()``` will delete the value and will set it only when the replacement isn't None
```ItemLoader.load_item()``` iterates over all fields present in ```_values``` and uses only the not ```None```
To delete an item from a python dict you don't just assign ```None``` to its value, None is a value.

Most of the above cannot be fixed without breaking compatibility with projects that use the current item loaders implementation since many depend on the implicit filtering of what value each processing step may consider as empty.
I saw the discussion on the accepted PR https://github.com/scrapy/scrapy/pull/556 , which undoubtedly broke loaders that used ```None``` values. I think the loaders already took a course incompatible with a fix of the above issues. Probably many projects depend already on the implicit filtering of the various types of null.
Instead of fixing, I 'd prefer more explicit documentation on their behaviour.
e.g.:
```TakeFirst()``` filters 'non-null/non-empty' but doesn't define this non-empty as non-empty string.