Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#10786 closed defect (fixed)

json-rpc search.performSearch results in 'maximum recursion depth exceeded' Error

Reported by: Marco Antonio Islas Cruz Owned by: Marco Antonio Islas Cruz
Priority: normal Component: XmlRpcPlugin
Severity: blocker Keywords: search
Cc: Olemis Lang, Jun Omae Trac Release: 1.0

Description

When trying to use json-rpc search this it results in this error:

{    u'error': {    u'code': -32603,
                    u'message': u'maximum recursion depth exceeded',
                    u'name': u'JSONRPCError'},
     u'id': None,
     u'result': None}

Data used in the query (althought it ends with the error if the search returns something):

{   'method': 'search.performSearch', 'params': ['proxy']}

As I've said, this error appears if something is found by the search and makes the json-rpc call unusable.

Attachments (2)

ticket10786-xmlrpc-r12546.diff (2.2 KB) - added by Jun Omae 4 years ago.
ticket10786-with-tests-xmlrpc-r12546.diff (5.6 KB) - added by Jun Omae 4 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 4 years ago by Marco Antonio Islas Cruz

The problem here seems to be the TracRpcJSONEncoder json encoder, it helps by adding a clue of binary and datetime types by returning a jsonclass list with the first element being the clue and the second a string representation of the element. But when TracRpcJSONEncoder doesn't know how to handle data it passes it to JSONEncoder which for some reason is doing a recursión.

My proposed fix is, instead of just calling to JSONEncoder, catch if there is any error, if there is just return a representation of the element (repr(obj)).

        def default(self, obj):
            if isinstance(obj, datetime.datetime):
                # http://www.ietf.org/rfc/rfc3339.txt
                return {'__jsonclass__': ["datetime",
                                obj.strftime('%Y-%m-%dT%H:%M:%S')]}
            elif isinstance(obj, Binary):
                return {'__jsonclass__': ["binary",
                                obj.data.encode("base64")]}
            elif obj is empty:
                return ''
            else:
                try:
                    return json.JSONEncoder(self, obj)
                except:
                    return repr(obj)

I'm currently using Trac 1.0 and I'm not sure if this bug is present in previous versions but it seems that json can't encode a <fragment> object which seems to be part of the result to be returned to the user.

comment:2 in reply to:  1 ; Changed 4 years ago by Olemis Lang

Keywords: search added

Replying to markuzmx:

The problem here seems to be the TracRpcJSONEncoder json encoder, it helps by adding a clue of binary and datetime types by returning a jsonclass list with the first element being the clue and the second a string representation of the element. But when TracRpcJSONEncoder doesn't know how to handle data it passes it to JSONEncoder which for some reason is doing a recursión.

Could you please mention Python version ? First thing needed is to identify whether this is a bug related to XmlRpcPlugin or otherwise json module (or equivalent) .

comment:3 in reply to:  2 Changed 4 years ago by anonymous

Replying to olemis:

Could you please mention Python version ? First thing needed is to identify whether this is a bug related to XmlRpcPlugin or otherwise json module (or equivalent) .

I'm running Python 2.6.5

comment:4 Changed 4 years ago by Odd Simon Simonsen

Oh, so some part of the search implementation returns items of type Fragment that we don't handle correctly? That isn't surprising, but what versions of Trac this affects is hard to say without testing. The trac.search backend is a terrible implementation from an RPC standpoint as it entangles the search results with its representation by returning HTML fragments.

Testing RPC search is currently not done. There should be a tracrpc/tests/search.py testing XML-RPC and JSON-RPC implementations of searching to make sure it works...

markuzmx, if you can be tempted to contribute a patch I've written an RPC development guide that should get you running tests in no time:

https://www.coderesort.com/u/simon/blog/2010/09/23/tracrpc_development

comment:5 Changed 4 years ago by Marco Antonio Islas Cruz

Owner: changed from Odd Simon Simonsen to Marco Antonio Islas Cruz
Status: newassigned

Of course, I'll take care of the tests ;-)

Changed 4 years ago by Jun Omae

