Query Server package code review

[iblislin]

Let's review it commit-by-commit!

Some highlight:

• The __main__.py will invoke server.__init__.SimpleQueryServer
• The SimpleQueryServer support some legacy query protocol. Are we still need them?

Here is the snippet about the command support.

        if (0, 9, 0) <= self.version < (0, 10, 0):
            self.commands['show_doc'] = render.show_doc
            self.commands['list_begin'] = render.list_begin
            self.commands['list_row'] = render.list_row
            self.commands['list_tail'] = render.list_tail
            self.commands['validate'] = validate.validate

        elif (0, 10, 0) <= self.version < (0, 11, 0):
            self.commands['show'] = render.show
            self.commands['list'] = render.list
            self.commands['filter'] = filters.filter
            self.commands['update'] = render.update
            self.commands['validate'] = validate.validate

        elif self.version >= (0, 11, 0):
            ddoc_commands = {}
            ddoc_commands['shows'] = render.ddoc_show
            ddoc_commands['lists'] = render.ddoc_list
            ddoc_commands['filters'] = filters.ddoc_filter
            ddoc_commands['updates'] = render.ddoc_update
            ddoc_commands['validate_doc_update'] = validate.ddoc_validate

        if self.version >= (1, 1, 0):
            self.commands['add_lib'] = state.add_lib
            ddoc_commands['views'] = filters.ddoc_views

        if self.version >= (0, 11, 0):
            self.commands['ddoc'] = ddoc.DDoc(ddoc_commands)

[iblislin]

I was wandering ... why do we make this limit?

We can simply pop the var from globals_ and insert it into context, then let user enjoy the global var.
( I think user imported module can be global, also)

[kxepal]

Because there is no any sane reason for using anything else except modules and functions in globals. Otherwise users will try to use classes and hit awkward issues because all the design functions should be pure.

[iblislin]

If we throw all the function and module(s) into global scope, it will be handy for user. But do we introduce some terrible side effect?

diff --git a/couchdb/server/compiler.py b/couchdb/server/compiler.py
index c91a4ef..3ba2022 100644
--- a/couchdb/server/compiler.py
+++ b/couchdb/server/compiler.py
@@ -385,4 +385,8 @@ def compile_func(funsrc, ddoc=None, context=None, encoding='utf-8', **options):
     if msg is not None:
         log.error('%s\n%s', msg, funsrc)
         raise Error('compilation_error', msg)
+
+    context.update(globals_)
+    globals_.clear()
+
     return func
diff --git a/couchdb/tests/server/compiler.py b/couchdb/tests/server/compiler.py
index ac50b3b..edc0c32 100644
--- a/couchdb/tests/server/compiler.py
+++ b/couchdb/tests/server/compiler.py
@@ -254,19 +259,23 @@ class CompilerTestCase(unittest.TestCase):
     def test_allow_imports(self):
         funsrc = (
             'import math\n'
-            'def test(math=math):\n'
+            'def test():\n'
             '  return math.sqrt(42*42)'
         )
         func = compiler.compile_func(funsrc)
         self.assertEqual(func(), 42)

-    def test_fail_for_non_clojured_imports(self):
+    def test_for_module_scope(self):
         funsrc = (
             'import math\n'
             'def test():\n'
-            '  return math.sqrt(42*42)')
+            '  assert "math" in globals()\n'
+            '  return math.sqrt(42*42)\n'
+            # At the time being executed,
+            # we do not move module/func into globals yet
+            'assert "math" not in globals()\n'
+        )
         func = compiler.compile_func(funsrc)
-        self.assertRaises(NameError, func)
+        self.assertEqual(func(), 42)

     def test_ascii_function_source_string(self):
         funsrc = 'def test(): return 42'

[kxepal]

