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

goqu rework & Support for PostegreSQL, MySQL/MariaDB #5

Open
2 of 9 tasks
Tracked by #8
CrescentKohana opened this issue Feb 26, 2022 · 19 comments
Open
2 of 9 tasks
Tracked by #8

goqu rework & Support for PostegreSQL, MySQL/MariaDB #5

CrescentKohana opened this issue Feb 26, 2022 · 19 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed server Concerns Mangatsu server

Comments

@CrescentKohana
Copy link
Member

CrescentKohana commented Feb 26, 2022

The rework is being work on in https://github.com/Mangatsu/server/tree/goqu-rework

Currently only SQLite3 is supported. Support for more databases would be nice.

Supported databases

  • SQLite3
  • PostgreSQL
  • MySQL/MariaDB (optional)

Tasks

@CrescentKohana CrescentKohana added enhancement New feature or request good first issue Good for newcomers server Concerns Mangatsu server labels Feb 26, 2022
@CrescentKohana CrescentKohana mentioned this issue Feb 26, 2022
17 tasks
@karmek-k
Copy link
Collaborator

karmek-k commented Apr 4, 2022

Hi! I'm looking for a OSS project to contribute to, and a manga reader is actually what I thought of creating. 😃

Regarding the issue, my Go experience is still limited, but I'm pretty sure that providing support to other DBMSs is more complicated than just modifying sql.Open("sqlite3", config.BuildDataPath("mangatsu.sqlite")).

I guess that the most difficult part would be configuring migrations, as the current ones are SQLite-specific. It would be great to have some kind of schema builder that allows for DB-agnostic migration generation - Laravel does that, and I think it works pretty well.

And I can see that some kind of SQL query builder is used, but github.com/mattn/go-sqlite3 is still imported in pkg/db/gallery.go and other files. I'm not sure, but creating query adapters for each DBMS could be a solution 😄

Looking forward to your reply!

@CrescentKohana
Copy link
Member Author

CrescentKohana commented Apr 4, 2022

Hi, and thanks for showing interest in this project!

Migrations

but I'm pretty sure that providing support to other DBMSs is more complicated than just modifying sql.Open("sqlite3", config.BuildDataPath("mangatsu.sqlite")).

Correct. Like you said, configuring the migrations would probably require some work as there are some differences in SQLite, MySQL and PSQL. PSQL could be easier to implement as SQLite somewhat follows its syntax IIRC.

Then again migrations in this project aren't that complicated, so it wouldn't probably take that much time to just create separate migrations for all three DBMSs if needed.

It would be great to have some kind of schema builder that allows for DB-agnostic migration generation - Laravel does that, and I think it works pretty well.

Currently I'm using Goose for the migrations. But I'm also open to changing this completely as it really doesn't affect the application itself. It also supports Go code in the migrations but I haven't looked into it that deeply.

Query Builder

And I can see that some kind of SQL query builder is used

