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

Merged
merged 88 commits into from
Feb 1, 2025
Merged

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.

Siteproxy
bumyy32 and others added 8 commits November 7, 2024 20:23

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
Copy link
Contributor

@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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

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.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
fix
zamitto and others added 26 commits January 29, 2025 08:58
…-permission

# Conflicts:
#	src/main/events/library/open-game.ts
#	src/main/events/user-preferences/update-user-preferences.ts
#	src/main/index.ts
#	src/main/main.ts
#	src/main/services/achievements/achievement-watcher-manager.ts
#	src/main/services/achievements/get-game-achievement-data.ts
#	src/main/services/notifications/index.ts
#	src/renderer/src/pages/downloads/download-group.tsx

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…ermission

feat: changing permission verify strategy

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
…tion-to-leveldb

Unverified

This user has not yet uploaded their public signing key.
Copy link

sonarqubecloud bot commented Feb 1, 2025

@thegrannychaseroperation thegrannychaseroperation merged commit 64c397d into main Feb 1, 2025
4 checks passed
@zamitto zamitto deleted the feat/migration-to-leveldb branch February 2, 2025 00:10
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.

None yet

4 participants