Yes, that's the most annoying moment. I don't see any way how it will cause terrible side effect (since we're on Python with a lot of side effects) while we control global scope for each function. I think usability matters more since you don't do def main(math=math) in your everydays code.

[kxepal]

Worth to add another assertion that we don't pollute next function compile/call cycle by imports from the previous one.

[kxepal]

nice

[iblislin]

So, maybe we should note user not to make some side effect on module. Modules will be cached by Cpython.

Acctually, even if we do not change the scope of module into global, this problem still exists.
Once we are using the same Cpython process, we still have this problem.

In [1]: def f():
   ...:     import math
   ...:     math.answer = 42
   ...:     

In [2]: f()

In [3]: def g():
   ...:     import math
   ...:     print(math.answer)
   ...:     

In [4]: g()
42

[iblislin]

I think the patch v1 is fine enougth now, except we really want to take care the module cache( manually GC? ).

diff --git a/couchdb/server/compiler.py b/couchdb/server/compiler.py
index c91a4ef..3ba2022 100644
--- a/couchdb/server/compiler.py
+++ b/couchdb/server/compiler.py
@@ -385,4 +385,8 @@ def compile_func(funsrc, ddoc=None, context=None, encoding='utf-8', **options):
     if msg is not None:
         log.error('%s\n%s', msg, funsrc)
         raise Error('compilation_error', msg)
+
+    context.update(globals_)
+    globals_.clear()
+
     return func
diff --git a/couchdb/tests/server/compiler.py b/couchdb/tests/server/compiler.py
index ac50b3b..edc0c32 100644
--- a/couchdb/tests/server/compiler.py
+++ b/couchdb/tests/server/compiler.py
@@ -254,19 +259,23 @@ class CompilerTestCase(unittest.TestCase):
     def test_allow_imports(self):
         funsrc = (
             'import math\n'
-            'def test(math=math):\n'
+            'def test():\n'
             '  return math.sqrt(42*42)'
         )
         func = compiler.compile_func(funsrc)
         self.assertEqual(func(), 42)

-    def test_fail_for_non_clojured_imports(self):
+    def test_module_scope(self):
         funsrc = (
             'import math\n'
             'def test():\n'
-            '  return math.sqrt(42*42)')
+            '  assert "math" in globals()\n'
+            '  return math.sqrt(42*42)\n'
+            # At the time being executed,
+            # we do not move module/func into globals yet
+            'assert "math" not in globals()\n'
+        )
         func = compiler.compile_func(funsrc)
-        self.assertRaises(NameError, func)
+        self.assertEqual(func(), 42)

     def test_ascii_function_source_string(self):
         funsrc = 'def test(): return 42'

[kxepal]

Ok, I think we can create issue on this to improve that behaviour. Finally, if you do such blackmagic you should know what are you doing and since we don't provide any sandbox, you shouldn't blindly trust thirdparty code without very pedantic analysis of it.

[iblislin]

Ha, forgot 'manaully gc'. That's some kind of joking words, but text message is hard to express it. (Maybe i should add a delete line 😆 )

I will stand for patch v1 and highlight "no sandbox, use side effect at your own risk" on documentation.
Perhaps there are someone really need this hidden feature side effect on module.

[iblislin]

Err... It seems that it blew up when user put some builtin function here.

e.g.:

In [20]: sorted
Out[20]: <function sorted>

In [21]: type(sorted)
Out[21]: builtin_function_or_method

In [22]: inspect.getsource(sorted)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-22-c9deb9140b13> in <module>()
----> 1 inspect.getsource(sorted)

/home/iblis/py/py36/lib/python3.6/inspect.py in getsource(object)
    925     or code object.  The source code is returned as a single string.  An
    926     OSError is raised if the source code cannot be retrieved."""
--> 927     lines, lnum = getsourcelines(object)
    928     return ''.join(lines)
    929 

/home/iblis/py/py36/lib/python3.6/inspect.py in getsourcelines(object)
    912     raised if the source code cannot be retrieved."""
    913     object = unwrap(object)
--> 914     lines, lnum = findsource(object)
    915 
    916     if ismodule(object):

/home/iblis/py/py36/lib/python3.6/inspect.py in findsource(object)
    725     is raised if the source code cannot be retrieved."""
    726 
--> 727     file = getsourcefile(object)
    728     if file:
    729         # Invalidate cache if needed.

/home/iblis/py/py36/lib/python3.6/inspect.py in getsourcefile(object)
    641     Return None if no way can be identified to get the source.
    642     """
