Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#325 closed defect (fixed)

Glitch in web interface handler

Reported by: skaplun Owned by: jcaffaro
Priority: major Milestone:
Component: WebSearch Version:
Keywords: opensearch Cc:

Description

[...]
The following problem occurred on <http://inspirebeta.net>; (CDS Invenio 0.99.90.20100823)

2010-10-26 07:31:34 -> AttributeError: 'function' object has no attribute '_traverse'

User details

uri: </opensearchdescription/?>

Traceback details

Traceback (most recent call last):
File "/usr/lib/python2.4/site-packages/invenio/webinterface_handler_wsgi.py", line 367, in application
ret = invenio_handler(req)
File "/usr/lib/python2.4/site-packages/invenio/webinterface_handler.py", line 331, in _profiler
return _handler(req)
File "/usr/lib/python2.4/site-packages/invenio/webinterface_handler.py", line 373, in _handler
return root._traverse(req, path, False, guest_p)
File "/usr/lib/python2.4/site-packages/invenio/webinterface_handler.py", line 223, in _traverse
return obj._traverse(req, path, do_head, guest_p)
AttributeError: 'function' object has no attribute '_traverse'
Locals by frame, innermost last
[...]

Attachments (2)

0001-WebStyle-fixed-traversal-of-final-URLs.patch (1.2 KB) - added by jcaffaro 3 years ago.
Patch (master)
0001-WebStyle-fixed-traversal-of-final-URLs.2.patch (1.2 KB) - added by jcaffaro 3 years ago.
Patch (maint)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by jcaffaro

  • Priority changed from trivial to major

Same would occur for URLs:

/openurl/
/logout_SSO_hook/
/record/XXX/comments/display/
/record/XXX/comments/add/
/record/XXX/comments/vote/
/record/XXX/comments/report/
/record/XXX/comments/index/
/record/XXX/comments/attachments/
/record/XXX/comments/subscribe/
/record/XXX/comments/unsubscribe/
/youralerts/input/
/youralerts/modify/
/youralerts/list/
/youralerts/add/
/youralerts/update/
/youralerts/remove/
/yourmessages/display/
/yourmessages/write/
/yourmessages/send/
/yourmessages/delete/
/yourmessages/delete_all/
/yourmessages/display_msg/
/youraccount/display/
/youraccount/edit/
/youraccount/change/
/youraccount/send_email/
/youraccount/lost/
/youraccount/youradminactivities/
/youraccount/access/
/youraccount/delete/
/youraccount/logout/
/youraccount/login/
/youraccount/register/
/youraccount/resetpassword/
/yourbaskets/display_item/
/yourbaskets/display/
/yourbaskets/search/
/yourbaskets/write_note/
/yourbaskets/save_note/
/yourbaskets/delete_note/
/yourbaskets/add/
/yourbaskets/delete/
/yourbaskets/modify/
/yourbaskets/edit/
/yourbaskets/edit_topic/
/yourbaskets/create_basket/
/yourbaskets/display_public/
/yourbaskets/list_public_baskets/
/yourbaskets/subscribe/
/yourbaskets/unsubscribe/
/yourbaskets/write_public_note/
/yourbaskets/save_public_note/
/yourbaskets/attachments/
/yourgroups/display/
/yourgroups/create/
/yourgroups/join/
/yourgroups/leave/
/yourgroups/edit/
/yourgroups/members/
[...]

Should probably be fixed at the level of the URL handler.

comment:2 Changed 3 years ago by skaplun

Ups! Definitively. This looks like a regression with WSGI then, because with mod_python this paths where handled correctly.

comment:3 Changed 3 years ago by jcaffaro

I wanted to test something of that kind:

diff --git a/modules/webstyle/lib/webinterface_handler.py b/modules/webstyle/lib/webinterface_handler.py
index c396e65..3a76f64 100644
--- a/modules/webstyle/lib/webinterface_handler.py
+++ b/modules/webstyle/lib/webinterface_handler.py
@@ -220,7 +220,11 @@ class WebInterfaceDirectory(object):
         # resolving, otherwise call the method as it is our final
         # renderer. We even pass it the parsed form arguments.
         if path:
