volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.73k stars 544 forks source link

AreaDTOColumns redeclared in this block #180

Closed ArnoldoR closed 6 years ago

ArnoldoR commented 7 years ago

If you're having a generation problem please answer these questions before submitting your issue. Thanks!

What version of SQLBoiler are you using (sqlboiler --version)?

V2.5.0

If this happened at generation time what was the full SQLBoiler command you used to generate your models? (if not applicable leave blank)

sqlboiler -b MW_tca_artAlmacen -b me_tca_artAlmacenes -b mw_Tca_CodBarAlt mssql

If this happened at runtime what code produced the issue? (if not applicable leave blank)

What is the output of the command above with the -d flag added to it? (Provided you are comfortable sharing this, it contains a blueprint of your schema)

// AreaDTO is an object representing the database table. type AreaDTO struct { ZoneID string boil:"zone_id" json:"zone_id" toml:"zone_id" yaml:"zone_id" WHID string boil:"wh_id" json:"wh_id" toml:"wh_id" yaml:"wh_id" Area string boil:"area" json:"area" toml:"area" yaml:"area" Available null.Float64 boil:"available" json:"available,omitempty" toml:"available" yaml:"available,omitempty" Blocked null.Float64 boil:"blocked" json:"blocked,omitempty" toml:"blocked" yaml:"blocked,omitempty" Occupied null.Float64 boil:"occupied" json:"occupied,omitempty" toml:"occupied" yaml:"occupied,omitempty" Reserved null.Float64 boil:"reserved" json:"reserved,omitempty" toml:"reserved" yaml:"reserved,omitempty"

R *AreaDTOR `boil:"-" json:"-" toml:"-" yaml:"-"`
L AreaDTOL  `boil:"-" json:"-" toml:"-" yaml:"-"`

}

var AreaDTOColumns = struct { ZoneID string WHID string Area string Available string Blocked string Occupied string Reserved string }{ ZoneID: "zone_id", WHID: "wh_id", Area: "area", Available: "available", Blocked: "blocked", Occupied: "occupied", Reserved: "reserved", }

// AreaDTOR is where relationships are stored. type AreaDTOR struct { }

// AreaDTOL is where Load methods for each relationship are stored. type AreaDTOL struct{}

var ( AreaDTOColumns = []string{"zone_id", "wh_id", "area", "available", "blocked", "occupied", "reserved"} AreaDTOColumnsWithAuto = []string{} AreaDTOColumnsWithoutDefault = []string{"zone_id", "wh_id", "area", "available", "blocked", "occupied", "reserved"} AreaDTOColumnsWithDefault = []string{} AreaDTOPrimaryKeyColumns = []string{"area", "wh_id", "zone_id"} )

Please provide a relevant database schema so we can replicate your issue (Provided you are comfortable sharing this)

CREATE TABLE [dbo].[AreaDTO]( [zone_id] varchar NOT NULL, [wh_id] varchar NOT NULL, [area] varchar NOT NULL, [available] [float] NULL, [blocked] [float] NULL, [occupied] [float] NULL, [reserved] [float] NULL, PRIMARY KEY CLUSTERED ( [zone_id] ASC, [wh_id] ASC, [area] ASC )WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY] ) ON [PRIMARY]

GO

SET ANSI_PADDING OFF GO

Further information. What did you do, what did you expect?

aarondl commented 7 years ago

Hey there. Thanks for filing this issue. What you're seeing is different to what I'm observing here. But I'm also not able to use mssql as a driver for a few reasons currently (this may be where the difference is but I can't imagine why that'd be).

When I have a table named: AreaDTO in postgres, it's giving me a struct Areadto. Also it gives me these outputs:

var AreadtoColumns = struct {
    ID string
}{
    ID: "id",
}

var (
    areadtoColumns               = []string{"id"}
    ...
)

Now the table name from postgres actually comes out as totally lowercase. I see this output when using --debug during generation:

[{"Name":"areadto","SchemaName":"","Columns":[{"Name":"id","Type":"int","DBType":"integer","Default":"nextval('areadto_id_seq'::regclass)","Nullable":false,"Unique":true,"Validated":false,"ArrType":null,"UDTName":"int4","FullDBType":"","AutoGenerated":false}],"PKey":{"Name":"areadto_pkey","Columns":["id"]},"FKeys":null,"IsJoinTable":false,"ToOneRelationships":null,"ToManyRelationships":null}]