--> 643     filename = getfile(object)
    644     all_bytecode_suffixes = importlib.machinery.DEBUG_BYTECODE_SUFFIXES[:]
    645     all_bytecode_suffixes += importlib.machinery.OPTIMIZED_BYTECODE_SUFFIXES[:]

/home/iblis/py/py36/lib/python3.6/inspect.py in getfile(object)
    623         return object.co_filename
    624     raise TypeError('{!r} is not a module, class, method, '
--> 625                     'function, traceback, frame, or code object'.format(object))
    626 
    627 def getmodulename(path):

TypeError: <built-in function sorted> is not a module, class, method, function, traceback, frame, or code object

[kxepal]

Yes, I know. Same story with lambdas. Basically, both are not suitable for any kind of design function, so no reason to care about.

[iblislin]

I'm going to attend PyCon TW this weekend.

Maybe disappear about 3 days....

[kxepal]

Cool! Have fun there! (:

[iblislin]

We init server.state in the constructor of BaseQueryServer.

        self._state = {
            'view_lib': None,
            'line_length': 0,
            'query_config': {},
            'functions': [],
            'functions_src': [],
            'row_line': {}
        }

But in this reset function, we do not reset the value of line_length and row_line.

About line_length: I cannot find any function need it. Is this variable an unimplemented feature?

Following grep shows that only docstring mensioned it.
It's the partner of reduce_limilt, but, the reduce_limit cannot be configured from commandline optoin.
Is reduce_limit an unimplemented feature, also?

└─[iblis@imfsa]% grep -1 line_length *
__init__.py-            'view_lib': None,
__init__.py:            'line_length': 0,
__init__.py-            'query_config': {},
Binary file __init__.pyc matches
--
views.py-          If any Python exception occurs or reduce output is twice longer
views.py:          as state.line_length and reduce_limit is enabled in state.query_config
views.py-    """
--
views.py-          If any Python exception occurs or reduce output is twice longer
views.py:          as state.line_length and reduce_limit is enabled in state.query_config
views.py-    """
Binary file views.pyc matches

[kxepal]

That's an artefact from JS query server backport. It has line_length value in state, but we eventually don't use it. Instead we have it used differently without using state.

[iblislin]

👍 . This patch removed the state.line_length and change the docstring of reduce and rereduce

diff --git a/couchdb/server/__init__.py b/couchdb/server/__init__.py
index 40a4328..f72e855 100644
--- a/couchdb/server/__init__.py
+++ b/couchdb/server/__init__.py
@@ -51,7 +51,6 @@ class BaseQueryServer(object):
         self._config = {}
         self._state = {
             'view_lib': None,
-            'line_length': 0,
             'query_config': {},
             'functions': [],
             'functions_src': [],
diff --git a/couchdb/server/views.py b/couchdb/server/views.py
index e885c65..6ce7ac3 100644
--- a/couchdb/server/views.py
+++ b/couchdb/server/views.py
@@ -79,7 +79,8 @@ def reduce(server, reduce_funs, kvs, rereduce=False):
     :raises:
         - :exc:`~couchdb.server.exceptions.Error`
           If any Python exception occurs or reduce output is twice longer
-          as state.line_length and reduce_limit is enabled in state.query_config
+          than input key-value pairs.
+          In the latter case, we consider it as ``reduce_overflow_error``.
     """
     reductions = []
     _append = reductions.append
@@ -132,7 +133,8 @@ def rereduce(server, reduce_funs, values):
     :raises:
         - :exc:`~couchdb.server.exceptions.Error`
           If any Python exception occurs or reduce output is twice longer
-          as state.line_length and reduce_limit is enabled in state.query_config
+          than input key-value pairs.
+          In the latter case, we consider it as ``reduce_overflow_error``.
     """
     log.debug('Rereducing values:\n%s', values)
     return reduce(server, reduce_funs, values, rereduce=True)

[kxepal]

+1 Nice catch!

[iblislin]

Hmm..next issues: Do we need to expose the reduce_limilt to command line? If yes, what do we expect when the reset is invoked?.( reset cleanup the state['query_config'], where the reduce_limit live.)

[kxepal]

Reset is a command which server sends to QS. Mostly everytime when it tries to execute a view.

[iblislin]

Yep. The main concern in my question is that "If state get reset again and again, the value of reduce_limilt will be volatile. Should this value be basically consist?"
I guess reduce_limit do not change in most of the time, so let user set it via command line option (just a boolean flag) make more sense to me.

FYI:
I am looking this code block in BaseQueryServer

    def is_reduce_limited(self):
        """Checks if output of reduce function is limited."""
        return self.state['query_config'].get('reduce_limit', False)

[kxepal]

Well, this option also comes from CouchDB with the reset command (:
http://docs.couchdb.org/en/1.6.1/query-server/protocol.html#reset
So no reason to care about - that's not something we control.

[iblislin]

Ha! Ok, i miss the doc 😄

[iblislin]

The patch about state.line_length was committed @ a2ff50d.

[iblislin]

Add test case about resetting view_lib.

diff --git a/couchdb/server/state.py b/couchdb/server/state.py
index ee0ee0e..8cf8617 100644
--- a/couchdb/server/state.py
+++ b/couchdb/server/state.py
@@ -29,7 +29,7 @@ def reset(server, config=None):
         log.debug('Set new query config:\n%s', config)
         server.state['query_config'].update(config)
     if server.version >= (1, 1, 0):
-        server.state['view_lib'] = ''
+        server.state['view_lib'] = None
     return True


diff --git a/couchdb/tests/server/state.py b/couchdb/tests/server/state.py
index 42d7f52..71ad40a 100644
--- a/couchdb/tests/server/state.py
+++ b/couchdb/tests/server/state.py
@@ -64,6 +64,13 @@ class StateTestCase(unittest.TestCase):
         state.reset(server, {'foo': 'baz'})
         self.assertEqual(server.state['query_config'], {'foo': 'baz'})

+    def test_reset_view_lib(self):
+        server = MockQueryServer(version=(1, 1, 0))
+        self.assertEqual(server.state['view_lib'], None)
+        server.state['view_lib'] = {'foo': 'bar'}
+        state.reset(server)
+        self.assertEqual(server.state['view_lib'], None)
+

 def suite():
     suite = unittest.TestSuite()

[iblislin]

I have to get through my final exam 😭
I will be back about 1 week later...

[iblislin]

Good to check output of map_doc?

diff --git a/couchdb/tests/server/views.py b/couchdb/tests/server/views.py
index 0788d4c..7751487 100644
--- a/couchdb/tests/server/views.py
+++ b/couchdb/tests/server/views.py
@@ -115,7 +115,7 @@ class MapTestCase(unittest.TestCase):
             '  pass'
         )
         doc = {'_id': 'foo'}
-        views.map_doc(self.server, doc)
+        self.assertEqual(views.map_doc(self.server, doc), [[]])

     def test_yield_non_iterable(self):
         """should raise Error if map function do not yield iterable"""

FYI: the second example in http://docs.couchdb.org/en/1.6.1/query-server/protocol.html#map-doc

[kxepal]

+1

[iblislin]

comitted @ f91a237

[iblislin]

have no idea why the build failed ...

[kxepal]

How?

[iblislin]

I add the verbose flag but the error gone.
https://travis-ci.org/iblis17/couchdb-python/builds/139946573
It seems that it failed randomly.

[kxepal]

Heisenbug! Wonder what caused it since we don't do any blocking operations in the tests. May be something travis specific? I remember I had a test ran for 2 days on travis (not a joke) being hanged. There wasn't bug in code however.

[iblislin]

😆 Heisenbug.
I am trying to press rebuild button again and again now...

[iblislin]

Ooooh! I guess i triggered the Heisenbug, finally.
https://travis-ci.org/iblis17/couchdb-python/jobs/140084656
https://travis-ci.org/iblis17/couchdb-python/builds/140084637

[kxepal]

Nice catch!

======================================================================

FAIL: test_timeout_retry (couchdb.tests.couchhttp.SessionTestCase)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/iblis17/couchdb-python/couchdb/tests/couchhttp.py", line 32, in test_timeout_retry

    self.assertRaises(socket.timeout, session.request, 'GET', db.resource.url)

AssertionError: timeout not raised by request

----------------------------------------------------------------------

but seems like ENOTOURPROBLEM.

[iblislin]

Time to update this list?
https://github.com/rails/rails/blob/v5.0.0.1/actionpack/lib/action_dispatch/http/mime_types.rb

[kxepal]

Good idea!

[iblislin]

'xhtml': ['application/xhtml+xml', 'xhtml'],
                                    ^^^^^^ why duplicate `xhtml` here?

---

And in case of text

'text': ['text/plain; charset=utf-8', 'txt'],

I guess it should be

- 'text': ['text/plain; charset=utf-8', 'txt'],
+ 'text': ['text/plain; charset=utf-8'],
+ 'txt': ['text/plain; charset=utf-8'],

make MimeProvider.register_type create two entries of mimes_by_key ?

[kxepal]

Don't know.

[iblislin]

OMG... i am going to open ticket on jira ( but stuck on jira's login page, no idea why login failed even i changed password again and again

[kxepal]

Good idea. May be JIRA is on maintenance or something? Have you change your keyboard layout before typing a password?

[iblislin]

hmm,i just reset my password again... now encounter this. And i changed the browser... but still cannot pass captcha. 😭

[iblislin]

JIRA isn't on maintenance...

[iblislin]

Maybe I should create another account on jira...

---

Patch

• remove duplicate xhtml
• introduce type aliases for DEFAULT_TYPES, e.g.
  ('text', 'txt'): ['text/plain; charset=utf-8'],. txt is an alias of text.

diff --git a/couchdb/server/mime.py b/couchdb/server/mime.py
index 7e185d5..767c809 100644
--- a/couchdb/server/mime.py
+++ b/couchdb/server/mime.py
@@ -78,26 +78,35 @@ def best_match(supported, header):
 # Build list of `MIME types
 # <http://www.iana.org/assignments/media-types/>`_ for HTTP responses.
 # Ported from `Ruby on Rails
-# <https://github.com/rails/rails/blob/v3.1.0/actionpack/lib/action_dispatch/http/mime_types.rb>`_
+# <https://github.com/rails/rails/blob/v5.0.0.1/actionpack/lib/action_dispatch/http/mime_types.rb>`_
 DEFAULT_TYPES = {
-    'all': ['*/*'],
-    'text': ['text/plain; charset=utf-8', 'txt'],
-    'html': ['text/html; charset=utf-8'],
-    'xhtml': ['application/xhtml+xml', 'xhtml'],
-    'xml': ['application/xml', 'text/xml', 'application/x-xml'],
-    'js': ['text/javascript', 'application/javascript',
-           'application/x-javascript'],
-    'css': ['text/css'],
-    'ics': ['text/calendar'],
-    'csv': ['text/csv'],
-    'rss': ['application/rss+xml'],
-    'atom': ['application/atom+xml'],
-    'yaml': ['application/x-yaml', 'text/yaml'],
+    # (type, alias, ...): [content_type, ...]
+    ('all',): ['*/*'],
+    ('text', 'txt'): ['text/plain; charset=utf-8'],
+    ('html',): ['text/html; charset=utf-8'],
+    ('xhtml',): ['application/xhtml+xml'],
+    ('js',): ['text/javascript',
+              'application/javascript',
+              'application/x-javascript'],
+    ('css',): ['text/css'],
+    ('ics',): ['text/calendar'],
+    ('csv',): ['text/csv'],
+    ('vcf',): ['text/vcard'],
+
+    ('xml',): ['application/xml', 'text/xml', 'application/x-xml'],
+    ('rss',): ['application/rss+xml'],
+    ('atom',): ['application/atom+xml'],
+    ('yaml', 'yml'): ['application/x-yaml', 'text/yaml'],
     # just like Rails
-    'multipart_form': ['multipart/form-data'],
-    'url_encoded_form': ['application/x-www-form-urlencoded'],
+    ('multipart_form',): ['multipart/form-data'],
+    ('url_encoded_form',): ['application/x-www-form-urlencoded'],
     # http://www.ietf.org/rfc/rfc4627.txt
-    'json': ['application/json', 'text/x-json']
+    # http://www.json.org/JSONRequest.html
+    ('json',): ['application/json', 'text/x-json', 'application/jsonrequest'],
+
+    ('pdf',): ['application/pdf'],
+    ('zip',): ['application/zip'],
+    ('gzip', 'gz'): ['application/gzip'],
     # TODO: https://issues.apache.org/jira/browse/COUCHDB-1261
     # 'kml', 'application/vnd.google-earth.kml+xml',
     # 'kmz', 'application/vnd.google-earth.kmz'
@@ -113,8 +122,9 @@ class MimeProvider(object):
         self.funcs_by_key = OrderedDict()
         self._resp_content_type = None

-        for k, v in DEFAULT_TYPES.items():
-            self.register_type(k, *v)
+        for types, v in DEFAULT_TYPES.items():
+            for type_ in types:
+                self.register_type(type_, *v)

     def is_provides_used(self):
         """Checks if any provides function is registered."""
@@ -135,21 +145,33 @@ class MimeProvider(object):

         Predefined types:
             - all: ``*/*``
-            - text: ``text/plain; charset=utf-8``, ``txt``
+            - text: ``text/plain; charset=utf-8``
+            - txt: alias of ``text``
             - html: ``text/html; charset=utf-8``
-            - xhtml: ``application/xhtml+xml``, ``xhtml``
-            - xml: ``application/xml``, ``text/xml``, ``application/x-xml``
+            - xhtml: ``application/xhtml+xml``
             - js: ``text/javascript``, ``application/javascript``,
               ``application/x-javascript``
             - css: ``text/css``
             - ics: ``text/calendar``
             - csv: ``text/csv``
+            - vcf: ``text/vcard``,
+
+            - xml: ``application/xml``, ``text/xml``, ``application/x-xml``
             - rss: ``application/rss+xml``
             - atom: ``application/atom+xml``
             - yaml: ``application/x-yaml``, ``text/yaml``
+            - yml: alias of ``yaml``
+
             - multipart_form: ``multipart/form-data``
             - url_encoded_form: ``application/x-www-form-urlencoded``
-            - json: ``application/json``, ``text/x-json``
+
+            - json: ``application/json``, ``text/x-json``,
+              ``application/jsonrequest``
+
+            - pdf: ``application/pdf``
+            - zip: ``application/zip``
+            - gzip: ``application/gzip``
+            - gz: alias of ``gzip``

         Example:
             >>> register_type('png', 'image/png')

[kxepal]

LGFM

[iblislin]

Patched @ d099408

(TODO: open ticket on JIRA

[iblislin]

How about leveraging standard lib ?
https://docs.python.org/3.7/library/difflib.html#difflib.get_close_matches
or some similar function.

[iblislin]

Oops, forgot to update test case.

Patch:

diff --git a/couchdb/tests/server/mime.py b/couchdb/tests/server/mime.py
index 463aa9b..6587f78 100644
--- a/couchdb/tests/server/mime.py
+++ b/couchdb/tests/server/mime.py
@@ -154,9 +154,10 @@ class ProvidesTestCase(MimeTestCase):
         self.assertEqual(
             sorted(self.provider.mimes_by_key.keys()),
             sorted([
-                'all', 'atom', 'css', 'csv', 'html', 'ics', 'js', 'json',
-                'multipart_form', 'rss', 'text', 'url_encoded_form', 'xhtml',
-                'xml', 'yaml'])
+                'all', 'atom', 'css', 'csv', 'gz', 'gzip', 'html', 'ics',
+                'js', 'json', 'multipart_form', 'pdf', 'rss', 'text', 'txt',
+                'url_encoded_form', 'vcf', 'xhtml', 'xml', 'yaml', 'yml',
+                'zip'])
         )

     def test_provides(self):