Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.1.4
    • Fix Version/s: 3.3.1
    • Component/s: core
    • Labels:
      None

      Description

      When calling psp.PSP() explicitly to render PSP pages, it will internally setup
      req.form if it determines that the form is accessed by the PSP page.

      Problem is that if you are wanting to trigger psp.PSP() from a publisher function
      any form parameters have already been processed and req.form created. For a
      POST request this is problematic as in doing this it will have consumed all the
      content of the request.

      This means that when the form is processed a second time by psp.PSP(), it will
      find no request content. Thus, the PSP page will only be able to make use of
      GET form parameters and not POST form parameters.

      It would be an improvement if psp.PSP() allowed a instance of util.FieldStorage
      which has previously been created to be passed in through the "form" parameter
      of the constructor. Ie.,

      template = psp.PSP(req,filename=path,vars=settings,form=req.form)
      template.run()

        Activity

        Hide
        grahamd Graham Dumpleton added a comment -

        Alternatively, and probably preferably, psp.PSP() should look explicitly
        for the presence of req.form as may have been left by a prior parsing
        of the form parameters. This would be better than a "form" parameter
        as it would be transparent.

        Show
        grahamd Graham Dumpleton added a comment - Alternatively, and probably preferably, psp.PSP() should look explicitly for the presence of req.form as may have been left by a prior parsing of the form parameters. This would be better than a "form" parameter as it would be transparent.
        Hide
        grahamd Graham Dumpleton added a comment -

        A workaround at present seems though to be to pass in "req.form" explicitly
        through the "vars" dictionary of user settings. This will the take precedence
        over that set up by psp.PSP(). It is still wasteful for psp.PSP() to process
        the form parameters a second time.

        settings.update(

        {"form":req.form}

        )
        template = psp.PSP(req,filename=path,vars=settings)
        template.run()

        Show
        grahamd Graham Dumpleton added a comment - A workaround at present seems though to be to pass in "req.form" explicitly through the "vars" dictionary of user settings. This will the take precedence over that set up by psp.PSP(). It is still wasteful for psp.PSP() to process the form parameters a second time. settings.update( {"form":req.form} ) template = psp.PSP(req,filename=path,vars=settings) template.run()
        Hide
        grahamd Graham Dumpleton added a comment -

        Adding more to this and why PSP should check for req.form and also set req.form
        if it is the first to create the form object, one should look at the set_error_page()
        mechanism in PSP pages. If setting an error page, and it gets triggered, an
        additional instance of the PSP object is created, it will yet again create a unique
        instance of the form object if a reference to it exists in the page. Again, POST
        form parameters will not be preserved.

        Related to the form object, one also needs to be careful with the session object
        which is created by PSP run() method. If PSP pages were being created explicitly
        from a basic content handler or publisher, and the parent was creating a session
        object and the PSP code also tried to reference a session object, the code would
        deadlock on itself. Even if the parent didn't create a session, if the main PSP page
        and an error page both tried to access a session object, it would again deadlock
        on itself.

        Code should perhaps be something like:

        1. does this code use session?
          session = None
          if "session" in code.co_names:
          if not hasattr(req,"session"):
          session = Session.Session(req)
          req.session = session
        1. does this code use form?
          if "form" in code.co_names:
          if not hasattr(req,"form"):
          req.form = util.FieldStorage(req, keep_blank_values=1)
        1. create psp interface object
          psp = PSPInterface(req, self.filename, req.form)

        try:
        global_scope = globals().copy()
        global_scope.update(

        {"req":req.form, "session":req.session, "form":form, "psp":psp}

        )
        global_scope.update(self.vars) # passed in _init_()
        global_scope.update(vars) # passed in run()
        try:
        exec code in global_scope
        req.flush()

        1. the mere instantiation of a session changes it
        2. (access time), so it always has to be saved
          if session is not None:
          session.save()
          except:
          et, ev, etb = sys.exc_info()
          if psp.error_page:
        3. run error page
          psp.error_page.run( {"exception": (et, ev, etb)}

          )
          else:
          raise et, ev, etb
          finally:
          if session is not None:
          session.unlock()

        Show
        grahamd Graham Dumpleton added a comment - Adding more to this and why PSP should check for req.form and also set req.form if it is the first to create the form object, one should look at the set_error_page() mechanism in PSP pages. If setting an error page, and it gets triggered, an additional instance of the PSP object is created, it will yet again create a unique instance of the form object if a reference to it exists in the page. Again, POST form parameters will not be preserved. Related to the form object, one also needs to be careful with the session object which is created by PSP run() method. If PSP pages were being created explicitly from a basic content handler or publisher, and the parent was creating a session object and the PSP code also tried to reference a session object, the code would deadlock on itself. Even if the parent didn't create a session, if the main PSP page and an error page both tried to access a session object, it would again deadlock on itself. Code should perhaps be something like: does this code use session? session = None if "session" in code.co_names: if not hasattr(req,"session"): session = Session.Session(req) req.session = session does this code use form? if "form" in code.co_names: if not hasattr(req,"form"): req.form = util.FieldStorage(req, keep_blank_values=1) create psp interface object psp = PSPInterface(req, self.filename, req.form) try: global_scope = globals().copy() global_scope.update( {"req":req.form, "session":req.session, "form":form, "psp":psp} ) global_scope.update(self.vars) # passed in _ init _() global_scope.update(vars) # passed in run() try: exec code in global_scope req.flush() the mere instantiation of a session changes it (access time), so it always has to be saved if session is not None: session.save() except: et, ev, etb = sys.exc_info() if psp.error_page: run error page psp.error_page.run( {"exception": (et, ev, etb)} ) else: raise et, ev, etb finally: if session is not None: session.unlock()
        Hide
        grahamd Graham Dumpleton added a comment -

        Whoops, got the following code wrong in a couple of ways:

             global_scope.update(

        {"req":req.form, "session":req.session, "form":form, "psp":psp}

        )

        Should be something like:

        global_scope.update(

        {"req":req,"psp":psp}

        )

        if hasattr(req,"form"):
        global_scope["form"] = req.form
        else:
        global_scope["form"] = None

        if hasattr(req,"session"):
        global_scope["session"] = req.session
        else:
        global_scope["session"] = None

        Another thing to be considered is that if PSP object is used from external handler
        and session object is created by external handler, should the PSP class still be
        responsible for saving the session. Ie., should it do:

             if session is not None:
                  session.save()
        elif hasattr(req,"session"):
        req.session.save()

        Show
        grahamd Graham Dumpleton added a comment - Whoops, got the following code wrong in a couple of ways:      global_scope.update( {"req":req.form, "session":req.session, "form":form, "psp":psp} ) Should be something like: global_scope.update( {"req":req,"psp":psp} ) if hasattr(req,"form"): global_scope ["form"] = req.form else: global_scope ["form"] = None if hasattr(req,"session"): global_scope ["session"] = req.session else: global_scope ["session"] = None Another thing to be considered is that if PSP object is used from external handler and session object is created by external handler, should the PSP class still be responsible for saving the session. Ie., should it do:      if session is not None:           session.save() elif hasattr(req,"session"): req.session.save()
        Hide
        nlehuen Nicolas Lehuen added a comment -

        We could also solve the problem by modifying the FieldStorage and Session constructors, so that if the req object already has a 'form' or 'session' attribute, respectively, we just return its value :

        class FieldStorage(object):
        def _new_(cls,req,args,*kw):
        try:
        return req.form
        except AttributeError:
        req.form = object._new_(cls)
        return req.form

        1. the rest of the object is as before

        This way, we can build util.FieldStorage(req) as many time we want, we will always get the same instance.

        Show
        nlehuen Nicolas Lehuen added a comment - We could also solve the problem by modifying the FieldStorage and Session constructors, so that if the req object already has a 'form' or 'session' attribute, respectively, we just return its value : class FieldStorage(object): def _ new _(cls,req, args, *kw): try: return req.form except AttributeError: req.form = object._ new _(cls) return req.form the rest of the object is as before This way, we can build util.FieldStorage(req) as many time we want, we will always get the same instance.
        Hide
        nlehuen Nicolas Lehuen added a comment -

        The comment above is not really true. We have to modify _init_ as well so that the instance does not get initialized more than once.

        Show
        nlehuen Nicolas Lehuen added a comment - The comment above is not really true. We have to modify _ init _ as well so that the instance does not get initialized more than once.
        Hide
        jgallacher Jim Gallacher added a comment -

        Explicitly checking for req.session within PSP.run would also fix an unreported bug, where the calling code uses a Session class which is different from the default.

        def handler(req):
        ...
        sess = Session.FileSession()
        template = psp.PSP(req,filename=path,vars=settings)
        template.run()

        And in class PSP we have:

        def run(self, vars={}):

        code, req = self.code, self.req

        1. does this code use session?
          session = None
          if "session" in code.co_names:
          session = Session.Session(req)

        Currently Session.Session uses dbm as the default session class, so in the above code snippet we end up with 2 different persistent stores for the session data.

        Show
        jgallacher Jim Gallacher added a comment - Explicitly checking for req.session within PSP.run would also fix an unreported bug, where the calling code uses a Session class which is different from the default. def handler(req): ... sess = Session.FileSession() template = psp.PSP(req,filename=path,vars=settings) template.run() And in class PSP we have: def run(self, vars={}): code, req = self.code, self.req does this code use session? session = None if "session" in code.co_names: session = Session.Session(req) Currently Session.Session uses dbm as the default session class, so in the above code snippet we end up with 2 different persistent stores for the session data.
        Hide
        grahamd Graham Dumpleton added a comment -

        Have something else to question in the run() code above. That is, I think
        that doing:

                   if session is not None:
                            session.unlock()

        is questionable and probably shouldn't be done.

        It should be done in the cleanup handler registered by the session object
        itself when it is created if I remember things correctly.

        Especially if we start caching session object in the req object, by unlocking
        it, even if it was created by the PSP object, it makes the session object
        unusable in code that may follow explicit use of PSP object in a handler,
        or in a subsequent log handler.

        Show
        grahamd Graham Dumpleton added a comment - Have something else to question in the run() code above. That is, I think that doing:            if session is not None:                     session.unlock() is questionable and probably shouldn't be done. It should be done in the cleanup handler registered by the session object itself when it is created if I remember things correctly. Especially if we start caching session object in the req object, by unlocking it, even if it was created by the PSP object, it makes the session object unusable in code that may follow explicit use of PSP object in a handler, or in a subsequent log handler.
        Hide
        grahamd Graham Dumpleton added a comment -

        Some of the unfinished session object issues in this entry split out into MODPYTHON-175 and MODPYTHON-176.

        This entry should only be used for req.form/PSP interaction issue.

        Show
        grahamd Graham Dumpleton added a comment - Some of the unfinished session object issues in this entry split out into MODPYTHON-175 and MODPYTHON-176 . This entry should only be used for req.form/PSP interaction issue.
        Hide
        grahamd Graham Dumpleton added a comment -

        Fixed, with PSP code now using FieldStorage object cached as req.form and left there by prior executing code. Allows for better interaction between publisher handler and PSP pages, plus PSP error pages.

        All the session issues described in this issue are dealt with by other problem reports.

        Show
        grahamd Graham Dumpleton added a comment - Fixed, with PSP code now using FieldStorage object cached as req.form and left there by prior executing code. Allows for better interaction between publisher handler and PSP pages, plus PSP error pages. All the session issues described in this issue are dealt with by other problem reports.

          People

          • Assignee:
            grahamd Graham Dumpleton
            Reporter:
            grahamd Graham Dumpleton
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development