Mangatsu / server

🌕 Media server for storing, tagging and viewing doujinshi, manga, art collections and other galleries with API and user control. Written in Go.
GNU General Public License v3.0
43 stars 6 forks source link

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

Open CrescentKohana opened 2 years ago

CrescentKohana commented 2 years ago

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

Tasks

karmek-k commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Pushed some basic envs for databases.

karmek-k commented 2 years ago

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

karmek-k commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Sorry if it's a stupid question, but should I start working on #14, as you said in https://github.com/Mangatsu/server/issues/5#issuecomment-1089079916? There are some new tasks in the OP.

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

CrescentKohana commented 2 years ago

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.

karmek-k commented 2 years ago

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 commented 2 years ago
  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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

Yup. Lets go with lib/pq 👍.

CrescentKohana commented 2 years ago

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