comment:6 in reply to:  4 ; Changed 4 years ago by Jun Omae

Cc: Jun Omae added

Replying to osimons:

Oh, so some part of the search implementation returns items of type Fragment that we don't handle correctly?

Reproduced! And I got SEGV on Python 2.4.3 with simplejson 2.0.9....

TicketModule.get_search_results() returns Fragment (or maybe LazyProxy from tag_?) instances as title. Also, json_rpc.py don't handle them. See http://trac.edgewall.org/browser/branches/0.12-stable/trac/ticket/web_ui.py?rev=10639&marks=214-218#L186.

The _normalize_xml_output in xml_rpc.py handles correctly Fragment instances. See xmlrpcplugin/trunk/tracrpc/xml_rpc.py@9360#L178.

I created the patch, attachment:ticket10786-xmlrpc-r12546.diff.

comment:7 in reply to:  6 ; Changed 4 years ago by Olemis Lang

Replying to jun66j5:

Replying to osimons:

Oh, so some part of the search implementation returns items of type Fragment that we don't handle correctly?

Reproduced! And I got SEGV on Python 2.4.3 with simplejson 2.0.9....

TicketModule.get_search_results() returns Fragment (or maybe LazyProxy from tag_?) instances as title. Also, json_rpc.py don't handle them. See http://trac.edgewall.org/browser/branches/0.12-stable/trac/ticket/web_ui.py?rev=10639&marks=214-218#L186.

The _normalize_xml_output in xml_rpc.py handles correctly Fragment instances. See xmlrpcplugin/trunk/tracrpc/xml_rpc.py@9360#L178.

I created the patch, attachment:ticket10786-xmlrpc-r12546.diff.

I'll try to review this in the next few days and attempt to write some test cases for search RPC handler (if possible) .

Changed 4 years ago by Jun Omae

comment:8 in reply to:  7 ; Changed 4 years ago by Jun Omae

Replying to olemis:

I'll try to review this in the next few days and attempt to write some test cases for search RPC handler (if possible) .

Thanks, Olemis! I added minimum unit tests for json-rpc with fragment and search, ticket10786-with-tests-xmlrpc-r12546.diff. Could you please review the latest patch?

comment:9 in reply to:  8 ; Changed 4 years ago by Olemis Lang

Replying to jun66j5:

Replying to olemis:

I'll try to review this in the next few days and attempt to write some test cases for search RPC handler (if possible) .

Thanks, Olemis! I added minimum unit tests for json-rpc with fragment and search, ticket10786-with-tests-xmlrpc-r12546.diff. Could you please review the latest patch?

Trac version Results
0.12 ok
1.0 http://pastebin.com/wKqeJivW
trunk http://pastebin.com/yEjMttXH

comment:10 in reply to:  9 ; Changed 4 years ago by Jun Omae

Replying to olemis:

|| trunk || http://pastebin.com/yEjMttXH ||

The failure in test_getActions brings the other two failures. Also, the failure happens with xmlrpcplugin/trunk-r12546, too. Then, that is another issue.

Anyway, I made test_fragment_in_search more robust. See https://github.com/jun66j5/xmlrpcplugin/compare/trunk...ticket10786 (diff).

I'll try to fix the test_getActions later.

comment:11 Changed 4 years ago by Odd Simon Simonsen

Superb, Jun. Looks good to me!

Olemis, I seem to remember you having reported that particular test_getActions failure before in some other context, but I have yet to replicate it. At my end tests are passing for all ~recent Trac branches (0.11++). Hard to research for me without actual test failure...

Jun, I'm ready to integrate your changes. Or should I perhaps set you up with write permission to the plugin so that you can push the changes yourself? Your involvement is always appreciated! ;-)

comment:12 Changed 4 years ago by Jun Omae

Resolution: fixed
Status: assignedclosed

(In [13193]) XmlRpcPlugin: Fixed 'maximum recursion depth exceeded' error on search.performSearch with JSON-RPC. Closes #10786.

comment:13 Changed 4 years ago by Jun Omae

