-
Notifications
You must be signed in to change notification settings - Fork 31
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
refactor(neon_framework): migrate to neon_storage #2529
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nikolas Rimikis <[email protected]>
This is temporary and the long term goal is to remove most of these classes as the framework transitions to direct sqlite access. Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2529 +/- ##
==========================================
+ Coverage 28.66% 28.69% +0.02%
==========================================
Files 366 369 +3
Lines 136296 136370 +74
==========================================
+ Hits 39076 39137 +61
- Misses 97220 97233 +13
*This pull request uses carry forward flags. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the first commit is a bit confused. It still uses per Account caching, but simply doesn't use the class anymore.
|
||
/// Column for the [Credentials.serverURL]. | ||
@protected | ||
static const String serverURL = 'server_urlL'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static const String serverURL = 'server_urlL'; | |
static const String serverURL = 'server_url'; |
sqflite: | ||
dependency: transitive | ||
description: | ||
name: sqflite | ||
sha256: a43e5a27235518c03ca238e7b4732cf35eabe863a369ceba6cbefa537a66f16d | ||
url: "https://pub.dev" | ||
source: hosted | ||
version: "2.3.3+1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still required for Android/iOS
await database.close(); | ||
}); | ||
|
||
test('throws when unutilized', () async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test('throws when unutilized', () async { | |
test('throws when uninitialized', () async { |
|
||
expect( | ||
persistence.getValue('key'), | ||
equals('value'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equals('value'), | |
'value', |
PRIMARY KEY("prefix","key"), | ||
UNIQUE("key","prefix") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRIMARY KEY("prefix","key"), | |
UNIQUE("key","prefix") | |
PRIMARY KEY("prefix", "key") |
PRIMARY KEY is already UNIQUE
/// Column for the [Credentials.serverURL]. | ||
@protected | ||
static const String serverURL = 'server_urlL'; | ||
|
||
/// Column for the [Credentials.username]. | ||
@protected | ||
static const String loginName = 'login_name'; | ||
|
||
/// Column for the [Credentials.appPassword]. | ||
@protected | ||
static const String appPassword = 'app_password'; | ||
|
||
/// Column to store the active account. | ||
@protected | ||
static const String active = 'active'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pre- or suffix these with column
?
"$serverURL" TEXT NOT NULL, | ||
"$loginName" TEXT NOT NULL, | ||
"$appPassword" TEXT, | ||
"$active" BOOLEAN NOT NULL DEFAULT 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should store this separately as it is not really an information about the account itself.
Also there is no check that two accounts can not be active at the same time.
return credentials.build(); | ||
} on DatabaseException catch (error, stackTrace) { | ||
_log.warning( | ||
'Error loading cookies.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong error message (same for all other catch blocks in this class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work without the platform specific initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The neon_storage package handles this with a conditional import/export.
neon/packages/neon_framework/packages/neon_storage/lib/src/sqlite/sqlite.dart
Lines 1 to 3 in 5484049
export 'database_factory.dart' | |
if (dart.library.js_interop) '_browser_database_factory.dart' | |
if (dart.library.io) '_io_database_factory.dart'; |
We can not easily run the neon_storage tests on web but I'll verify again that this actually works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could have upgrade testing from old schema versions to the latest schema version. This should also include test data to ensure everything is migrated correctly.
Signed-off-by: Nikolas Rimikis [email protected]