-            return obj._traverse(req, path, do_head, guest_p)
+            if hasattr(obj, '_traverse'):
+                return obj._traverse(req, path, do_head, guest_p)
+            else:
+                raise apache.SERVER_RETURN, apache.HTTP_NOT_FOUND

but it looks more like a new behavior than a regression fix, so if you think that it got broken by the migration to WSGI, we might have to check somewhere else.

comment:4 Changed 3 years ago by jcaffaro

These URLs were already not properly handled before the migration to WSGI.
For eg (0.99.2): https://invenio-demo.cern.ch/youraccount/display/

Changed 3 years ago by jcaffaro

Patch (master)

Changed 3 years ago by jcaffaro

Patch (maint)

comment:5 Changed 3 years ago by jcaffaro

  • Status changed from new to in_merge

I have attached two patches, one for master and one for maint (0.99.2) branches.
(Note that http://invenio-demo.cern.ch has now been patched...)

comment:6 Changed 3 years ago by skaplun

I just wonder, with your patch you return a 404. Would indeed implicitly redirecting /foo/ to /foo (when _traverse does not exist) be the wrong thing to do? I guess it might be a problem for OAI, but are there other cases?

comment:7 Changed 3 years ago by jcaffaro

I also wondered about redirecting /foo/ to /foo, but I thought it was conceptually better to keep the distinction between pages (function) and directories (WebInterfaceDirectory). Indeed redirecting /foo/ to /foo would be similar to having a directory with an implicit "index" page inside (in these case "index" would be the only possible page).

In the case of a WebInterfaceDirectory, one might expect that /foo/index would be the same as /foo/ or /foo. That would not be the case with my patch, even with the redirection from /foo/ to /foo: we would have to also implement the redirection /foo/index to /foo (although we have not always been consistent regarding this across all modules...). I am not really foo about that.

Looking at the above list of URLs, they should be more like "leaf" pages than directories to me. Does one expect that /foo.html/ is redirected to /foo.html, or return a 404?

In the end, one or the other solution does not seen to change too much for the end-user, nor the developer. If we change our mind later, it will be easier to add the redirection, than removing it...

But I am sure there are plenty of reasons to choose one or the other way :-)
It is indeed the right moment to wonder if one of the solutions would have serious, annoying consequences for further developments...

comment:8 Changed 3 years ago by jcaffaro

  • Summary changed from Glitch in opensearch web handler to Glitch in web interface handler

comment:9 Changed 3 years ago by simko

Note that /search/ works well and serves the same content as /search. Maybe we should do the same for others like /yourbaskets/add/. Though this introduces non-canonicality, which is not good.

In the case of /somefacility/someaction, I agree that it makes sense to return 404 for the trailing slash. In the case of /somefacility only, it may be better to redirect, because people are generally used to that, e.g. compare http://directory.google.com/Top/Computers/Software/ and http://directory.google.com/Top/Computers/Software. In other words, for our URLs /somefacility/someaction, like /yourbaskets/add or /yourbaskets/display, the first one looks like a directory and the second one looks like a page, and I guess I'd propose to interpret trailing slashes for all-but-last components of the URL, and return 404 for the last one.

Seems similar to Jerome's take on the matter.

comment:10 Changed 3 years ago by Jerome Caffaro <jerome.caffaro@…>

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

In [4070b7e3ad372b830a2d2f4841d97049b0ccce26]:

WebStyle: fixed traversal of final URLs

  • Only try to traverse URL "parts" which have the _traverse() function available. Others return HTTP_NOT_FOUND. (closes #325)

comment:11 Changed 3 years ago by Jerome Caffaro <jerome.caffaro@…>

In [4070b7e3ad372b830a2d2f4841d97049b0ccce26]:

WebStyle: fixed traversal of final URLs

  • Only try to traverse URL "parts" which have the _traverse() function available. Others return HTTP_NOT_FOUND. (closes #325)
Note: See TracTickets for help on using tickets.