Thanks, Simon and Olemis! I've directly pushed in [13193].

comment:14 Changed 4 years ago by Odd Simon Simonsen

Goodie. But the tracrpc/tests/search.py file seems not to have quite made it across from Git til Subversion. No file added in [13193].

comment:15 Changed 4 years ago by Jun Omae

Ouch. Added in [13194], sorry.

comment:16 in reply to:  10 ; Changed 4 years ago by Olemis Lang

Replying to jun66j5:

Replying to olemis:

|| trunk || http://pastebin.com/yEjMttXH ||

The failure in test_getActions brings the other two failures. Also, the failure happens with xmlrpcplugin/trunk-r12546, too. Then, that is another issue.

yes , now I noticed ... :-/

Anyway, I made test_fragment_in_search more robust. See https://github.com/jun66j5/xmlrpcplugin/compare/trunk...ticket10786 (diff).

I'll try to fix the test_getActions later.

@jun66j5 : So this means you are also experiencing the same issue regarding test_getActions ?

If so , what's the version of your python interpreter ? /me testing with python=2.6

comment:17 in reply to:  16 ; Changed 4 years ago by Jun Omae

Replying to olemis:

@jun66j5 : So this means you are also experiencing the same issue regarding test_getActions ?

Yes. I get the same failures as http://pastebin.com/wKqeJivW with 1.0-stable.

If so , what's the version of your python interpreter ? /me testing with python=2.6

Python 2.5 and 2.6. For Python 2.4, tracrpc/tests/api.py has an syntax error. Also, All tests pass on Trac 0.12 - 0.12.5, 0.12-stable and 1.0 - 1.0.1 with Python 2.5 and 2.6.

comment:18 in reply to:  17 Changed 4 years ago by Olemis Lang

Replying to jun66j5:

Replying to olemis:

@jun66j5 : So this means you are also experiencing the same issue regarding test_getActions ?

Yes. I get the same failures as http://pastebin.com/wKqeJivW with 1.0-stable.

ok , please see #11109 for further details ;)

[...]

For Python 2.4, tracrpc/tests/api.py has an syntax error.

I'm of the opinion that support for python=2.4 should be removed in recent versions of the plugin ... objections ?

[...]

comment:19 Changed 4 years ago by Odd Simon Simonsen

It is my intention to support all versions of Python supported by the corresponding versions of Trac. As long as the plugin (trunk) still supports 0.11 and 0.12, Python2.4 is a requirement as I see it. In theory even 2.3 is supported, but it's been a long time since I've had that installed ;-)

I intend to branch for future major incompatible changes, but until actually required I'm happy to add the necessary compat code to make this work across versions. I very much prefer one codebase, and don't subscribe to the opinion that plugins should follow Trac branching. Breaking changes to the RPC API or implementations are more valid reasons to branch, IMO. And if we start branching for each combination of major Trac version + internal RPC changes, everything would quickly become a confusing mess...

However, as correctly spotted, the plugin did indeed have Python 2.4 issues - but only in the tests, not in the actual RPC code/handlers. I've fixed it in [13201].

comment:20 in reply to:  19 Changed 4 years ago by Olemis Lang

Replying to osimons:

It is my intention to support all versions of Python supported by the corresponding versions of Trac. As long as the plugin (trunk) still supports 0.11 and 0.12, Python2.4 is a requirement as I see it.

Considering this , I've just changed my mind on this subject . You are right .

I intend to branch for future major incompatible changes, but until actually required I'm happy to add the necessary compat code to make this work across versions. I very much prefer one codebase, and don't subscribe to the opinion that plugins should follow Trac branching. Breaking changes to the RPC API or implementations are more valid reasons to branch, IMO. And if we start branching for each combination of major Trac version + internal RPC changes, everything would quickly become a confusing mess...

of course , I get your point , and agree with you .

However, as correctly spotted, the plugin did indeed have Python 2.4 issues - but only in the tests, not in the actual RPC code/handlers. I've fixed it in [13201].

:)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Marco Antonio Islas Cruz.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.