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

use orm like https://entgo.io/ instead of writing sqls manually #2849

Closed
zwpaper opened this issue Jan 28, 2024 · 6 comments
Closed

use orm like https://entgo.io/ instead of writing sqls manually #2849

zwpaper opened this issue Jan 28, 2024 · 6 comments
Labels
enhancement New feature or request Stale

Comments

@zwpaper
Copy link
Contributor

zwpaper commented Jan 28, 2024

Describe the solution you'd like

currently, memos support sqlite, mysql, postgresql, but writing target sql separately, we could use orm, like https://entgo.io/ to simplify the db logic and get rid of manually written sql.

Type of feature

Other

Additional context

I would rewrite the mysql to do a POC if accepted

@zwpaper zwpaper added the enhancement New feature or request label Jan 28, 2024
@boojack
Copy link
Collaborator

boojack commented Jan 30, 2024

Although it's a bit more complicated to write SQL for each driver, but the execution/migration will be open and transparent. IMO, I think writing raw SQL is better then using ORM.

But you can try adding a new storev1 to the current logic to implement a store that uses ORM, so let's see how it works.

@reddec
Copy link
Contributor

reddec commented Jan 30, 2024

@boojack I was working on #2855 (comment) and and quite stuck on DB part. I would like to say that the current migration strategy and manual updates is quite tricky and probably (if you may) - it's a good time to use rare opportunity and make DB part slightly more polished.

In case you don't like ORM (which I personally sympathize), may I suggest the approach I used in other projects? sqlc + sql-migrate

However, manual queries (even with SQLc) will require multiple work for each supported DB, as well as duplicated data structures.

@boojack
Copy link
Collaborator

boojack commented Jan 31, 2024

@reddec Of course, we can use your design or ORM (as long as it solves the problem correctly and works well). But it's best to do this in a new package and not change the existing store.

@xwjdsh
Copy link
Contributor

xwjdsh commented Feb 6, 2024

I recently started a project that borrows from the memos project structure, It's great and saves me tons of time, but in the store part I found a lot of duplicate code, and I often had to copy code between drivers.

Then I noticed that each driver contains a *sql.DB, Go already handles the differences between drivers through it, drivers call APIs like QueryContext, ExecContext and so on.

For different drivers (based on *sql.DB), the only difference is the sql and args to be executed, while the rest of the scheduling logic is the same. So, I thought it might be possible to raise the scheduling logic from drivers to store and it will cut down a lot of duplicate code.

For example, the current ListActivities is,

// interface
type Driver interface{
        ...
	(ctx context.Context, find *FindActivity) ([]*Activity, error)
}

// store
func (s *Store) ListActivities(ctx context.Context, find *FindActivity) ([]*Activity, error) {
	return s.driver.ListActivities(ctx, find)
}

// db/sqlite
func (d *DB) ListActivities(ctx context.Context, find *store.FindActivity) ([]*store.Activity, error) {
	where, args := []string{"1 = 1"}, []any{}
	if find.ID != nil {
		where, args = append(where, "`id` = ?"), append(args, *find.ID)
	}
	if find.Type != nil {
		where, args = append(where, "`type` = ?"), append(args, find.Type.String())
	}

	query := "SELECT `id`, `creator_id`, `type`, `level`, `payload`, `created_ts` FROM `activity` WHERE " + strings.Join(where, " AND ") + " ORDER BY `created_ts` DESC"
	rows, err := d.db.QueryContext(ctx, query, args...)
	if err != nil {
		return nil, err
	}
	defer rows.Close()

	list := []*store.Activity{}
	for rows.Next() {
		activity := &store.Activity{}
		var payloadBytes []byte
		if err := rows.Scan(
			&activity.ID,
			&activity.CreatorID,
			&activity.Type,
			&activity.Level,
			&payloadBytes,
			&activity.CreatedTs,
		); err != nil {
			return nil, err
		}

		payload := &storepb.ActivityPayload{}
		if err := protojsonUnmarshaler.Unmarshal(payloadBytes, payload); err != nil {
			return nil, err
		}
		activity.Payload = payload
		list = append(list, activity)
	}

	if err := rows.Err(); err != nil {
		return nil, err
	}

	return list, nil
}

after modification, it will be,

// interface
type Driver interface{
        ...
	ListActivitiesSQL(find *FindActivity) (string, []any)
}

// store
func (s *Store) ListActivities(ctx context.Context, find *FindActivity) ([]*Activity, error) {
	query, args := s.driver.ListActivitiesSQL(find)
	rows, err := s.driver.GetDB().QueryContext(ctx, query, args...)
	if err != nil {
		return nil, err
	}
	defer rows.Close()

	list := []*Activity{}
	for rows.Next() {
		activity := &Activity{}
		var payloadBytes []byte
		if err := rows.Scan(
			&activity.ID,
			&activity.CreatorID,
			&activity.Type,
			&activity.Level,
			&payloadBytes,
			&activity.CreatedTs,
		); err != nil {
			return nil, err
		}

		payload := &storepb.ActivityPayload{}
		if err := protojsonUnmarshaler.Unmarshal(payloadBytes, payload); err != nil {
			return nil, err
		}
		activity.Payload = payload
		list = append(list, activity)
	}

	if err := rows.Err(); err != nil {
		return nil, err
	}

	return list, nil
}

// db/sqlite
func (d *DB) ListActivitiesSQL(find *store.FindActivity) (string, []any) {
	where, args := []string{"1 = 1"}, []any{}
	if find.ID != nil {
		where, args = append(where, "`id` = ?"), append(args, *find.ID)
	}
	if find.Type != nil {
		where, args = append(where, "`type` = ?"), append(args, find.Type.String())
	}

	query := "SELECT `id`, `creator_id`, `type`, `level`, `payload`, `created_ts` FROM `activity` WHERE " + strings.Join(where, " AND ") + " ORDER BY `created_ts` DESC"
	return query, args
}

The problem with this modification is that drivers can only be implemented via *sql.DB and cannot accept non-sql integrations (e.g. mongodb, redis).

@zwpaper
Copy link
Contributor Author

zwpaper commented Feb 8, 2024

I have created a POC PR #2940, we can discuss the ent there.

@github-actions github-actions bot added the Stale label Mar 1, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2024
@zwpaper
Copy link
Contributor Author

zwpaper commented Mar 8, 2024

@boojack can you reopen this issue, I am still working on this.

@boojack boojack reopened this Mar 9, 2024
@github-actions github-actions bot removed the Stale label Mar 9, 2024
@github-actions github-actions bot added the Stale label Mar 23, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale
Projects
None yet
Development

No branches or pull requests

4 participants