Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Add optional deleteSessions, findSessionsByShop #418

Merged
merged 6 commits into from
Jul 19, 2022

Conversation

mkevinosullivan
Copy link
Contributor

@mkevinosullivan mkevinosullivan commented Jul 8, 2022

WHY are these changes introduced?

To support using the session database as a means to track app installations, adding optional methods to search for sessions by shop and deleting all sessions by a range of ids (e.g., for a shop, or as a garbage collector, if desired).

WHAT is this pull request doing?

Adding optional findSessionsByShop and deleteSessions interface methods to SessionStorage and the corresponding implementations in the various DB adapters.

Type of change

  • Minor: New feature (non-breaking change which adds functionality)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@mkevinosullivan mkevinosullivan force-pushed the kos/add_deleteSessions_findSessionsByShop branch from 22cc0cf to 38b960c Compare July 8, 2022 18:10
Copy link
Member

@surma surma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM.

One question: What is the use-case for findSessionsByShop? Do we need to worry about the returned array being uber large for popular apps?

Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I had some thoughts on how to make a couple of these more efficient, but overall I think this makes sense!

src/auth/session/storage/__tests__/battery-of-tests.ts Outdated Show resolved Hide resolved
src/auth/session/storage/__tests__/battery-of-tests.ts Outdated Show resolved Hide resolved
src/auth/session/storage/custom.ts Outdated Show resolved Hide resolved
src/auth/session/storage/custom.ts Outdated Show resolved Hide resolved
src/auth/session/storage/custom.ts Outdated Show resolved Hide resolved
src/auth/session/storage/mongodb.ts Outdated Show resolved Hide resolved
src/auth/session/storage/mysql.ts Outdated Show resolved Hide resolved
src/auth/session/storage/mysql.ts Outdated Show resolved Hide resolved
src/auth/session/storage/redis.ts Outdated Show resolved Hide resolved
): Promise<SessionInterface[] | {[key: string]: unknown}[] | undefined> {
await this.ready;

const keys = await this.client.keys('*');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's worth storing the ids for specific shops under a single key, so that we don't have to iterate over every entry to find the ids for a shop. Quite a big change, but I think it's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K, that would be better (anything would be better than this current search implementation!) but since it's such a big change to the underlying data structure, I'll leave it for its own PR.

@mkevinosullivan
Copy link
Contributor Author

This LGTM.

One question: What is the use-case for findSessionsByShop? Do we need to worry about the returned array being uber large for popular apps?

It's "give me the sessions for this one shop", so, with the additional change of using offline sessions, it realistically should be just one entry.

@mkevinosullivan mkevinosullivan marked this pull request as ready for review July 18, 2022 23:23
@mkevinosullivan mkevinosullivan requested a review from a team as a code owner July 18, 2022 23:23
@mkevinosullivan
Copy link
Contributor Author

@mkevinosullivan mkevinosullivan changed the title [WIP] Add optional deleteSessions, findSessionsByShop Add optional deleteSessions, findSessionsByShop Jul 18, 2022
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from a nit about returning the maps directly, LGTM! Haven't tried the mongodb changes myself, though.

src/auth/session/storage/__tests__/battery-of-tests.ts Outdated Show resolved Hide resolved
src/auth/session/storage/__tests__/battery-of-tests.ts Outdated Show resolved Hide resolved
src/auth/session/storage/custom.ts Outdated Show resolved Hide resolved
src/auth/session/storage/mongodb.ts Outdated Show resolved Hide resolved
To support using the session database as a means to track app
installations, adding optional methods to search for sessions by shop
and deleting all sessions by a range of ids (e.g., for a shop, or as
a garbage collector, if desired).
@mkevinosullivan mkevinosullivan force-pushed the kos/add_deleteSessions_findSessionsByShop branch from 665c113 to 303a556 Compare July 19, 2022 18:36
@mkevinosullivan mkevinosullivan merged commit f3236a0 into main Jul 19, 2022
@mkevinosullivan mkevinosullivan deleted the kos/add_deleteSessions_findSessionsByShop branch July 19, 2022 21:02
@shopify-shipit shopify-shipit bot temporarily deployed to production July 20, 2022 21:00 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants