Skip to content
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.

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

print(os.environ.keys()) should only print the keys #76481

Open
asmeurer mannequin opened this issue Dec 13, 2017 · 20 comments
Open

print(os.environ.keys()) should only print the keys #76481

asmeurer mannequin opened this issue Dec 13, 2017 · 20 comments
Labels
3.12 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@asmeurer
Copy link
Mannequin

asmeurer mannequin commented Dec 13, 2017

BPO 32300
Nosy @rhettinger, @vstinner, @bitdancer, @asmeurer, @serhiy-storchaka, @csabella

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2017-12-13.08:37:00.625>
labels = ['type-bug', 'library', '3.9', '3.10']
title = 'print(os.environ.keys()) should only print the keys'
updated_at = <Date 2020-11-06.19:11:50.449>
user = 'https://github.com/asmeurer'

bugs.python.org fields:

activity = <Date 2020-11-06.19:11:50.449>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2017-12-13.08:37:00.625>
creator = 'Aaron.Meurer'
dependencies = []
files = []
hgrepos = []
issue_num = 32300
keywords = []
message_count = 20.0
messages = ['308190', '308199', '308200', '308219', '308220', '308223', '308241', '308244', '308249', '308251', '308266', '308309', '308314', '308315', '308321', '308334', '308335', '308338', '308357', '308358']
nosy_count = 6.0
nosy_names = ['rhettinger', 'vstinner', 'r.david.murray', 'Aaron.Meurer', 'serhiy.storchaka', 'cheryl.sabella']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32300'
versions = ['Python 3.9', 'Python 3.10']

@asmeurer
Copy link
Mannequin Author

asmeurer mannequin commented Dec 13, 2017

Take the following scenario which happened to me recently. I am trying to debug an issue on Travis CI involving environment variables. Basically, I am not sure if an environment variable is being set correctly. So in my code, I put

print(os.environ.keys())

The reason I put keys() was 1, I didn't care about the values, and 2, I have secure environment variables set on Travis.

To my surprise, in the Travis logs, I found something like this

KeysView(environ({'TRAVIS_STACK_FEATURES': 'basic cassandra chromium couchdb disabled-ipv6 docker docker-compose elasticsearch firefox go-toolchain google-chrome jdk memcached mongodb mysql neo4j nodejs_interpreter perl_interpreter perlbrew phantomjs postgresql python_interpreter rabbitmq redis riak ruby_interpreter sqlite xserver', 'CI': 'true', ..., 'MANPATH': '/home/travis/.nvm/versions/node/v7.4.0/share/man:/home/travis/.kiex/elixirs/elixir-1.4.5/man:/home/travis/.rvm/rubies/ruby-2.4.1/share/man:/usr/local/man:/usr/local/clang-3.9.0/share/man:/usr/local/share/man:/usr/share/man:/home/travis/.rvm/man'}))

So instead of just printing the keys like I asked for, it printed the whole environment, plus "KeysView(environ(". Included here was my secure environment variable.

Now, fortunately, Travis hides the contents of secure environment variables in the logs, but it didn't used to (https://blog.travis-ci.com/2017-05-08-security-advisory).

Aside from being a potential security issue, it's just annoying that it prints the whole environment. The values are much larger than the keys. With a normal dictionary, print(d.keys()) just prints the keys:

>>> print(dict(a=1, b=2).keys())
dict_keys(['a', 'b'])

@asmeurer asmeurer mannequin added the 3.7 (EOL) end of life label Dec 13, 2017
@csabella
Copy link
Contributor

For your current situation, list(os.environ) or iter(os.environ) both return keys only.

It looks like the __repr__ on the class for os.environ is printed for os.environ (which is expected). For os.environ.keys(), the same __repr__ is wrapped as a KeysView, for os.environ.values(), it's wrapped as a ValuesView, and os.environ.items(), as an ItemsView.

In 2.7, os.environ.keys() print just keys.

@csabella csabella added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 13, 2017
@bitdancer
Copy link
Member