Notice the lowercase table name. It's possible we've been building on these assumptions of lowercase table names for mysql/postgres this entire time, and that adversely affects tables that are named uppercase things.

There's two fixes that are possible here:

  1. Make it explicit where we want lower/uppercase so there's no confusion
  2. Coerce the mssql driver to output things with lowercase

I think number 2 doesn't really work since apparently depending on collation the actual text that goes to mssql can become case sensitive, which actually presents a bigger problem for us: https://stackoverflow.com/questions/1411161/sql-server-check-case-sensitivity

ArnoldoR commented 7 years ago

thanks...

aarondl commented 7 years ago

@ArnoldoR I was expecting a bit of discussion around the solution proposed! :D Because I don't have a MSSQL sitting around and the latest docker image basically freezes on me and doesn't run (thanks MS :) I can't really experiment.

I don't understand the case sensitivity around MSSQL. Is it possible that you can test a simple select statement in SQL Server using the lowercase table name like: select * from dbo.areadto; and see if it complains about a table not existing?

aarondl commented 7 years ago

Closing for inactivity.

nadilas commented 6 years ago

@ArnoldoR what's the current status of this? I have encountered this issue today, trying out sqlboiler.

aarondl commented 6 years ago

@nadilas Can you elaborate? Do you have uppercase table names? Is it the mssql driver? Any extra details would be good to know.

nadilas commented 6 years ago

@aarondl Yes and yes. :) they are uppercase and it is the mssql driver. I updated to the v3 branch and tried aliases. I needed to tweek the aliases.go, because viper doesn’t read uppercase strings, so the driver tables have to be turned to lowercase before the aliases can be replaced. I also have uppercase FKs, so I guess there I have to make similar changes to make it work.

On an unrelated note: in v3 I had to disable hooks, one of the templates is missing a comma, I didn’t find the wrong template. It appears somewhere before “o.doAfterUpsertHooks(“. But I’ll let you if I find it.

aarondl commented 6 years ago

Question: In mssql if you are to make a query with the table name or column name lowercase does it even work? Because if it works the easiest way to combat all this is to lowercase the identifiers coming from the sqlboiler mssql driver.

nadilas commented 6 years ago

If you can make this decision based on the database/table/column collation, it could work. As far as I know all case sensitive collations contain “CS” and case insensitive contains “CI”. Although I think this would be a semi solution, because if you would need CS, viper still can’t pick that up when reading the config. I would say for the lookup and generation on the aliases both should be converted to lower- or uppercase, regardless what the driver uses for querying.

aarondl commented 6 years ago

@nadilas I just fixed some mssql bugs and pushed v3.0.0-rc6 (including the one you reported with the comma). I was able to get the tests passing and everything so it's looking like a better base to build our changes off of.

I agree with you that the driver should just use whatever the driver got for querying. And aliases should be generated by comparing lowercase names only, that seems reasonable. Unless of course someone has created TABLE and table in their mssql database, which will cause us problems.... So maybe it's not realistic to do that?

With your patch to aliases, were you able to get it working?

nadilas commented 6 years ago

@aarondl Thanks for that. 😄

To me having two identically named tables table and TABLE makes no sense, as no developer without context could interpret what is what and why are there two versions of the same table. I agree it is technically possible, however I'd see it as an edge-case where close alignment between database engineers and developers is required to make sense of that db setup. I would even argue that in the end those tables would be renamed for everyone's sake.

So the quick-fix I applied was able to get me as far as generating clean named fields and methods. https://gist.github.com/nadilas/b14476f8bce00e7c689d7c5b90798909

However the compiler chokes on having fields and relationship methods named identically. type StateChange has both field and method named ObjType I'm not exactly sure what the best approach would be: either add a simple suffix in the templates and code usage for the relationship methods or simply don't generate the field in the first place?

aarondl commented 6 years ago

I'll definitely do something for the lowercase bit, I think you're right about the table TABLE thing it just feels sad because SOMEONE out there is going to have it as a valid use case haha. Before implementing the fixes I want to see if there's anymore bugs you have first so we can nail them all in the next rc. I'm also going to re-open this so we're not working against a closed issue.

In terms of the identical method/column: it's intentional. sqlboiler really expects your foreign key columns to be named with ObjType_ID or ObjType_UUID or what have you because it's not an ObjType, it's the ID of that thing! :p This naming schema is pretty standard and so we'll continue to do nicer things for people who have databases that look like this. However you can cheat and alias your way out of this by aliasing the column, or by aliasing the table? Personally I would alias the column to ObjTypeID. That -may- present other bugs because I haven't tested much with aliasing columns that are foreign keys, in theory it should work, but if they do exist we'll get them all fixed up for you :)

