Dzoukr / Dapper.FSharp

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

Multi join / constant expression fix #63

Closed JordanMarr closed 2 years ago

JordanMarr commented 2 years ago

Creating this as a draft so you can poke at it. So far I've added a DU to allow visitJoin to return a JoinInfo list to allow for either columns or constant values. (Will clean up and worry about names later.)

I think the other side of this is the Join DU having an abstraction to handle either. Feel free to mess with that part if you want. It's bed time here. 😴

Dzoukr commented 2 years ago

Will have a look, thanks for all the work, man!

Dzoukr commented 2 years ago

Man, it seems we just did the same thing? :)

Dzoukr commented 2 years ago

Oh, I pushed it in - https://github.com/Dzoukr/Dapper.FSharp/commit/1840e54c685a74980862d9c4a47195e4ff4519ed

JordanMarr commented 2 years ago

fuck!

Dzoukr commented 2 years ago

I didn't want to you do all the hard job again so said to myself "I gonna fix it" and now you did the same 🤣

JordanMarr commented 2 years ago

It's cool, I was playing Overwatch and my brain was like, "this will be easy, just do this..." haha

No worries, I was coding super fast and didn't spend very long it. Please just overwrite with what you already did so it can be a joint effort. 😀

And btw that was all my miscommunication. I pretty much asked you to finish it, so I should have checked with you first to see if you had started.

Dzoukr commented 2 years ago

Now I am confused... so should we create a new feature branch or should I just update it in master?

JordanMarr commented 2 years ago

Maybe you could pull the changes I just pushed and do a merge and just keep your changes across the board? Or it might be easier if I revert my changes to last night and then push. Then your merge will be super easy.

Dzoukr commented 2 years ago

Btw... it seems we are going a slightly different way. My idea was to simplify to have both Joins with list by default to drop "OnMany" cases but I may be wrong here...

JordanMarr commented 2 years ago

Interesting. I think that's a better idea!

This means i can play another round of Overwatch while you fix the merge mess. 😂

Dzoukr commented 2 years ago

Merged... Somehow... Now we just need to finish the conversion from obj to correct SQL syntax. I'll try to look at it and you'll keep kicking asses in Overwatch 😄

Dzoukr commented 2 years ago

To make it really safe, I need to go over adding new list of params just for joins - rabbit holeeeeeeee 😄

JordanMarr commented 2 years ago

To make it really safe, I need to go over adding new list of params just for joins - rabbit holeeeeeeee So true.

To make it really safe, I need to go over adding new list of params just for joins - rabbit holeeeeeeee 😄

Yeah, parametrizing it is really going to be the hardest part but definitely safer that way. 🐰

Dzoukr commented 2 years ago

It's getting a little bit hacky now... I mean it works, but:

This is the only version that works:

let thirdPerson = persons.[2]
let thirdDog = dogs.[2]
let tpi = thirdPerson.Id

select {
    for p in personsView do
    innerJoin d in dogsView on ((p.Id, tpi) = (d.OwnerId,d.OwnerId))
    selectAll
} |> crud.SelectAsync<Persons.View, Dogs.View>

This will evaluate as not constant:

select {
    for p in personsView do
    innerJoin d in dogsView on ((p.Id, thirdPerson.Id) = (d.OwnerId,d.OwnerId))
    selectAll
} |> crud.SelectAsync<Persons.View, Dogs.View>

This will return all values (not filtered), but I assume this is SQL default (works same from direct SQL console)

select {
    for p in personsView do
    innerJoin d in dogsView on (tpi= d.OwnerId) // joins also other values
    selectAll
} |> crud.SelectAsync<Persons.View, Dogs.View>
Dzoukr commented 2 years ago

The last sample is quite ok-ish, because if this is default SQL behavior then it's only about the library user's SQL knowledge.

BUT the fact, that I MUST to assign property to constant value before using it in join, that's somehow bothers me. The question is if we can avoid it. 🤔

Dzoukr commented 2 years ago

Published in v3.3. Thanks for all the help, mate!