Skip to content

Fix issue #36271 to disambiguate json string #36273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
+14 −0
Closed

Fix issue #36271 to disambiguate json string #36273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8c40f23
Fix issue #36271 to disambiguate json string
tbachlechner Sep 10, 2020
e5777f4
BUG: pd.read_json() fails for strings that look similar to fsspec_url…
tbachlechner Sep 10, 2020
ed49bfe
fixed duplicate test_is_fsspec_url()
tbachlechner Sep 10, 2020
0d7e063
Update common.py
tbachlechner Sep 10, 2020
3706d18
check if url is a json string
tbachlechner Sep 10, 2020
210d659
Test test_is_fsspec_url() interprets json as URL
tbachlechner Sep 10, 2020
6b2c98c
Update test_common.py
tbachlechner Sep 10, 2020
6019978
Update common.py
tbachlechner Sep 10, 2020
def1881
use regex to check for json in url
tbachlechner Sep 17, 2020
943f644
simplify is_json
tbachlechner Nov 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pandas/io/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import mmap
import os
import pathlib
import re
from typing import (
IO,
TYPE_CHECKING,
Expand Down Expand Up @@ -153,6 +154,16 @@ def urlopen(*args, **kwargs):
return urllib.request.urlopen(*args, **kwargs)


def is_json(url: FilePathOrBuffer) -> bool:
"""
Returns true if the given string looks like
json
"""
json_pattern = re.compile(r"^\s*[\[{]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably need something like the following for typing

if not isinstance(url, str):
    return False

The pre-commit is complaining about too many new lines.

return json_pattern.match(url) is not None



def is_fsspec_url(url: FilePathOrBuffer) -> bool:
"""
Returns true if the given URL looks like
Expand All @@ -161,6 +172,7 @@ def is_fsspec_url(url: FilePathOrBuffer) -> bool:
return (
isinstance(url, str)
and "://" in url
and not is_json(url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For pandas maintainers: should this check only be done within read_json (which will presumably call json.loads anyway, repeating the work) ? Are there any other similar cases that the string passed might be handled as a path, or decoded directly, that I might have missed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read_json doesn't call json.loads

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tbachlechner if you really think t his out to be caught then i suppose a 'better' check instead of and "://" in url could be done (e.g. a regex)

and not url.startswith(("http://", "https://"))
)

Expand Down
2 changes: 2 additions & 0 deletions pandas/tests/io/test_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,3 +417,5 @@ def test_is_fsspec_url():
assert not icom.is_fsspec_url("random:pandas/somethingelse.com")
assert not icom.is_fsspec_url("/local/path")
assert not icom.is_fsspec_url("relative/local/path")
# Ensure json string is not interpreted as URL
assert not icom.is_fsspec_url('{"json": "text ://"}')