Thanks for continuing on this btw. I know it sucks going on multi-days not being able to generate, but hang in there! We'll get it fixed up!

nadilas commented 6 years ago

@aarondl No worries ;-) I see the benefit of holding on :-D

So you mean if the column would be named ObjTypeID then the method generated would still just be ObjType(), hence no duplicate? Okay that’s smart. Unfortunately I can’t get around it as multiple services use the database, but I see your point. For now I just suffixed all relationship queries locally with a “Q” for my convenience (templates 05 and 06).

I managed to generate the models with the aliases quick fix and the added suffix.

To help you with hunting down further bugs, here’s one :-) Querying works fine without relationships, as soon as I use qm.Load, the LoadObjType method fails with “char cannot be converted to money”. Neither the ObjTypes table contain char or money types nor the generated structs/queries. Where should I look further? DB or sqlboiler / driver?

Thanks for reopening and your continuing support.

aarondl commented 6 years ago

Oh! No, don't change the templates. If that's necessary we've lost! It's a much better idea to alias the column. If you do that multiple services have no problem (since there's no database change, it's all in sqlboiler). Also the bug you're seeing may be related to a half-change to the templates.

https://github.com/volatiletech/sqlboiler/blob/v3/README.md#aliases

Note the syntax for changing the column name.

[aliases.tables.TABLE.columns]
ObjType = "ObjTypeID"

This should prevent your collision and in the code you simply refer to the column differently than in the database. Should solve all problems but that's where I might expect new bugs :p

nadilas commented 6 years ago

👍 Okay, so I reverted the suffix change and added the aliases. The column alias is not picked up due to the same lowercase issue. I monkey-patched that and it gets generated. Line ~76 & Line ~176

The result is having trouble at the Reload function where the code is referencing the column name not the aliased column name. I'm referring to the 19_reload.go.tpl:38, especially this .Table.PKey.Columns in this line:

Line 38:
    ret, err := Find{{$alias.UpSingular}}({{if not .NoContext}}ctx, {{end -}} exec, {{.Table.PKey.Columns | stringMap .StringFuncs.titleCase | prefixStringSlice "o." | join ", "}})

edit: maybe I missed a few ID fields... let me double-check that... Did doulbe-check. This is the generated:

// Reload refetches the object from the database
// using the primary keys with an executor.
func (o *StateChange) Reload(ctx context.Context, exec boil.ContextExecutor) error {
    ret, err := FindStateChange(ctx, exec, o.ObjType, o.Timestamp, o.UID)
    if err != nil {
        return err
    }

    *o = *ret
    return nil
}

this is what it should be in order to compile:

// Reload refetches the object from the database
// using the primary keys with an executor.
func (o *StateChange) Reload(ctx context.Context, exec boil.ContextExecutor) error {
    ret, err := FindStateChange(ctx, exec, o.ObjTypeID, o.Timestamp, o.UID)
    if err != nil {
        return err
    }

    *o = *ret
    return nil
}
aarondl commented 6 years ago

Oh definitely. I see where you're talking about. It tries to refer to the primary key and ignores aliases. Though it shouldn't be necessary to column alias any primary keys so I'd be surprised if this was breaking after only aliasing foreign keys. Still something I will fix. I'm out right now so I won't be able to commit anything for probably 5 hours. If you can't Jerry rig it to work, maybe create a mini schema I can test with (just the names and parts that cause troubles) and I'll try to get it all working in one fell swoop. We're definitely getting closer to the right solution now though.

nadilas commented 6 years ago

I’ll prepare you a Schema for testing.

aarondl commented 6 years ago

OK awesome. I'll try to deal with it at the tail end of today.

nadilas commented 6 years ago

Here's a schema:

create table dbo.MARS_STATE_OBJ_TYPES
(
    Id int not null
        primary key,
    type nvarchar(15) not null,
    description nvarchar(200) not null
)
go

