From f00af362dc83a9b968f80491ae914019ec806151 Mon Sep 17 00:00:00 2001
From: Timothy Messier <tim.messier@gmail.com>
Date: Fri, 29 Apr 2022 15:59:39 -0400
Subject: [PATCH] fix(session): Include where clause with order and limit

When applying the limit and order by, the where clause also needs to be
applied to yield the correct results. This can be seen with an example.

Assuming a data set where there are 100 sessions, with 10 sessions
belonging to a user with id `u_user1` and 90 sessions belonging to
`u_user2`. If the constructed query is like:

    select *
    from
        (select public_id from session limit 10) s,
        session_list ss
    where
        s.public_id = ss.public_id
        and s.user_id = 'u_user1'
    order by create_time asc;

The initial sub-query with the limit could return sessions only
belonging to `u_user2`, then it would apply the where clause and exclude
all of the rows.

Instead if the query is:

    with
    session_ids as (
        select public_id
        from sessions as s
        where
            s.user_id = 'u_user1'
        order by create_time asc
        limit 10
    )
    select *
    from session_list
    where
        session_list.public_id in (select * from session_ids)
    order by create_time asc;

The CTE applies the where clause ensuring that only sessions for
`u_user1` are used for the final select from the view.
Note that the CTE is use primarily for readability.

This also adds an index on `session (create_time)` to aide the query
planner in performing the order by.
---
 .../28/01_session_create_time_idx.up.sql      | 11 ++++++++++
 internal/session/query.go                     | 21 +++++++++++++------
 internal/session/repository_session.go        |  4 ++--
 3 files changed, 28 insertions(+), 8 deletions(-)
 create mode 100644 internal/db/schema/migrations/oss/postgres/28/01_session_create_time_idx.up.sql

diff --git a/internal/db/schema/migrations/oss/postgres/28/01_session_create_time_idx.up.sql b/internal/db/schema/migrations/oss/postgres/28/01_session_create_time_idx.up.sql
new file mode 100644
index 000000000..e60b8ffdb
--- /dev/null
+++ b/internal/db/schema/migrations/oss/postgres/28/01_session_create_time_idx.up.sql
@@ -0,0 +1,11 @@
+begin;
+-- Session list queries always use an order by on create_time.
+-- This index can aide in performing this order by.
+create index
+  session_create_time
+on
+  session (create_time);
+
+analyze session;
+
+commit;
diff --git a/internal/session/query.go b/internal/session/query.go
index 541c4b475..7a10af70f 100644
--- a/internal/session/query.go
+++ b/internal/session/query.go
@@ -126,13 +126,22 @@ select public_id, scope_id, user_id from session
 `
 
 	sessionList = `
-select *
-from
-	(select public_id from session %s) s,
-	session_list ss
-where
-	s.public_id = ss.public_id
+with
+session_ids as (
+	select public_id
+	from session as s
+	-- where clause is constructed
 	%s
+	-- order by clause is constructed
+	%s
+	-- limit is constructed
+	%s
+)
+select *
+from session_list
+where
+	session_list.public_id in (select * from session_ids)
+-- order by clause again since order from cte is not guaranteed to be preserved
 %s
 ;
 `
diff --git a/internal/session/repository_session.go b/internal/session/repository_session.go
index f1d5dbcd5..d16edfa21 100644
--- a/internal/session/repository_session.go
+++ b/internal/session/repository_session.go
@@ -319,10 +319,10 @@ func (r *Repository) ListSessions(ctx context.Context, opt ...Option) ([]*Sessio
 
 	var whereClause string
 	if len(where) > 0 {
-		whereClause = " and " + strings.Join(where, " and ")
+		whereClause = " where " + strings.Join(where, " and ")
 	}
 	q := sessionList
-	query := fmt.Sprintf(q, limit, whereClause, withOrder)
+	query := fmt.Sprintf(q, whereClause, withOrder, limit, withOrder)
 
 	rows, err := r.reader.Query(ctx, query, args)
 	if err != nil {