BUG: Fix json_normalize throwing TypeError when record_path has a sequence of dicts #22706
State: closed

 
- [x] closes #22706
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: IO JSON
description: read_json, to_json, json_normalize

Comments:
Hello @vuminhle! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`pandas/io/json/normalize.py`](https://github.com/vuminhle/pandas/blob/da84997c3c7589b5d29d64c9bf935ab16f4d4ecb/pandas/io/json/normalize.py) !

 - There are no PEP8 issues in the file [`pandas/tests/io/json/test_normalize.py`](https://github.com/vuminhle/pandas/blob/da84997c3c7589b5d29d64c9bf935ab16f4d4ecb/pandas/tests/io/json/test_normalize.py) !



##### Comment last updated on September 26, 2018 at 04:20 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=h1) Report
> Merging [#22804](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/91802fb0accde031c3b6aca040a8b533a193fef6?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22804/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #22804      +/-   ##
==========================================
+ Coverage   92.21%   92.21%   +<.01%     
==========================================
  Files         162      162              
  Lines       51763    51770       +7     
==========================================
+ Hits        47733    47741       +8     
+ Misses       4030     4029       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.62% <100%> (ø)` | :arrow_up: |
| #single | `42.99% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/json/normalize.py](https://codecov.io/gh/pandas-dev/pandas/pull/22804/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vbm9ybWFsaXplLnB5) | `96.93% <100%> (+0.06%)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/22804/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.65% <0%> (ø)` | :arrow_up: |
| [pandas/core/groupby/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/22804/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9nZW5lcmljLnB5) | `87.12% <0%> (+0.04%)` | :arrow_up: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/22804/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `96.67% <0%> (+0.16%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=footer). Last update [91802fb...db24124](https://codecov.io/gh/pandas-dev/pandas/pull/22804?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Any other feedback @WillAyd @jreback ?
I think this PR also fixes #21605
Can you merge master?
> Can you merge master?

Done.
Is there a reason for both `_extract` and `_recursive_extract`? Pretty confusing IMO would be better served as one consolidated function I think
> Is there a reason for both `_extract` and `_recursive_extract`? Pretty confusing IMO would be better served as one consolidated function I think

The new `_extract` performs the actual record extraction. I can consolidate the two but we need to convert `data` to list in case it is not. Then we can iterate on `data` as in the original code. It's the same hack as line 198. `_recursive_extract` will look roughly like this:

```python
if len(path) > 1:
    ...
else:
    if not isinstance(data, list):
        data = [data]
    for obj in data:
        # perform actual extraction
```
Since we only have 1 function now, I think we can revert the change of returning tuples. IMO it looks ugly. What do you think?


> I can consolidate the two but we need to convert `data` to list in case it is not

I'm pretty sure this is done quite a few other places so not out of the ordinary, though feel free to double check.



> Since we only have 1 function now, I think we can revert the change of returning tuples. IMO it looks ugly. What do you think?

Not sure I fully understand what you have in mind but yea I don't think as is this is very readable. Any refactor / simplification here which does away with similar but separate functions and does not require mutation of an argument would be a great improvement
> > I can consolidate the two but we need to convert `data` to list in case it is not
> 
> I'm pretty sure this is done quite a few other places so not out of the ordinary, though feel free to double check.
> 
> > Since we only have 1 function now, I think we can revert the change of returning tuples. IMO it looks ugly. What do you think?
> 
> Not sure I fully understand what you have in mind but yea I don't think as is this is very readable. Any refactor / simplification here which does away with similar but separate functions and does not require mutation of an argument would be a great improvement

Can you take another look at the changes? Since `_recursive_extract` assumes that `data` is always a list, I added 2 lines to convert `data` to list if it is not.
this also fixed #21605? can you add a test and add to the whatsnew
> this also fixed #21605? can you add a test and add to the whatsnew

Looks like #21605 is a duplicate (of #22804 ). We can just close #21605.
hmm, though that is a list of dicts?
> hmm, though that is a list of dicts?

list of dicts should work already.
ok this lgtm then. @WillAyd over to you.