create table dbo.MARS_STATE_TYPES
(
    Id int not null
        primary key,
    name nvarchar(15) not null,
    descrpition nvarchar(200) not null
)
go

create table dbo.MARS_STATE_CHANGES
(
    uid nvarchar(36) not null,
    objType int not null
        constraint FK_MARS_STATE_CHANGES_MARS_STATE_OBJ_TYPES
            references dbo.MARS_STATE_OBJ_TYPES,
    fromState int not null
        constraint FK_MARS_STATE_CHANGES_MARS_STATE_TYPES_FROM
            references dbo.MARS_STATE_TYPES,
    toState int not null
        constraint FK_MARS_STATE_CHANGES_MARS_STATE_TYPES_TO
            references dbo.MARS_STATE_TYPES,
    oldValue nvarchar(200),
    newValue nvarchar(200),
    timestamp datetime default getdate() not null,
    target nvarchar(70),
    constraint PK__MARS_STA__04FA56DCA3910876
        primary key (uid, objType, timestamp)
)
go

And the .toml:

[aliases.tables.MARS_STATE_CHANGES]
up_plural     = "StateChanges"
up_singular   = "StateChange"
down_plural   = "stateChanges"
down_singular = "stateChange"

[aliases.tables.MARS_STATE_CHANGES.columns]
objtype = "ObjTypeID"
fromstate = "FromStateID"
tostate = "ToStateID"

[aliases.tables.MARS_STATE_OBJ_TYPES]
up_plural     = "StateObjectTypes"
up_singular   = "StateObjectType"
down_plural   = "stateObjectTypes"
down_singular = "stateObjectType"

[aliases.tables.MARS_STATE_TYPES]
up_plural     = "StateTypes"
up_singular   = "StateType"
down_plural   = "stateTypes"
down_singular = "stateType"
aarondl commented 6 years ago

Working on this right now. I'll try to give it a spin and make sure eager loading doesn't break either. I did do some fixes for eager loading just a little while ago so make sure you're on rc7.

aarondl commented 6 years ago

Okay @nadilas. So I've implemented a new syntax for aliases (this is in addition to the current syntax, since that one's a much nicer syntax). This allows us to load any case of identifier for the aliases. This also resolves the issue with TABLE and table because we can alias one or both of them without problems if we want :)

Here's the alternative syntax:

[[aliases.tables]]
name          = "MARS_STATE_CHANGES"
up_plural     = "StateChanges"
up_singular   = "StateChange"
down_plural   = "stateChanges"
down_singular = "stateChange"

  [[aliases.tables.columns]]
  name  = "objType"
  alias = "ObjTypeID"
  [[aliases.tables.columns]]
  name  = "fromState"
  alias = "FromStateID"
  [[aliases.tables.columns]]
  name  = "toState"
  alias = "ToStateID"

[[aliases.tables]]
name          = "MARS_STATE_OBJ_TYPES"
up_plural     = "StateObjectTypes"
up_singular   = "StateObjectType"
down_plural   = "stateObjectTypes"
down_singular = "stateObjectType"

[[aliases.tables]]
name          = "MARS_STATE_TYPES"
up_plural     = "StateTypes"
up_singular   = "StateType"
down_plural   = "stateTypes"
down_singular = "stateType"

There was several places in the templates that made similar assumptions as Reload did with regards to Primary Keys only. So we're lucky you tried to alias a primary key so we could see it blow up. These bugs have been corrected and with this sample schema I can compile both the generated code and generated tests.

There's two problems that remain:

1) Right now in the mssql driver, the way you get rid of foreign keys to allow the generated tests to run is by supplying a table_schema.sql file inside the models folder before running go test. This is basically just a dump of the schema because sqlcmd has no way for us to retrieve the schema for mssql. The other drivers simply call pg_dump or what have you and get the schema back in a predictable format. The problem here being your schema used inline foreign key notation and the mssql driver expects the ALTER TABLE syntax, I had to change the schema to use the following format:

