Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: migration to leveldb #1401

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

thegrannychaseroperation
Copy link
Contributor

When submitting this pull request, I confirm the following (please check the boxes):

  • I have read and understood the Contributor Guidelines.
  • I have checked that there are no duplicate pull requests related to this request.
  • I have considered, and confirm that this submission is valuable to others.
  • I accept that this submission may not be used and the pull request may be closed at the discretion of the maintainers.

Ditching SQLite to use LevelDB instead.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR migrates user authentication and subscription data from SQLite to LevelDB while maintaining a hybrid approach for other data storage needs.

  • Added encryption for sensitive data in LevelDB using new Crypto utility in src/main/services/hydra-api.ts
  • Removed UserAuth and UserSubscription entities from TypeORM/SQLite in favor of LevelDB key-value storage
  • Modified sign-out process to use LevelDB batch operations while maintaining SQLite transactions for game data
  • Added new levelDatabasePath constant for LevelDB storage while preserving SQLite paths for hybrid approach
  • Introduced potential atomicity concerns between LevelDB and SQLite operations in src/main/events/auth/sign-out.ts

💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!

20 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile

src/main/constants.ts Show resolved Hide resolved
export * from "./user-preferences.entity";
export * from "./user-subscription.entity";
export * from "./game-shop-cache.entity";
export * from "./game.entity";
Copy link

Choose a reason for hiding this comment

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

logic: duplicate export of game.entity on line 1 - remove this line

Comment on lines +10 to +12
const auth = await db.get<string, Auth>(levelKeys.auth, {
valueEncoding: "json",
});
Copy link

Choose a reason for hiding this comment

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

logic: db.get() can throw - needs try/catch to handle LevelDB errors gracefully

Comment on lines +15 to +17
const payload = jwt.decode(
Crypto.decrypt(auth.accessToken)
) as jwt.JwtPayload;
Copy link

Choose a reason for hiding this comment

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

logic: Crypto.decrypt() can throw - needs try/catch to handle decryption failures

Copy link

Choose a reason for hiding this comment

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

logic: The one-to-one relationship with UserAuth needs to be maintained in the new LevelDB structure to prevent data inconsistency

Comment on lines +12 to +14
const user = await db.get<string, User>(levelKeys.user, {
valueEncoding: "json",
});
Copy link

Choose a reason for hiding this comment

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

logic: no error handling for failed db.get() operations - could throw unhandled exceptions

Comment on lines 248 to 260
await db
.get<string, Auth>(levelKeys.auth, { valueEncoding: "json" })
.then((auth) => {
return db.put<string, Auth>(
levelKeys.auth,
{
...auth,
accessToken: Crypto.encrypt(accessToken),
tokenExpirationTimestamp,
},
{ valueEncoding: "json" }
);
});
Copy link

Choose a reason for hiding this comment

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

logic: No error handling if db.get() fails before the .then() - could leave auth in inconsistent state. Should use try/catch.

Comment on lines +189 to 191
const result = await db.getMany<string>([levelKeys.auth, levelKeys.user], {
valueEncoding: "json",
});
Copy link

Choose a reason for hiding this comment

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

logic: getMany could fail silently here since there's no error handling. Should wrap in try/catch to handle DB errors.

Comment on lines +80 to 88
db.put<string, Auth>(
levelKeys.auth,
{
id: 1,
accessToken,
accessToken: Crypto.encrypt(accessToken),
refreshToken: Crypto.encrypt(refreshToken),
tokenExpirationTimestamp,
refreshToken,
},
["id"]
{ valueEncoding: "json" }
);
Copy link

Choose a reason for hiding this comment

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

logic: db.put operation could fail but error is not handled. Should await the operation and handle potential errors.

Comment on lines +290 to +299
db.batch([
{
type: "del",
key: levelKeys.auth,
},
{
type: "del",
key: levelKeys.user,
},
]);
Copy link

Choose a reason for hiding this comment

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

logic: batch delete operation could fail but error is not caught. Should wrap in try/catch to handle DB errors.

@thegrannychaseroperation thegrannychaseroperation changed the title feat: migration to leveldb (WIP) feat: migration to leveldb Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants