Removal of struct tag based table mapping

[aacanakin]

A complex example;

type User struct {
    ID        string    `db:"_id" qb:"constraints:primary_key" json:"id"`
    Email     string    `qb:"type:varchar(255); constraints:unique,notnull; index" json:"email"`
    CreatedAt time.Time `qb:"constraints:not_null" json:"created_at, omitempty"`
}

This is clearly a violation of go idioms. The struct definition above is not readable in terms of tags. Instead of defining constraints in struct, the struct definition tags should only be used for mapping fields to sql row.

The expression api equivalent of this is the following;

users := qb.Table(
    "user",
    qb.Column("_id", qb.Varchar()),
    qb.Column("email", qb.Varchar().Unique().NotNull()),
    qb.Column("created_at", qb.Timestamp().NotNull()),
    qb.PrimaryKey("id"),
).Index("email")

This is clearly a better implementation without using any kind of magic(reflection). Therefore, the User struct is much more simplified and deals only with mapping rows to structs. The simplified version of User struct is the following;

type User struct {
    ID        string    `db:"_id" json:"id"`
    Email     string    `json:"email"`
    CreatedAt time.Time `json:"created_at, omitempty"`
}

One downside of this is the fact that you should have both table and the struct defined instead of single struct definition.

Because of the reasons above the ddl generator from struct tags should be removed. This will produce a lot of breaking changes. However, this would simplify lots of things in mapper, metadata, engine, etc. This would also enable the ease of relationships support.

I need votes for this.

[aacanakin]

ping @li3p, @shawnps, @proc, @joeblew99, @lijunfeng, @aodin

[cdevienne]

I am currently playing with qb, and one thing I do not like is the inspect-based table & mapper.
It is the way most go orms works, except for 'reform', which generates code from the structure.

What I am doing is to see how we could define a interface based (as opposed to tag&inspect based) mapper, the way reform does.

All this to say that I think it is too soon for qb to become a complete orm. The query builder is the strong suit of qb, and I have a feeling that it should concentrate on providing an equivalent of "sqlalchemy.sql" and stay away from "sqlalchemy.orm". That later part could be done in other packages depending on qb, which would allow exploring various ways easily.

So, my suggestion is to remove mapper things from qb, and move them into a different package (qb-mapper form example), so I (and others) can experiment on a different approach (I will open other issues related to the subject because some apis are getting in my way).

As for answering directly to your initial question, although I understand why you would like to remove the constraints and indexes from the tags, I do not think you can do that without rethinking completely what a orm in go is (and move away from how it currently work), which would be another good reason for removing mappers from qb...

[aacanakin]

I'm completely agreeing that it is too soon for that project to become a complete orm. Inspect & Tag based mapping is doing so much reflection that could cause performance & usability problems. At the start, the project was only aiming to port sqlalchemy's sql package which handles query building, etc. The reason why sqlalchemy orm works great (in terms of functionality) is the fact that python language gives so much flexibility and magic that enables so many different solutions. However, go idioms defends the against. I think they're right.

I think removing the mapper can be a solution. But, at the same time, it cuts down functionality. So, I have a solution.

Let's have an engine with only single sqlx connection dependency. Sqlx package wraps the default sql package & gives extra functions for mapping result sets into structs. By using this way, you can test different approaches. You can still run queries using the default sql package. The mapper.go & session.go would be removed as you suggested.

Code generation could be another solution. However, I feel like it would bring so much complexity to the implementation. I think it would also make harder for developers to understand what's going on.

@cdevienne What do you think?

[cdevienne]

I support the idea of letting a sqlx connection on the engine.

As for code generation, it is an experiment. Once I have more concrete code to show, I will share it.