Dzoukr / Dapper.FSharp

Lightweight F# extension for StackOverflow Dapper with support for MSSQL, MySQL, PostgreSQL, and SQLite
MIT License
374 stars 35 forks source link

Exclude columns for insert / update #29

Closed JordanMarr closed 3 years ago

JordanMarr commented 3 years ago

Here's an interesting one!

New Features:

Bug Fixes:

The ability to manually list fields is a must-have feature to enable the workflow of using generated record types that contain all columns. This prevents having to create multiple record variations for inserts/updates.

For example, when inserting a record that includes an identity ProjectId field, exclude can be used to remove only that field:

        insert {
            for p in projectsTable do
            values records
            exclude p.ProjectId
        } |> conn.InsertAsync

For updates, it provides a strongly typed way of updating a subset of fields:

        update {
            for p in personTable do
            set modifiedPerson
            include p.FirstName
            include p.LastName
        } |> conn.UpdateAsync

To facilitate the change, Fields : string list option was added to the insert and update queries and is defaulted to None.

Dzoukr commented 3 years ago

Hi @JordanMarr,

Thanks for the hard job again! However, I am not really sure about this. Let me explain why:

From the very beginning, the philosophy behind this library has been: Simplicity with nearly no knobs (the understandable exception was the configurable name of table & schema). What you use as a type is what you get. Simple as that. I loved your contribution with LinqBuilders because it kept the philosophy with additional type safety = πŸš€ boost for library usability with the same complexity for developers. Still love it and use it from the moment we released v2. But adding more knobs like exclude or include goes straight against this philosophy and can easily lead to adding another stuff like column mappings (because why not?), mixing include and exclude in one query and so on. I don't want this library to become another EF in the terms of how hard is to work with it.

I understand that creating variants of table type can be tedious (and error-prone), but I don't want to break its essential rule mentioned in README (https://github.com/Dzoukr/Dapper.FSharp#how-does-library-works). Maybe having some kind of type generator with variants would be the better solution here.

I see your commits having also an unrelated fix - please can you make a separate PR I can merge quickly before taking final thought on this one?

JordanMarr commented 3 years ago

Sure, no problem on the split! Coming right up...

I totally understand your concern, and I also agree that there should be limits to the new features so that it shouldn't continually spiral out to try to be another EF. Also agree with the existing guidelines of "columns must match" to keep things simple - that is all great. In fact, I wasn't intending to add more feature, but it became quickly evident when I tried doing some real work that these changes seemed useful just to get basic things done.

Dzoukr commented 3 years ago

Thanks, Jordan, and thanks twice for understanding! ❀️ Let's have it here for a while (no need to close PR) and see if we will find it necessary over time.

JordanMarr commented 3 years ago

Well I definitely do need it for my current SQLite project, so I think I will need maintain it as a forked version so that I don’t have it as a loose binary.

Dzoukr commented 3 years ago

Sure. Maybe you can share later the feedback from having such functionality and we can merge it back if no hidden dragons appear. πŸ˜„

Dzoukr commented 3 years ago

Hi @JordanMarr,

after giving it another (fresh πŸ˜„) thought I think such functionality could be hand and since it's for update / insert which allow specifying partial update anyway (using anonymous record, which can easily mess things up), its benefits outweigh the risks (I'll mention both in special readme sections). So let's get this into lib and see what happens. After all, if this will cause many issues, we'll just release a new major version with this as an experimental or so (SemVer ftw!), so let's not push too hard on this. It's free lib anyway. πŸ˜„

Few thoughts:

So I'll merge, do some work on the old version, tests, document, and will release soon-ish as another minor version.

JordanMarr commented 3 years ago

@Dzoukr I’m very glad to hear it!

regarding the include / exclude, that was my first choice, but I think include is reserved. But I am open to whatever names you think that will work.

Dzoukr commented 3 years ago

I'm glad you are glad - let's make it a kickass library! πŸ˜„

I think include is reserved

Oh, didn't know that. So maybe having column is good enough but I somehow feel that there must a slightly better (hopefully not reserved again) name for it. I'll probably check some thesaurus or something.

JordanMarr commented 3 years ago

I thought about maybe a compound like includeColumn / excludeColumn; they have more symmetry at least, although a single word would be better. A consult with the thesaurus might be helpful. πŸ˜…

Dzoukr commented 3 years ago

YES! That was my idea as well. Thesaurus told me nothing indeed because naming is hard and naming in programing is mega hard. πŸ˜„ But I really like the includeColumn / excludeColumn - yes, it's longer, but descriptive. And we can have includeColumns / excludeColumns (for passing list instead of single value)

I think we found it!

JordanMarr commented 3 years ago

Perfect!

Dzoukr commented 3 years ago

Aaaaaaaaaaaaaand published as v2.1.0 with updated docs + you in Nuget metadata + tests + support for (in/ex)cludeColumn in old syntax.

I was not able to correctly create includeColumns [p.Name; p.LastName] so the plural versions are not there yet, but it's not an urgent stuff.

Thanks again! Will add the contribution guide later today.

JordanMarr commented 3 years ago

Regarding multiple includes/excludes, it is possible to do this already:

        update {
            for p in projectsTable do
            set modifiedProject
            excludeColumn p.IsActive
            excludeColumn p.IsEnabled
            excludeColumn p.IsFavorite
            excludeColumn p.ProjectDescription
            where (p.ProjectId = modifiedProject.Id)
        } |> conn.UpdateAsync

But having excludeColumns could also be a nice convenience. We should able to use a tuple which would be very similar to groupBy which can handle a single value or a multiple via a tuple.

        update {
            for p in projectsTable do
            set modifiedProject
            excludeColumns (p.IsActive, p.IsEnabled, p.IsFavorite, p.ProjectDescription)
            where (p.ProjectId = modifiedProject.Id)
        } |> conn.UpdateAsync

(Although I'm not sure if there are any hidden limits on tuple length. hopefully not!)

A list is technically possible, but it would be not very nice to look at:

        excludeColumns (fun p -> [p.IsActive; p.IsEnabled])