This is a consequence of the repr used by KeysView, which it inherits from MappingView. I agree that the result is surprising, but there may not be a generic fix. It's not entirely clear what KeysView should do here, but presumably we could at least add a repr to environ's use of it that would produce something useful.

Note that a workaround when doing something like a travis debug is to print os.environ._data.keys(), which will print the bytes versions of the keys. I wouldn't recommend depending on that continuing to, though ;)

@bitdancer bitdancer removed the type-bug An unexpected behavior, bug, or error label Dec 13, 2017
@vstinner
Copy link
Member

Usually, I use print(sorted(os.environ)) since I prefer a sorted list and it prevents such issue :-)

David:

I agree that the result is surprising, but there may not be a generic fix.

What about something like:

vstinner@apu$ ./python -c 'import os; print(os.environ.keys())'
<KeysView ['LS_COLORS', 'XDG_MENU_PREFIX', 'LANG', 'GDM_LANG', 'HISTIGNORE', 'HISTCONTROL', 'DISPLAY', 'HOSTNAME', 'EDITOR', 'COLORTERM', 'DESKTOP_AUTOSTART_ID', 'USERNAME', 'XDG_VTNR', 'SSH_AUTH_SOCK', 'XDG_SESSION_ID', 'USER', 'DESKTOP_SESSION', 'GTK2_RC_FILES', 'PWD', 'SSH_ASKPASS', 'HOME', 'JOURNAL_STREAM', 'XDG_SESSION_TYPE', 'XDG_DATA_DIRS', 'XDG_SESSION_DESKTOP', 'LOADEDMODULES', 'MAIL', 'WINDOWPATH', 'VTE_VERSION', 'TERM', 'SHELL', 'QT_IM_MODULE', 'XMODIFIERS', 'XDG_CURRENT_DESKTOP', 'XDG_SEAT', 'SHLVL', 'PYTHONPATH', 'WINDOWID', 'MODULEPATH', 'GDMSESSION', 'GNOME_DESKTOP_SESSION_ID', 'LOGNAME', 'DBUS_SESSION_BUS_ADDRESS', 'XDG_RUNTIME_DIR', 'XAUTHORITY', 'PATH', 'PS1', 'MODULESHOME', 'MAKEFLAGS', 'HISTSIZE', 'SESSION_MANAGER', 'LESSOPEN', 'BASH_FUNC_module%%', 'BASH_FUNC_scl%%', '_', 'OLDPWD']>

vstinner@apu$ git diff
diff --git a/Lib/_collections_abc.py b/Lib/_collections_abc.py
index a5c7bfcda1..5e524dffbf 100644
--- a/Lib/_collections_abc.py
+++ b/Lib/_collections_abc.py
@@ -719,6 +719,9 @@ class KeysView(MappingView, Set):
     def __iter__(self):
         yield from self._mapping
 
+    def __repr__(self):
+        return '<KeysView %r>' % list(self)
+
 KeysView.register(dict_keys)
 
 

list(key_view) is valid. I mimicked dict views implementation:

static PyObject *
dictview_repr(_PyDictViewObject *dv)
{
    PyObject *seq;
    PyObject *result;
    seq = PySequence_List((PyObject *)dv);
    if (seq == NULL)
        return NULL;
    result = PyUnicode_FromFormat("%s(%R)", Py_TYPE(dv)->tp_name, seq);
    Py_DECREF(seq);
    return result;
}

@vstinner
Copy link
Member

If we decide to change abc.KeysView.__repr__, IMHO we should also modify abc.ValuesView.__repr__, and maybe also abc.ItemsView.__repr__.

@bitdancer
Copy link
Member

Agreed about the other classes if we change this. Your solution looks reasonable to me.

@vstinner
Copy link
Member

Cheryl: would you like to work on a PR? If yes, tests are needed.

@serhiy-storchaka
Copy link
Member

Don't make KeysView.__repr__ and ValuesView.__repr__ containing the lists of all keys and values. This will make the repr of the view of a mapping which is a proxy of an external DB containing the full content of that DB, which can be gigabytes. See for example rejected bpo-21670.

