- Notifications
You must be signed in to change notification settings - Fork84
Query Server package code review#286
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
- We use the `SimpleQueryServer` for `main` entry- Add config handler in `BaseQueryServer`- `BaseQueryServer.serve_forever` API- Test cases for query server includedAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>- pep8 coding style checked with $ pep8 --max-line-length=100 --show-source --first- using new style format string -- `str.format`Reference:djc#268See Also:djc#276
The dependency chain:(The "*" mark denote the function/obj included in this commit)compiler.compile_func | +- compiler.require | |* | +- compiler.resolve_module | | | +- compiler.maybe_export_egg | | | | | +- compiler.maybe_b64egg | | | | | +- compiler.import_b64egg | | | +- compiler.maybe_export_cached_egg | | | +- compiler.maybe_compile_function | | | +- compiler.maybe_export_bytecode | | | +- compiler.cache_to_ddoc | +- compiler.compile_to_bytecode- Test cases includedAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>- pep8 coding style checked with $ pep8 --max-line-length=100 --show-source --firstReference:djc#268See Also:djc#276
The dependency chain:(The "*" mark denote the function/obj included in this commit)compiler.compile_func | +- compiler.require | | | +- compiler.resolve_module | |* | +- compiler.maybe_export_egg* | | |* | | +- compiler.maybe_b64egg* | | |* | | +- compiler.import_b64egg | | | +- compiler.maybe_export_cached_egg | | | +- compiler.maybe_compile_function | | | +- compiler.maybe_export_bytecode | | | +- compiler.cache_to_ddoc | +- compiler.compile_to_bytecodeAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>Reference:djc#268See Also:djc#276
Helper functions for `compiler.require`The dependency chain:(The "*" mark denote the function/obj included in this commit)compiler.compile_func | +- compiler.require | | | +- compiler.resolve_module | | | +- compiler.maybe_export_egg | | | | | +- compiler.maybe_b64egg | | | | | +- compiler.import_b64egg | |* | +- compiler.maybe_export_cached_egg* | |* | +- compiler.maybe_compile_function* | |* | +- compiler.maybe_export_bytecode* | |* | +- compiler.cache_to_ddoc | +- compiler.compile_to_bytecodeAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>Reference:djc#268See Also:djc#276
- Test cases includedThe dependency chain:(The "*" mark denote the function/obj included in this commit)compiler.compile_func |* +- compiler.require | | | +- compiler.resolve_module | | | +- compiler.maybe_export_egg | | | | | +- compiler.maybe_b64egg | | | | | +- compiler.import_b64egg | | | +- compiler.maybe_export_cached_egg | | | +- compiler.maybe_compile_function | | | +- compiler.maybe_export_bytecode | | | +- compiler.cache_to_ddoc | +- compiler.compile_to_bytecodeAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>Reference:djc#268See Also:djc#276
-Test cases includedThe dependency chain:(The "*" mark denote the function/obj included in this commit)compiler.compile_func | +- compiler.require | | | +- compiler.resolve_module | | | +- compiler.maybe_export_egg | | | | | +- compiler.maybe_b64egg | | | | | +- compiler.import_b64egg | | | +- compiler.maybe_export_cached_egg | | | +- compiler.maybe_compile_function | | | +- compiler.maybe_export_bytecode | | | +- compiler.cache_to_ddoc |* +- compiler.compile_to_bytecodeAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>Reference:djc#268See Also:djc#276
Some objects required by `render.show_doc` will be commited later.The dependency chain:render.show_doc | +- mime.MimeProvider | +- mime.MimeProvider.register_type | +- render.response_with | | | +- mime.MimeProvider.resp_content_type | +- render.render_function | | | +- render.maybe_wrap_responseAuthor: Alexander Shorin <kxepal@gmail.com>Patched by: Iblis Lin <iblis@hs.ntnu.edu.tw>Reference:djc#268See Also:djc#276
Also, add `render` test suite.The render TestCase require the module `couchdb.server.mock`.render.show_doc | +- mime.MimeProvider | +- mime.MimeProvider.register_type | +- render.response_with | | | +- mime.MimeProvider.resp_content_type |* +- render.render_function* | |* | +- render.maybe_wrap_responseAuthor: Alexander Shorin <kxepal@gmail.com>Reference:djc#268See Also:djc#276
else: | ||
msg='Multiple functions are defined. Only one is allowed.' | ||
elifnotisinstance(item,ModuleType): | ||
msg='Only functions could be defined at top level namespace' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was wandering ... why do we make this limit?
We can simply pop the var fromglobals_
and insert it intocontext
, then let user enjoy the global var.
( I think user imported module can be global, also)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.pyindex 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 funcdiff --git a/couchdb/tests/server/compiler.py b/couchdb/tests/server/compiler.pyindex 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'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 dodef main(math=math)
in your everydays code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Worth to add another assertion that we don't pollute next function compile/call cycle by imports from the previous one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.pyindex 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 funcdiff --git a/couchdb/tests/server/compiler.py b/couchdb/tests/server/compiler.pyindex 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'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ha, forgot 'manaully gc'. That's some kind of joking words, but text message is hard to express it. (Maybe i should add adelete 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 thishidden feature side effect on module.
return dedent(getsource(fun)) | ||
elif isinstance(fun, util.strbase): | ||
return fun | ||
raise TypeError('Function object or source string expected, got %r' % fun) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Err... It seems that it blew up when user put some builtin function here.
e.g.:
In [20]:sortedOut[20]:<functionsorted>In [21]:type(sorted)Out[21]:builtin_function_or_methodIn [22]:inspect.getsource(sorted)---------------------------------------------------------------------------TypeErrorTraceback (mostrecentcalllast)<ipython-input-22-c9deb9140b13>in<module>()---->1inspect.getsource(sorted)/home/iblis/py/py36/lib/python3.6/inspect.pyingetsource(object)925orcodeobject.Thesourcecodeisreturnedasasinglestring.An926OSErrorisraisedifthesourcecodecannotberetrieved."""--> 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."""913object=unwrap(object)-->914lines,lnum=findsource(object)915916ifismodule(object):/home/iblis/py/py36/lib/python3.6/inspect.pyinfindsource(object)725israisedifthesourcecodecannotberetrieved.""" 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 """-->643filename=getfile(object)644all_bytecode_suffixes=importlib.machinery.DEBUG_BYTECODE_SUFFIXES[:]645all_bytecode_suffixes+=importlib.machinery.OPTIMIZED_BYTECODE_SUFFIXES[:]/home/iblis/py/py36/lib/python3.6/inspect.pyingetfile(object)623returnobject.co_filename624raiseTypeError('{!r} is not a module, class, method, '-->625'function, traceback, frame, or code object'.format(object))626627defgetmodulename(path):TypeError:<built-infunctionsorted>isnotamodule,class,method,function,traceback,frame,orcodeobject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I know. Same story with lambdas. Basically, both are not suitable for any kind of design function, so no reason to care about.
I'm going to attendPyCon TW this weekend. Maybe disappear about 3 days.... |
Cool! Have fun there! (: |
server.state['query_config'].update(config) | ||
ifserver.version>= (1,1,0): | ||
server.state['view_lib']='' | ||
returnTrue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We initserver.state
in the constructor ofBaseQueryServer
.
self._state= {'view_lib':None,'line_length':0,'query_config': {},'functions': [],'functions_src': [],'row_line': {} }
But in thisreset
function, we do not reset the value ofline_length
androw_line
.
Aboutline_length
: I cannot find any function need it. Is this variable an unimplemented feature?
Followinggrep
shows that only docstring mensioned it.
It's the partner ofreduce_limilt
, but, thereduce_limit
cannot be configured from commandline optoin.
Isreduce_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 longerviews.py: as state.line_length and reduce_limit is enabledin state.query_configviews.py-"""--views.py- If any Python exception occurs or reduce output is twice longerviews.py: as state.line_length and reduce_limit is enabled in state.query_configviews.py-"""Binary file views.pyc matches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 . This patch removed thestate.line_length
and change the docstring ofreduce
andrereduce
diff --git a/couchdb/server/__init__.py b/couchdb/server/__init__.pyindex 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.pyindex 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 Nice catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm..next issues: Do we need to expose thereduce_limilt
to command line? If yes, what do we expect when thereset
is invoked?.(reset
cleanup thestate['query_config']
, where thereduce_limit
live.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reset is a command which server sends to QS. Mostly everytime when it tries to execute a view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep. The main concern in my question is that "Ifstate
get reset again and again, the value ofreduce_limilt
will be volatile. Should this value be basically consist?"
I guessreduce_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 inBaseQueryServer
defis_reduce_limited(self):"""Checks if output of reduce function is limited."""returnself.state['query_config'].get('reduce_limit',False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ha! Ok, i miss the doc 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The patch aboutstate.line_length
was committed @a2ff50d.
server.state['query_config']['foo']='bar' | ||
state.reset(server, {'foo':'baz'}) | ||
self.assertEqual(server.state['query_config'], {'foo':'baz'}) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add test case about resettingview_lib
.
diff --git a/couchdb/server/state.py b/couchdb/server/state.pyindex 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 Truediff --git a/couchdb/tests/server/state.py b/couchdb/tests/server/state.pyindex 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()
I have to get through my final exam 😭 |
couchdb/tests/server/views.py Outdated
' pass' | ||
) | ||
doc = {'_id': 'foo'} | ||
views.map_doc(self.server, doc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good to check output ofmap_doc
?
diff --git a/couchdb/tests/server/views.py b/couchdb/tests/server/views.pyindex 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 inhttp://docs.couchdb.org/en/1.6.1/query-server/protocol.html#map-doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
comitted @f91a237
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
have no idea why the build failed ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I add the verbose flag but the error gone.
https://travis-ci.org/iblis17/couchdb-python/builds/139946573
It seems that it failed randomly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
😆 Heisenbug.
I am trying to press rebuild button again and again now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
e71e522
toea10798
Compare# <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>`_ | ||
DEFAULT_TYPES= { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Time to update this list?
https://github.com/rails/rails/blob/v5.0.0.1/actionpack/lib/action_dispatch/http/mime_types.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea!
couchdb/server/mime.py Outdated
'all': ['*/*'], | ||
'text': ['text/plain; charset=utf-8','txt'], | ||
'html': ['text/html; charset=utf-8'], | ||
'xhtml': ['application/xhtml+xml','xhtml'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'xhtml': ['application/xhtml+xml','xhtml'],^^^^^^whyduplicate`xhtml`here?
And in case oftext
'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'],
makeMimeProvider.register_type
create two entries ofmimes_by_key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good idea. May be JIRA is on maintenance or something? Have you change your keyboard layout before typing a password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
hmm,i just reset my password again... now encounterthis. And i changed the browser... but still cannot pass captcha. 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
JIRA isn't on maintenance...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 oftext
.
diff --git a/couchdb/server/mime.py b/couchdb/server/mime.pyindex 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')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGFM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Patched @d099408
(TODO: open ticket on JIRA
returnfitness_and_quality(mimetype,ranges) | ||
defbest_match(supported,header): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How about leveraging standard lib ?
https://docs.python.org/3.7/library/difflib.html#difflib.get_close_matches
or some similar function.
'all','atom','css','csv','html','ics','js','json', | ||
'multipart_form','rss','text','url_encoded_form','xhtml', | ||
'xml','yaml']) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, forgot to update test case.
Patch:
diff --git a/couchdb/tests/server/mime.py b/couchdb/tests/server/mime.pyindex 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):
Let's review it commit-by-commit!
Some highlight:
__main__.py
will invokeserver.__init__.SimpleQueryServer
SimpleQueryServer
support some legacy query protocol. Are we still need them?Here is the snippet about the command support.