Issue #1604 again with further details

[topperspal]

In context of #1604 , and please don't close the request without reading further

Yes, I am currently using two different class, one for dao and another for Repository but that is boilerplate, create one entire class just for naming conflict.

Second, the most important, I, like other people, like to use drift for sqlite database. This is really a great package and solves a lot of problems for me. But please think it twice. Breaking changes can be refactored ( you already did good job while renaming the package ). Names of the methods must be meaningful.

For example, what arguments you think these methods will take after first look at the methods? delete(...).delete(...) update(...).update(...)

And if this method is named something like this deleteFrom(...).delete(...)

Anyone can easily tell that first method will take either a Tableor a Collection and the second one will take an object. Concise means make a word smaller but meaningful.

Have a look at these examples also

selectFrom(usersTable).get()
deleteFrom(usersTable).delete(user)

OR

select(userstable).insert(user);
select(userstable).update(user);
select(userstable).delete(user);

I know breaking changes are painful but right action must be taken as soon as possible. On first side where drift is rescuing us from boilerplate using code generation and on the other hand you are asking me to create boilerplate. This is not nice.

Please consider it correctly because it is getting popular rapidly, once a dev use it, he fall in love with it.

Again think about it.

select(userstable).insert(user);
select(userstable).update(user);
select(userstable).delete(user);

It will also help us to generate some code on top of drift generation

ThankU:)

[simolus3]

In 6efe6de3bacd4a81a45a7b64691c1a72be7b170b, I have added an alternative API similar to the one you're suggesting. I mainly see it as a way to simplify common operations, but I may expand it and remove the original methods in a future breaking release if it turns out to be ergonomical enough. It allows writing from(users).insertOne(user), from(users).replace(user) and from(users).deleteOne(user).

[topperspal]

ThankU for this. In order to make it ergonomic, It needs to be used in documentations and examples.

[topperspal]

Please release the latest version of drift so that we can start using from(users).insertOne(user), from(users).replace(user) and from(users).deleteOne(user) so that we can test it, and start refactoring everywhere even in the drift docs.

[topperspal]

Hi simolus3, drift should be updated in pub.dev so that we can take benefit of the latest changes and bug fixes like the one you fixed today about group_concat(), etc. when there are no breaking changes, what are you waiting for. Versioning number never ends.😃

[simolus3]

Ideally I want to fix #1630 as well before the release. Do you have more information on how to reproduce this?

[topperspal]

Hey sorry for the inconvenience, but it seems absurd to use from(...) syntax also;

for eg., it is pretty good to use from(...) syntax for reading or deleting a row, because if we take something out of something we use from in general English, but if we want to put something into something else, we generally use to or into. So it doesnot look nice to use

from(someTable).insert() // From means we are requesting something from someTable
from(someTable).update() // So it it is not a nice syntax
from(someTable).delete() // Good!
from(someTable).get() // also Good

I prefer Firebase or MongoDB for serverless app. They use the same syntax for CRUD operations like this.

collection(someCollection).insert(doc);
collection(someCollection).update(doc);
collection(someCollection).delete(doc);

So we can use something like that

table(someTable).insert(row);
table(someTable).update(row);
table(someTable).delete(row);

// OR

select(someTable).insert(row);
select(someTable).update(row);
select(someTable).delete(row);

// OR to be more concise (avoiding the difficult situation of choosing the right word for selecting
// the table like from(...), select(...), table(...)), You can 
// generate some methods on tables directly for CRUD operations, so we can use it like this -

// simple and clean yet contextual and consice, preferable
someTable.insert(row); // users.insert(user);
someTable.update(row); // users.update(user);
someTable.delete(row); // users.delete(user);

We also like insertOne() and deleteOne() methods, but why there is no updateOne(), and to be more robust in meaning MongoDB also provides insertMany(), updateMany() and deleteMany(). By providing such apis, anyone can easily understand the usages of an api or method. If you don't think so, have a look down at the code

users.insertOne(...); // Clear
users.insert(...) // Why is this?

// OR 

users.insert(...); // Why is this?
users.insertMany(...) // Clear

// OR 

users.insertOne(...); // Clear
users.insertMany(...) // OK, this also fine

The last one consideration is important because we all use different types of databases for different apps and the all provides same apis for CRUD but there are some conflicts between different database's apis, because some database use insert() to insert a single document and insertMany(...) for inserting multiple documents, while some other databases use insertOne() for inserting a single document, and use insert(...) for inserting multiple documents.

[simolus3]

I agree that a having those methods on the table can be a better API than the current from() method.

However, I think that we shouldn't have insertMany / updateMany and deleteMany methods on the table directly - users should use batches for that. The reason that insert exists because we have more options with SQL than with most document databases. This includes the onConflict clause or more options that can be set on the InsertStatement class. The insertOne is a shortcut for the most common case, but that doesn't mean that there's no use case for insert()

[topperspal]

I agree that a having those methods on the table can be a better API than the current from() method.

Definitely! I started using from(...) each and everywhere possible. but soon I started realizing that from(...) API is not appearing appropriate for insert/update statements. Although it appears good for reading/deleting records.

If we think about SQL, every statement is clear like

SELECT * FROM table
INSERT INTO table
DELETE FROM table
UPDATE table

and drift's default implementation is something similar to this.

select(table).get(...)
into(table).insert(...)
update(table).replace(...)
delete(table).delete(...)

