Modify

Opened 2 years ago

Closed 19 months ago

Last modified 19 months ago

#10786 closed defect (fixed)

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

Reported by: markuzmx Owned by: markuzmx
Priority: normal Component: XmlRpcPlugin
Severity: blocker Keywords: search
Cc: olemis, jun66j5 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 jun66j5 19 months ago.
ticket10786-with-tests-xmlrpc-r12546.diff (5.6 KB) - added by jun66j5 19 months ago.

Download all attachments as: .zip

Change History (22)

comment:1 follow-up: Changed 2 years ago by 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.

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 ; follow-up: Changed 2 years ago by olemis

  • 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 2 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 follow-up: Changed 2 years ago by osimons

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 2 years ago by markuzmx

  • Owner changed from osimons to markuzmx
  • Status changed from new to assigned

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

Changed 19 months ago by jun66j5

comment:6 in reply to: ↑ 4 ; follow-up: Changed 19 months ago by jun66j5

  • Cc jun66j5 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 ; follow-up: Changed 19 months ago by olemis

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 19 months ago by jun66j5

comment:8 in reply to: ↑ 7 ; follow-up: Changed 19 months ago by 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?

comment:9 in reply to: ↑ 8 ; follow-up: Changed 19 months ago by olemis

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 ; follow-up: Changed 19 months ago by 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.

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 19 months ago by osimons

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 19 months ago by jun66j5

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:13 Changed 19 months ago by jun66j5

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

comment:14 Changed 19 months ago by osimons

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 19 months ago by jun66j5

Ouch. Added in [13194], sorry.

comment:16 in reply to: ↑ 10 ; follow-up: Changed 19 months ago by olemis

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 ; follow-up: Changed 19 months ago by 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.

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 19 months ago by olemis

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 follow-up: Changed 19 months ago by 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. 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 19 months ago by olemis

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].

:)

Add Comment

Modify Ticket

Action
as closed The owner will remain markuzmx.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.