env.get_known_users() should also return session.last_visit

[lkraav <leho@…>]

I'm trying to build up something really lightweight to get people visible in my projects without #2456 and various hardcore TH plugins. I'd like to make use of the last_visited attribute to for example only show recently logged in people. But it turns out I gotta jump through hoops to get to it.

For example ​th:AccountManagerPlugin goes straight SQL here:

​th:source:accountmanagerplugin/trunk/acct_mgr/api.py@11554#L277

accountmanagerplugin/trunk/acct_mgr/api.py:277:    def last_seen(self, user =
None):

In 1.0dev source:/trunk/trac/env.py@11073#618​ we have:

618     def get_known_users(self, cnx=None):
619         """Generator that yields information about all known users,
620         i.e. users that have logged in to this Trac environment and
621         possibly set their name and email.
622
623         This function generates one tuple for every user, of the form
624         (username, name, email) ordered alpha-numerically by username.

Why couldn't this function also include the apparently orphanized last_visit attribute right there? Can't be a significant performance penalty, can it?

Oh yeah, going through trac.web.session.SessionAdmin source:/trunk/trac/web/session.py@11012#L398​ doesn't seem to make much sense either. Or…?

[Remy Blank]

What's wrong with _do_list()?

[lkraav <leho@…>]

Well, it prints its results right now. Are you suggesting just subclassing SessionAdmin and overriding _do_list() with my own is the way to go here?

[Remy Blank]

Oh, sorry I misread your sentence. I read "Oh yeah, trac.web.session.SessionAdmin … doesn't seem to make much sense either." (without the "going through"). And I was wondering why that method didn't make much sense ;)

I suppose we could add an argument to Environment.get_known_users() to tell it to return the "last seen" info as well, but then again, the method is nothing else than a simple SQL query, so you could just as well put the query in your own code.

[lkraav <leho@…>]

Well yeah, but as #2456 is still stalled for unknown amount of time and there are other plugins out there looking for this info, why not just include it in core. I don't think much else is planned for the session model anyway, in favor of, let's say anything related to #2456, for any "Pandoras box" concern or am I wrong?

Anyway, I'll go ahead and present a simple patch for get_known_users() soon. It'd definitely be nice not having to write SQL in plugins.

[lkraav <leho@…>]

So rblank, perhaps you can point out some obvious errors for what I have so far. For one, I am not sure how to approach the for + yield part. I'd like to to just add a variable to the for loop list based on last_visit, but I'm not sure that's syntactically possible? Or can you give a list as an argument to

for list in cursor:

trac/env.py

@@ -460 +460 @@
         self.log.info('-' * 32 + ' environment startup [Trac %s] ' + '-' * 32,
                       get_pkginfo(core).get('version', VERSION))
 
-    def get_known_users(self, cnx=None):
+    def get_known_users(self, cnx=None, last_visit=False):
         """Generator that yields information about all known users, i.e. users
         that have logged in to this Trac environment and possibly set their name
         and email.
@@ -470 +470 @@
 
         @param cnx: the database connection; if ommitted, a new connection is
                     retrieved
+        @param last_visit: pass True here if you need to access session.last_visit
         """
         if not cnx:
             cnx = self.get_db_cnx()
         cursor = cnx.cursor()
-        cursor.execute("SELECT DISTINCT s.sid, n.value, e.value "
+
+        last_visit = " s.last_visit," if last_visit else ""
+
+        cursor.execute("SELECT DISTINCT s.sid,{} n.value, e.value"
                        "FROM session AS s "
                        " LEFT JOIN session_attribute AS n ON (n.sid=s.sid "
                        "  and n.authenticated=1 AND n.name = 'name') "
                        " LEFT JOIN session_attribute AS e ON (e.sid=s.sid "
                        "  AND e.authenticated=1 AND e.name = 'email') "
-                       "WHERE s.authenticated=1 ORDER BY s.sid")
-        for username, name, email in cursor:
-            yield username, name, email
+                       "WHERE s.authenticated=1 ORDER BY s.sid"
+                       .format(last_visit))
+
+        for username, name, email, last_visit in cursor:
+            yield username, name, email, last_visit
 
     def backup(self, dest=None):
         """Create a backup of the database.

[lkraav <leho@…>]

Btw is {{{#!diff }}} rendering supposed to work in the instant preview here? I'm definitely not getting a nice colored rendering in the preview, just regular black and white text, although it has proper layout. Fortunately the final rendering seems to be fine.

[Christian Boos]

Replying to lkraav <leho@…>:

Btw is {{{#!diff }}} rendering supposed to work in the instant preview here?

#10470

[Ryan J Ollos <ryano@…>]

Replying to lkraav <leho@…>:

Anyway, I'll go ahead and present a simple patch for get_known_users() soon. It'd definitely be nice not having to write SQL in plugins.

I agree, though I'm not so much against writing SQL in plugin as much as I think anyone looking at the API would question why last_visit is not being returned, and why they have to go out of their way to get that info. Things like that always make me wonder if I'm doing coding correctly against the API or rather just completely missing something key. Its just one of those little things that makes it slightly harder to write a plugin, so it would be nice to improve upon.

[Christian Boos]

Not for 0.12.x.

[Ryan J Ollos]

After #11597 default_handler is also a session attribute, and the number of session attributes is likely to grow in forthcoming releases. Therefore a parameter that specified a list of additional attributes to return might be more generally useful than a last_visit parameter.

[Ryan J Ollos]

With the proposed changes in log:rjollos.git:t10736 you could access the last_visit attribute using:

for session in Session.select(self.env):
   last_visit = session.get('last_visit')

This way we don't need to change Environment.get_known_users, which has a narrowly-defined purpose anyway, and we've exposed an easy way to work with all of the existing sessions and session attributes. We've also moved another SQL query back into the "model" (though Session/DetactedSession doesn't look quite like the other model classes yet, see #11701 and specifically comment:3:ticket:11701 which suggests method parameters for select that would allow for ordering and limiting the number of results returned).

Furthermore, if the changes in #8172 are committed (comment:14:ticket:8172), all of the SQL queries will likely have been moved out of env.py and into the classes that they are more-closely associated with, which together with this ticket would result in a nice overall refactoring of the classes in env.py.

[Peter Suter]

Replying to rjollos:

With the proposed changes in log:rjollos.git:t10736

I suspect performance is a problem. On a synthetic Trac environment with 2500 sessions counting the number of Environment.get_known_users() makes one DB query and takes about 0.02 seconds. After your changes it would make about 5000 DB queries and take about 2 seconds.

for session in Session.select(self.env):

It could be a bit surprising that Session.select() returns DetachedSessions.

This way we don't need to change Environment.get_known_users, which has a narrowly-defined purpose anyway, and we've exposed an easy way to work with all of the existing sessions and session attributes.

All authenticated sessions. But I guess that would be fixed in #11701 with the where parameter?

[Ryan J Ollos]

Thanks for profiling. I'll look into it again soon and see if we can do better with the restructured code.

[Ryan J Ollos]

Retargetting tickets to narrow focus for milestone:1.2. Please move the ticket back to milestone:next-dev-1.1.x if you intend to resolve it by milestone:1.2.

[lkraav <leho@…>]

Is this likely to land some time in the near future? Since lots of changes in the infrastructure, perhaps this has become easier?

[Ryan J Ollos]

It won't be included in milestone:1.2, but it's possible for 1.3.x if someone reworks the patch.