But the problem here is SQL is a proper language. And INSERT, UPDATE, SELECT, DELETE are reserved words in SQL. While drift depends on dart. As we know that dart already has many reserved words, so it is not a good practice for developing packages to reserve more words especially most common ones like update, delete, get, etc. By providing methods directly on tables, drift will give the same taste of SQL statements without reserving a single word.

someTable.select(); // SELECT * FROM someTable
someTable.insert(); // INSERT INTO someTable
someTable.update(); // UPDATE someTable
someTable.delete(); // DELETE FROM someTable

// Much simpler and similar to SQL Statements than drift's default apis

However, I think that we shouldn't have insertMany / updateMany and deleteMany methods on the table directly

You are right. I thought batch operations can be handled behind the seen like

Future<void> insertMany(rows) {
  await db.batch((batch) {
    batch.insertAll(rows);
  });
}

But by doing this we are just hiding the batch job instead of avoiding some boilerplate. batch jobs are already simple.

[simolus3]

I thought batch operations can be handled behind the seen like

IMO the problem with doing that implicitly is that we'd completely hide the batch internally. Users would be tempted to write

await firstTable.insertMany(largeList1);
await secondTable.insertMany(largeList2);

without realizing that there are now two transactions happening that could be optimized further by moving them into a single batch.

[topperspal]

IMO the problem with doing that implicitly is that we'd completely hide the batch internally.

You are right here. I also meant that

Me :> But by doing this we are just hiding the batch job instead of avoiding some boilerplate. batch jobs are already simple.

[topperspal]

Hi I saw your last commit, you created some useful extensions on tables. I don't want to disappoint you but I think these methods should be injected directly inside the table class. There are three main reasons of doing this-

1. Extensions need to be imported separately, consider the following example

user.dart

class User {
  final String name;

  const User(this.name);
}

extension UserExt on User {
  void greet() => print('Hi $name');
}

other.dart

final user = await authRepo.signIn(...);

user.greet(); // Here we cannot use greet() without importing user.dart file

In case of drift we need to import package:drift/drift.dart, else we won't be able to use new APIs. That is wierd because This is not a good replacement of drift's default APIs because suppose we are exporting the table or passing the table as argument, then IDE will not show any apis on table unless we import drift.dart.

final users = getUsersTable();
usersTable. // Note that IDE does not have any idea about APIs. This is wierd

If we will use all these insertOne(), deleteOne(), etc. methods directly on tables (instead of extensions), all these methods will be available for all the instances of the table, across the app.

2. drift_gen does awesome job of reducing the complexity and boilerplate of code by generating a lot of boilerplate. Generated data file in my app contains almost 5000 lines of code, and believe me I don't care if it adds few hundreds of lines more. I use a lot of extensions in my app ( mostly for my own usecases ), but that is completely different. I take care of importing the file everywhere I need it, but for a library it adds complexity. Suppose a person started some tutorials of using drift and he missed to import the file, he won't be able to understand why this is not working in his app, then he will search for it on the internet and bla bla. If this is the future of drift, then I shouldn't be like this.

3. All these methods can be injected easily to all the tables with the help of source generation.

[simolus3]

Extensions requiring a separate import is a good point, but I still think their advantages are overwhelming. Drift is already using extensions a lot, and they're super helpful for limiting things based on the type (so e.g. we can have different methods available depending on whether we have a vie or table). Typically, one would write queries in database or DAO classes, and those already import drift. Users could still export drift once from their database file if they want to use this extension everywhere.

Duplicating those methods into every class seems ridiculous, it will blow up binary size by a lot! We could add the methods into relevant base classes, but at least for tables the base class is a mix-in where added methods also have a small impact for each table. Also, let's not forget about why you were asking for this in the first place: Drift using method names that you want to use! For extensions, there's no conflict when you declare a method (or, in this case, a column) with the same name as a drift method. For methods on a superclass, this would then be a problem.

[topperspal]

You can use superclass that will contain all the methods necessary for a View or a Table.

class TableBase {
  late final String name;

  Future<int> insertOne(dynamic data) async {
     // construct query here
    print('INSERT INTO $name ...');
   ...
  }
}

class UsersTable extends TableBase {
  @override
  late final String name = "users";
}

// THEN 
void main() {
  final users = UsersTable();
  users.insertOne('object'); // prints => INSERT INTO users ...;
}

Superclasses can be tested easily and they can be created without the help of code generation

[topperspal]

I am still waiting for your response. If you do not yet agree with me about extensions, let's look at this example

import 'package:myapp/database/drift_db.dart';
// import 'package:drift/drift.dart';
// I imported my database file
void main() {
  final db = DriftDB();

  // Now the following line of code will raise a CTE 
  // because the compiler does not know about the methods, 
  // So I need to import 'package:drift/drift.dart' package explicitly,
  db.users.insertOne(...)
}

This way we can use all extension methods but if you make it accessible directly on tables with the help of super classes or mixins, It will work like a charm

You made a lot of efforts to make this lib worthy, also tried to improve it in order to avoid some keywords conflicts like update() and delete().

So if it still does not matter to you or I failed to elaborate on the importance of instance methods or It is not possible you can close the issue.

[topperspal]

What is your plan?

[simolus3]

I think I'll stick with extensions. We already use extensions for most parts of the query builder and they're easier to maintain for me. Thanks for all the thoughtful discussion on this!