@asmeurer
Copy link
Mannequin Author

asmeurer mannequin commented Dec 13, 2017

So the best fix is to just override keys() in the _Environ class, so that it returns an EnvironKeysView class that overrides __repr__?

@vstinner
Copy link
Member

Don't make KeysView.__repr__ and ValuesView.__repr__ containing the lists of all keys and values. This will make the repr of the view of a mapping which is a proxy of an external DB containing the full content of that DB, which can be gigabytes. See for example rejected bpo-21670.

The current implementation displays repr(self._mapping):

class MappingView(Sized):
    ...
    def __repr__(self):
        return '{0.__class__.__name__}({0._mapping!r})'.format(self)

For your proxy example: what is the proxy? The KeysView subtype, or the mapping?

repr(list), repr(dict) and repr(set) all render their full content in the result, no?

repr() on a list of 1 million of items *will* render all items, even if nobody wants to read such very long string :-)

If you want to get a smarter __repr__() than the default implementation: just override __repr__(), no?

I don't see why KeysView must be special.

@serhiy-storchaka
Copy link
Member

shelve.Shelf is the example of such kind.

@csabella
Copy link
Contributor

Victor,

Thanks! Yes, I can work this. It may take a few days for me to get to it.

@vstinner
Copy link
Member

Currently, repr(Shelf.keys()) doesn't dump the content of the shelf:

>>> import pickle, shelve
>>> s=shelve.Shelf({b'key': pickle.dumps('value')})
>>> k=s.keys()
>>> k
KeysView(<shelve.Shelf object at 0x7fba1f6189b0>)
>>> list(k)
['key']
>>> list(s.values())
['value']

I understand that changing KeysView.__repr__() changes repr(Shelf.keys()) behaviour.

@vstinner
Copy link
Member

Cheryl: "Thanks! Yes, I can work this. It may take a few days for me to get to it."

It seems like we have a disagreement on how to fix this issue. We should first agree on the best solution.

@serhiy-storchaka
Copy link
Member

Options:

  1. Use the subclass of KeyView with overridden __repr__ for os.environ.keys(). This will add a code for pretty rare use case.

  2. Remove os.environ.__repr__. You can use repr(os.environ.copy()) or repr(dict(os.environ)) if you want to see a content.

  3. Do not change anything. You can use repr(list(os.environ.keys())) if you want to see only keys.

@asmeurer
Copy link
Mannequin Author

asmeurer mannequin commented Dec 14, 2017

Serhiy, isn't option 4?

  1. Make KeysView.__repr__ show list(self). Add a custom wrapper for Shelf's KeysView so that it doesn't do this.

This seems to be what Victor is suggesting. It makes the most sense to me for the common (i.e., default) case to be to show the keys (and just the keys), and for use cases that want otherwise to subclass and modify.

@serhiy-storchaka
Copy link
Member

With option 4 we need to add a custom wrapper not only for Shelf's KeysView, but for KeysView of all similar proxy classes, including classes in third-party code not available for us. This is impossible.

@asmeurer
Copy link
Mannequin Author

asmeurer mannequin commented Dec 14, 2017

Can't third party code write their own proxies? Why do we have to do that?

@csabella
Copy link
Contributor

I'm missing something here, so please forgive me for asking, but why is it bad to add:

    def keys(self):
        return self._data.keys()

    def values(self):
        return self._data.values()

    def items(self):
        return self._data.items()

to the _Environ class?

@csabella
Copy link
Contributor

Never mind. I see it's in bytes. I missed that the first time. But, if it wasn't in bytes, would that be an OK solution? Or is it bad to override those methods in that class?

@iritkatriel iritkatriel added 3.9 only security fixes 3.10 only security fixes type-bug An unexpected behavior, bug, or error and removed 3.7 (EOL) end of life labels Nov 6, 2020
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel iritkatriel removed the 3.9 only security fixes label Jun 16, 2022
@iritkatriel iritkatriel added 3.12 bugs and security fixes and removed 3.10 only security fixes labels Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants