Add tryGetSession (or getSessionOrNull) to SessionStorage interface to prevent unnecessary and wasteful internal Map creation

[tats-u]

getSession always runs createSession regardless of whether the user has a corresponding entry in the cookie.

const session = await sessionStorage.getSession(request.headers.get("cookie"));

It always creates, wraps, and returns a JS's Map even when it will not be used in the caller (e.g. when you check the user is a member registered in a database):

react-router/packages/react-router/lib/server-runtime/sessions.ts

Lines 93 to 140 in 255ac96

	export const createSession: CreateSessionFunction = <
	Data = SessionData,
	FlashData = Data,
	>(
	initialData: Partial<Data> = {},
	id = "",
	): Session<Data, FlashData> => {
	let map = new Map(Object.entries(initialData)) as Map<
	keyof Data | FlashDataKey<keyof FlashData & string>,
	any
	>;
	return {
	get id() {
	return id;
	},
	get data() {
	return Object.fromEntries(map) as FlashSessionData<Data, FlashData>;
	},
	has(name) {
	return (
	map.has(name as keyof Data) ||
	map.has(flash(name as keyof FlashData & string))
	);
	},
	get(name) {
	if (map.has(name as keyof Data)) return map.get(name as keyof Data);
	let flashName = flash(name as keyof FlashData & string);
	if (map.has(flashName)) {
	let value = map.get(flashName);
	map.delete(flashName);
	return value;
	}
	return undefined;
	},
	set(name, value) {
	map.set(name, value);
	},
	flash(name, value) {
	map.set(flash(name), value);
	},
	unset(name) {
	map.delete(name);
	},
	};
	};

Also, a beginner who does not read the document will not know to determine whether the user has a session. The correct way:

// Why the hell do we have to add the extra `.id` here just to check if a session exists?
// We won't add or modify entries in the session here if the user doesn't have one.
if (!session.id) {
  throw redirect("/login", { status: 303 });
}

However, the JSDoc string of SessionStorage.id is just:

A unique identifier for this session.

Note: This will be the empty string for newly created sessions and sessions that are not backed by a database (i.e. cookie-based sessions).

Who will learn that they can use this .id to determine whether the user has a session onlyfrom this description?

To make matters worth, this way cannot be applied to a storage created by createCookieSessionStorage. id is always "" there. You have no way to tell whether the session is missing or tampered/corrupted without the help of createCookie (Cookie). createCookie has no way to leave the default recommended cookie name ("__session") for a session to React Router unlike createCookieSessionStorage or createSessionStorage.

A nullish value (null or undefined) will be more intuitive and versatile if you do not touch the session when the user does not have a cookie:

// Just "if session does not exists". Clearer. Less noise.
if (!session) {
  throw redirect("/login", { status: 303 });
}
// process using the session. TypeScript has erased `| null`.

API:

  getSession: (
    cookieHeader?: string | null,
    options?: ParseOptions
  ) => Promise<Session<Data, FlashData>>;

vs

  tryGetSession: (
    cookieHeader?: string | null,
    options?: ParseOptions
  ) => Promise<Session<Data, FlashData> | null>;

react-router/packages/react-router/lib/server-runtime/sessions.ts

Lines 266 to 270 in 255ac96

	async getSession(cookieHeader, options) {
	let id = cookieHeader && (await cookie.parse(cookieHeader, options));
	let data = id && (await readData(id));
	return createSession(data || {}, id || "");
	},

The current getSession will perform a wasteful operation createSession, i.e. creating an extra, wasteful, and unused object and Map, even if an user has no merit to receive a session data.

vs

 async tryGetSession(cookieHeader, options) { 
   let id = cookieHeader && (await cookie.parse(cookieHeader, options)); 
   let data = id && (await readData(id)); 
   return data ? createSession(data, id) : null;
 }, 

As you might notice, it returns just null without creating new one when no entry.

const session = await tryGetSession(
  request.headers.get("Cookie"),
);
if (session == null) {
  return redirect("/login");
}

// Login page
const session = await tryGetSession(
  request.headers.get("Cookie"),
);
if (session != null) {
  return redirect("/");
}

This will not be a breaking change unless you use third party session libraries. However, an author of a third party session library have to modify their library to implement this method. Unfortunately JavaScript/TypeScript cannot provide default method implementations in an interface unlike Java or C#.

[tats-u]