ALTER TABLE dbo.MARS_STATE_CHANGES ADD CONSTRAINT FK_MARS_STATE_CHANGES_MARS_STATE_OBJ_TYPES FOREIGN KEY (objType) REFERENCES dbo.MARS_STATE_OBJ_TYPES
go
ALTER TABLE dbo.MARS_STATE_CHANGES ADD CONSTRAINT FK_MARS_STATE_CHANGES_MARS_STATE_TYPES_FROM FOREIGN KEY (fromState) REFERENCES dbo.MARS_STATE_TYPES
go
ALTER TABLE dbo.MARS_STATE_CHANGES ADD CONSTRAINT FK_MARS_STATE_CHANGES_MARS_STATE_TYPES_TO FOREIGN KEY (toState) REFERENCES dbo.MARS_STATE_OBJ_TYPES
go

Only then was I able to run the tests.

2) When I ran the tests, I got some unexpected problems with the MARS_STATE_CHANGES table. I suspect this is because it uses a timestamp as part of it's primary key and there's some kind of problem happening there. The following tests are failing:

    --- FAIL: TestDelete/StateChanges (0.01s)
    --- FAIL: TestSliceDeleteAll/StateChanges (0.00s)
    --- FAIL: TestExists/StateChanges (0.00s)
    --- FAIL: TestToOneSet/StateChangeToStateObjectTypeUsingObjTypeMARSSTATECHANGESS (0.00s)
    --- FAIL: TestToOneSet/StateChangeToStateTypeUsingFromStateMARSSTATECHANGESS (0.00s)
    --- FAIL: TestToManyAdd/StateObjectTypeToObjTypeMARSSTATECHANGESS (0.00s)
    --- FAIL: TestToManyAdd/StateTypeToToStateMARSSTATECHANGESS (0.00s)
    --- FAIL: TestReload/StateChanges (0.00s)
    --- FAIL: TestUpdate/StateChanges (0.00s)
    --- FAIL: TestSliceUpdateAll/StateChanges (0.00s)
    --- FAIL: TestUpsert/StateChanges (0.01s)

I haven't dug into exactly why, but I have reason to believe this problem would prevent you from safely using sqlboiler since it seems the queries that we're making with the timestamp primary key are failing somehow. It's pretty obvious that that's what's happening if you look at the delete test which is as simple as it is. It creates a struct, randomizes it, and then attempts to delete it. The delete fails and returns "0 rows affected", because somehow mssql isn't able to match the row which is why I think it's so obvious what's going on.

We'll have to look into this but I'm tapped out for today. v3.0.0-rc8 should be pretty close to what we want. There was one more issue that I pushed to v3 after I pushed that tag, but it shouldn't really affect you, it was just nicer to have it (missed one more aliasing place, but it was inconsequential). I'm closing this issue again and I've opened this one: https://github.com/volatiletech/sqlboiler/issues/317 if there's anything you know about why this might be happening let me know.

Re-open this if there's any more problems relating to the aliases.

nadilas commented 6 years ago

Looks great! I'll perform some tests on my end and get back to you.

nadilas commented 6 years ago

The "conversion" issue is still present. The full error message: models: failed to assign all query results to TableAliasName slice: mssql: Cannot convert a char value to money. The char value has incorrect syntax. This is being returned on line 131 from queries\reflect.go. Called from:

func (q tableAliasNameQuery) All(ctx context.Context, exec boil.ContextExecutor) (TableAliasNameSlice, error) {
    var o []*TableAliasName

    err := q.Bind(ctx, exec, &o)
    if err != nil {
        return nil, errors.Wrap(err, "models: failed to assign all query results to TableAliasName slice")
    }

    return o, nil
}

via the following code:

res, err := models.TableAliasNames(Where("fieldName = ?", inputData),
        Load("RelationShipFieldName"),
        Load("RelationShipFieldName.NestedRelationshipFieldName")).All(ctx, db)

I reeeally hope I misunderstood the concept of qm.Load. 😃

aarondl commented 6 years ago

I'm quite busy today so won't be able to look into anything until a bit later. But just make sure you're calling Load with a relationship name. That's to say the names on the relationship structs. Not a column name.

So if I have a Users and Videos table where videos has a fk to Users, I'll be calling Videos(Load("User")), or Users(Load("Videos")), note the plurality. Relationship aliases change the names of these too. In general the names on the relationship struct of the model is the name you want. Double check that for me. I have a feeling this might just be a decimal issue.

nadilas commented 6 years ago

You mean the name of the TableR struct field I need, right?

continued on https://github.com/volatiletech/sqlboiler/issues/319...