weldsorm / welds

BSD 3-Clause "New" or "Revised" License
119 stars 6 forks source link

Add a few more SQL Server types #24

Open delsehi opened 3 months ago

delsehi commented 3 months ago

I tried fetching definitions from my database and generating the models, but a lot of our real-world use cases couldn't be used so I thought I could add some of them.

A few more are still missing, like TIME and DATETIMEOFFSET.

Also not quite certain about NUMERIC and DECIMAL

I could possibly try adding something like this to the integration tests if I can get them running

create table x.tricky_types(
    table1 decimal(2,1),
    table2 decimal,
    table3 numeric,
    table4 numeric(5),
    table5 numeric(5,3),
    table6 TIME,
    table7 NTEXT,
    table8 TEXT,
    table9 REAL,
    table10 FLOAT(2),
    table11 FLOAT,
    table12 CHAR
)
lex148 commented 3 months ago

Great work @delsehi

Yeah, the integration tests are a little finicky to get working. Especially the mssql ones. The basic unit test can be ran from just the welds/welds directory with a cargo test. The database specific integration tests need to be ran from a Linux box with docker installed so they can test against a real DB.

The MSSQL tests are even worse. The migration tests can't be ran in parallel for MSSQL, so they need

cd tests
cd mssql
cargo test -- --test-threads=1

Your types look great, but I have a couple thoughts for you.

For recommended_db_type I thought the recommended MSSQL type for datetimes was DATETIME2. Both DATETIME2 and DATETIME should map just fine to a chrono::Datetime, just want to make the best recommendation as to what type we suggest using in the DB line:448 in your Merge request

You asked about NUMERIC and DECIMAL. I know tiberius will map these to rust_decimal::Decimal. This requires adding tiberius to your cargo.toml with the feature rust_decimal added. I would suggest going with these.

Really appreciate your help here. Let me know what you think.