Implementation:

dev...tats-u:react-router:session-get

[tats-u]

Creating a session when the user has no intention of logging in is a waste of resources.

[brophdawg11]

createSession does not touch the session backing store, it just creates a JS object implementing the Session interface:

react-router/packages/react-router/lib/server-runtime/sessions.ts

Line 93 in 6e0deab

export const createSession: CreateSessionFunction = <

I think what you're probably running into is calling commitSession on these "empty" sessions when you don't need to - that is what would create a new session in your DB:

https://github.com/remix-run/react-router/blob/6e0deabca97fc4816f0ad13d359bf316e04e500f/packages/react-router/lib/server-runtime/sessions.ts#L271-287

You can check session.id to see if it's a new session (id === "") or existing session (id is populated) and potentially use that along with a check on session.data to see if any data has been added to the session before calling commitSession.

[sergiodxa]

You can avoid the DB query by using the cookie. When you create a session storage you always have to pass a cookie object created with createCookie.

Many people just do createSomethingSessionStorage({ cookie: createCookie(opts) }) but you can define the cookie outside and export it.

export const sessionCookie = createCookie(cookieOptions)
export const sessionStorage = createDatabaseSessionStorage({ cookie: sessionCookie, db: YOUR_DB })

Then instead of using sessionStorage.getSession which as you said will try to do a SELECT from your DB, you can use sessionCookie first.

export async function loader({ request }: Route.LoaderArgs) {
  let sessionId = await sessionCookie.parse(request.headers.get("Cookie"))
  if (!sessionId) {
    // user has no session
  } else {
    // user has session, use sessionStorage.getSession here
  }
}

You can put this in a middleware, and store the Session object in the router context if there's a sessionId, this way it will run on every request but only for users with a session.

[brophdawg11]

I am going to need to see a minimal reproduction to provide further insight, I don't understand the issue you are having.

[tats-u]

Cookie.parse is typed as Promise<any>. It is useless while typed as any. We have to compromise on as for now instead:

export async function loader({ request }: Route.LoaderArgs) {
  const session = await sessionStorage.getSession(
    request.headers.get("cookie"),
  );
  if (!session.id) {
    // user has no session
  } else if (!session.has("user")) {
    // session is corrupted
  } else {
    // user has valid session
  }
}

const sessionStorage = createSessionStorage({
  // *snip*
  async readData(id) {
    // It's a pain in the neck having to insert the next line as a part of the boiler plate every single time but we have to do to prevent extra queries
    if (!id) return null;
    const data = await tryRead(id);
    return data ?? null;
  },
  // *snip*
});

It is fine as long as we mention this workaround in the docs.

[brophdawg11]

I still don't follow.

getSession always runs createSession when readData cannot find a corresponding entry.
...
If createSession is called too many times (e.g. by DoS attack), the performance of a database could be degraded by helpless and meaningless read queries to the session table/storage (e.g. SELECT * FROM session WHERE id = '').

createSession simply creates a JS "session" object - it does not do any DB queries, so I don't understand how this is an issue.

You have to add if (!id /* or id === "" */) return null at the beginning of readData to prevent invoking such extra helpless queries

Why? readData is not called if an id doesn't exist:

react-router/packages/react-router/lib/server-runtime/sessions.ts

Lines 266 to 270 in e7879a8

	async getSession(cookieHeader, options) {
	let id = cookieHeader && (await cookie.parse(cookieHeader, options));
	let data = id && (await readData(id));
	return createSession(data || {}, id || "");
	},

Can you provide a working minimal reproduction on stackblitz to show where these useless readData calls are happening?

[tats-u]

react-router/packages/react-router/lib/server-runtime/sessions.ts

Lines 266 to 270 in e7879a8

	async getSession(cookieHeader, options) {
	let id = cookieHeader && (await cookie.parse(cookieHeader, options));
	let data = id && (await readData(id));
	return createSession(data || {}, id || "");
	},

You are absolutely right. I misunderstood something. cookie.parse returns a falsy value (i.e. null) if the user does not have a valid cookie and it prevents readData from evaluated thanks to && in L268. createSession just creates and returns a wrapper of a JS's Map regardless of how a SessionStorage is implemented. readData is not called unnecessarily often as you said. My debugger did not react to a conditional breakpoint at the first line of readData where !id. I will change and weaken the background and motivation of the suggestion again.