Yes, I'm using Jet. Basically, it reads the schema of the initiated database and generates models and tables for the application. It's used to build the queries in pkg/db/*.

As a side-note, I didn't want to use an ORM for performance reasons so I went for a pure query builder based solution.

but github.com/mattn/go-sqlite3 is still imported in pkg/db/gallery.go and other files.

Those imports seem to be unnecessary and can be removed in all three files: gallery.go, library.go, user.go 😅.

Having said that, I think the real challenge is here: SQL statements and other db related functions are imported from Jet: github.com/go-jet/jet/v2/sqlite which complicate stuff from a syntax standpoint. Examples:

  • InsertStatement is actually sqlite.InsertStatement. I've just used a dot import to not have to prefix everything with sqlite.
  • Same with String (sqlite.String) which is a internal type for Jet.

There are also some SQLite specific logic being used in the code like IFNULL (that seems to be in SQLite and MySQL but not PSQL).

Solution

creating query adapters for each DBMS

So yes, I think this is the best solution 😉. To make it more clear:

First we should probably create an abstraction layer which would be used in API and other packages. An environment variable would determine which adapter the abstraction layer uses at a given time.

We could create 3 directories: db/sqlite, db/mysql, db/psql all including the same statements with different Jet imports github.com/go-jet/jet/v2/sqlite, github.com/go-jet/jet/v2/mysql, github.com/go-jet/jet/v2/psql respectively with required changes to the code.

The most obvious negative with this would be the duplicate code, but that's inevitable. Maintainability shouldn't be a problem as the Jet models would help make sure that every query adapter has feature parity. Of course it'd be good idea to write tests for the query adapters as well.

Also, we would have to generate different types for all three query adapters but that shouldn't be a problem as it's just the matter of setting up the dbs and running commands. Structure should probably follow query adapters: types/sqlite/, types/mysql and types/psql.

.

I'll probably get back to this tomorrow and see how much changes would migrations and queries require.

We could also replace Jet to something else but that would be even larger task and I'm not sure if there are any other non-ORM query builders for Go.

// Made some edits to clear up my thoughts about this.

@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

Okay I'm done with work stuff for today, time for Go 😄

After some searching I found goqu, a query builder that appears to be much simpler than Jet (it just generates SQL and that's it) - however, it allows for SQL generation for different SQL dialects - SQLite, PostgreSQL etc.
goqu appears to be more popular on GitHub than Jet, and in my opinion it looks a lot like Knex, a popular SQL builder for Node.js.

However, it comes with a cost - since it only generates SQL, no extra tooling is provided, so gallery.go, library.go, user.go would have to be rewritten in goqu.

The most obvious negative with this would be the duplicate code, but that's inevitable.

That's why I brought a different query builder in the first place, we can do something like this:

(note that I didn't test the following snippets)

// import database/sql, goqu, goqu dialects and DB drivers

// Database wraps around the sql.DB handle and a query builder
type Database struct {
	Dialect string
	Handle *sql.DB
	QB goqu.DialectWrapper
}

// CreateDatabase creates a new Database object
func CreateDatabase(dialect string, connString string) (*Database, error) {
	qb := goqu.Dialect(dialect)

	handle, err := sql.Open(dialect, connString)
	if err != nil {
		return nil, err
	}

	db := &Database{dialect, handle, qb}

	return db, nil
}

// GetQB returns the database's query builder
func (db *Database) GetQB() goqu.DialectWrapper {
	return db.QB	
}

And then for queries (this is a quite verbose way of doing things, but I didn't want to change existing code ^^;):

// SelectQuery performs a SELECT query to the database with a given
// dataset built by the query builder
// (replace []interface{} with a concrete type, consider using generics for code reuse)
func (db *Database) SelectQuery(queryObject *SelectDataset) ([]interface{}, error) {
	// there is a more idiomatic way of making queries with goqu:
	// http://doug-martin.github.io/goqu/docs/dialect.html#executing-queries

	sql, args, err := queryObject.ToSQL()
	if err != nil {
		return []interface{}, err
	}

	rows, err := db.Handle.Query(sql, ...args)
	if err != nil {
		return []interface{}, err
	}
	defer rows.Close()

	// handle rows...

	return result, nil
}

And so, it's not database-dependent anymore 🙂
(Using an ORM would be the obvious solution, but if you don't want one, then it's totally fine, a query builder would do as well)

As for migrations, I haven't had looked too much into that. Maybe we could create a Makefile or something, that builds migrations for all supported DBMSs?

There's also migrate, where you can then apply them either by a CLI or by accessing the migrate library in Go.
Maybe Goose has a similar API for programmatic migration application?

import (
	"database/sql"
	_ "github.com/lib/pq"
	"github.com/golang-migrate/migrate/v4"
	"github.com/golang-migrate/migrate/v4/database/postgres"
	_ "github.com/golang-migrate/migrate/v4/source/file"
)

func main() {
	db, err := sql.Open("postgres", "postgres://localhost:5432/database?sslmode=enable")
	// or in our case, `database.Handle` instead of `db`
	driver, err := postgres.WithInstance(db, &postgres.Config{})
	m, err := migrate.NewWithDatabaseInstance(
		"file:///migrations",
		"postgres", driver)
	m.Up()
}

@CrescentKohana
Copy link
Member Author

CrescentKohana commented Apr 5, 2022

After some searching I found goqu

Interesting. The downside with this would probably be the fact that it can't generate types, but to be fair they aren't that complex to begin with. Writing them manually would be pretty easy.

Using an ORM would be the obvious solution, but if you don't want one, then it's totally fine, a query builder would do as well

We could benchmark how much the difference would actually be with large collections of manga and tags. Though the performance is not the only reason why I'm relunctact in using an ORM. With "pure SQL", it's easier to fine tune and debug the queries. In the end SQL is a well designed language and doesn't really need a complex abstraction on top of it. But I'm not completely turning this idea away either.

And then for queries (this is a quite verbose way of doing things, but I didn't want to change existing code ^^;)

Yeah, I wasn't probably using the best practices when writing that, so a refactor isn't a bad idea. This is my second time writing Go for a project 😁.

goqu seems great and I'd say lets at least try that. For migrations, I'll test some things out with Goose, the migrate you linked and some other libraries as well.

@CrescentKohana
Copy link
Member Author

It was pretty annoying to write migrations in pure SQL anyway, at least for SQLite, as in some cases I had to drop and create the whole table like when renaming a column.

@CrescentKohana
Copy link
Member Author

Pushed some basic envs for databases.

@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

Feel free to assign me to a task if there's any, I'll fork the repo and make a PR.

@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

In the end SQL is a well designed language and doesn't really need a complex abstraction on top of it.

I think that a query builder has the advantage of being database-agnostic, which is of course great for this issue. Writing SQL by hand for all supported DBMSs would be horribly tedious.

@CrescentKohana
Copy link
Member Author

Feel free to assign me to a task if there's any, I'll fork the repo and make a PR.

You could start by migrating db.go and library.go to goqu, and see how suitable it would be in reality for this project. They shouldn't have too much code to work with even if it doesn't turn out to be a success. I can create an issue for them.

I think that a query builder has the advantage of being database-agnostic, which is of course great for this issue. Writing SQL by hand for all supported DBMSs would be horribly tedious.

Yes, of course. That's why query builder is good here.

By the way, I had this thought that supporting both PostgreSQL and MySQL/MariaDB isn't too important currently. One of them is enough, as nowadays people use stuff like Docker which makes it really easy to just fire up any DB.

But of course it'd be great to have support for all three. Just saying this if there are any problems, (temporarily) dropping support for either one isn't a problem.

@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

I can create an issue for them.

OK 😄

By the way, I had this thought that supporting both PostgreSQL and MySQL/MariaDB isn't too important currently. One of them is enough, as nowadays people use stuff like Docker which makes it really easy to just fire up any DB.

If I were to choose one, I'd pick PostgreSQL. People might want to deploy the server to Heroku, where Postgres is natively supported.

@CrescentKohana
Copy link
Member Author

If I were to choose one, I'd pick PostgreSQL. People might want to deploy the server to Heroku, where Postgres is natively supported.

Agreed.

@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

Sorry if it's a stupid question, but should I start working on #14, as you said in #5 (comment)? There are some new tasks in the OP.

Edit: I have just noticed that you assigned me to it 😃

@CrescentKohana
Copy link
Member Author

CrescentKohana commented Apr 5, 2022

Yeah I just created all relevant tasks. Also I just realized that we might not need any abstractions if goqu solves that. Anyway, there's the issue (#17) for the abstractions as well if needed.

@CrescentKohana CrescentKohana added the help wanted Extra attention is needed label Apr 5, 2022
@CrescentKohana CrescentKohana pinned this issue Apr 5, 2022
@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

Here are some problems with M$ Windows 😅

  1. In example.env, relative paths seem to not work with Windows 10.
  2. The MTSU_BASE_PATHS looks like this:
    MTSU_BASE_PATHS: freeform1;/home/user/doujinshi;;structured2:/home/user/manga
    and should look like this:
    MTSU_BASE_PATHS: freeform1;/home/user/doujinshi;;structured2;/home/user/manga
    (colon replaced with a semicolon after structured2)

Edit: Maybe there should be a new issue on it?

@CrescentKohana
Copy link
Member Author

  1. These work on my Windows machine MTSU_BASE_PATHS=freeform1;devarchive/freeform;;structured2;devarchive/structured
    • devarchive/freeform is in the same level as pkg, docs etc.
  2. Yes that should be a semicolon. I'll fix it.

@CrescentKohana
Copy link
Member Author

CrescentKohana commented Apr 5, 2022

@karmek-k Forgot to mention, for Postgres we should probably use pgx as the driver. The other large one lib/pq is also recommending it. Though it's not that big deal.

@karmek-k
Copy link
Collaborator

karmek-k commented Apr 5, 2022

@karmek-k Forgot to mention, for Postgres we should probably use pgx as the driver. The other large one lib/pq is also recommending it. Though it's not that big deal.

IDK if goqu works with pgx, the README says that it doesn't implement database/sql. Right now I'm rewriting library.go, I'd just stick with lib/pq for now, we probably don't need blazing fast speeds 😃

And pgx also says in the README that it's not recommended for applications supporting more than PostgreSQL.

@CrescentKohana
Copy link
Member Author

Yup. Lets go with lib/pq 👍.

@CrescentKohana
Copy link
Member Author

Made a new branch where we can merge the rework more freely: https://github.com/Mangatsu/server/tree/goqu-rework

@CrescentKohana CrescentKohana added priority and removed good first issue Good for newcomers labels Apr 6, 2022
@CrescentKohana CrescentKohana changed the title Support for PostegreSQL and MySQL/MariaDB goqu rework & Support for PostegreSQL, MySQL/MariaDB Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed server Concerns Mangatsu server
Projects
None yet
Development

No branches or pull requests

2 participants