Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insert Select With Conflict for postgres #155

Merged
merged 10 commits into from
Oct 28, 2019

Conversation

JoseD92
Copy link
Contributor

@JoseD92 JoseD92 commented Oct 22, 2019

Based on #86 I came with insertSelectWithConflict, it is like insertSelect but allows to perform an update if some row conflicts with the new addition on a given constraint. There is no way in postgres (that I could find) of responding to any possible conflict, so a specific one needs to be indicated with a unique.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of requiring users to provide the undefined values - I'd much rather do that automagically for them.

Overall I like this quite a lot, but I want to think on the API a bit more before merging.

entCurrent = EEntity $ I (tableName proxy)
-- there must be a better way to get the constrain name from a unique, make this not a list search
uniqueDef = head . filter ((==) (persistUniqueToFieldNames unique) . uniqueFields) . entityUniques . entityDef $ proxy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

head . filter p === find p

It would be nice to have a function toUniqueDef :: PersistEntity record => Unique record -> UniqueDef, but this is basically how I'd implement it 🤷‍♂️

Copy link
Contributor Author

@JoseD92 JoseD92 Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart guy, I always forget find exists, but it keeps being a list search, but I think it should not be too difficult to add toUniqueDef on persist, though I will have to look at the code to be sure.

Also, the thing with the undefined, I would also like it to be automatic, but the Unique creation function may have any amount of parameters, when defining a unique in persist we can:

...
Foo
    num1 Int
    num2 Int
    num3 Int
    UniqueFooNum1 num1
    UniqueFooNum23 num2 num3
    deriving Eq Show
...

so I can't just change the firm to (a -> Unique val) -> SqlQuery (SqlExpr (Insertion val)) -> (SqlExpr (Entity val) -> SqlExpr (Entity val) -> [SqlExpr (Update val)]) -> SqlWriteT m Int64 because I would not be able to use UniqueFooNum23 on this new function, if there is a way receive a variable parameter function and then fill all parameters with undefined I will add it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I linked to KnowResult in another comment. You can implement a type class that will "fill in" the unused values with undefined also. It's a bit of a trick of type class programming so if you'd like help on that please let me know :)

insertSelectWithConflict :: (MonadIO m, PersistEntity val) =>
Unique val
-- ^ Uniqueness constraint, this is used just to get the name of the postgres constraint, the value(s) is(are) never used, so if you have a unique "MyUnique 0", "MyUnique undefined" will work as well.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used a trick like this before that matches on the final result type of a value - see KnowResult here which allows you to only provide the data constructor. With another type class, you can even pass in the required undefineds automatically so that the user is not concerned with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! nice recommendation, I have not use type families much, but seeing KnowResult did help me avoid the undefined, though I think I should move KnowResult to another place (I copy KnowResult to Database.Esqueleto.PostgreSQL), any recommendations on where to place it and if it is possible to make the firm for insertSelectWithConflict prettier?

let test = map (OneUnique "test" . personFavNum) [p1,p2,p3]
test2 = map (OneUnique "test" . (+4) . (*2) . personFavNum) [p1,p2,p3]
liftIO $ map entityVal uniques1 `shouldBe` test
liftIO $ map entityVal uniques2 `shouldBe` test2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks for the tests!

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job! I don't mind things being where they are.

@bitemyapp thoughts on this? I'm in favor of inclusion.

finalR = id

instance (FinalResult b) => FinalResult (a -> b) where
finalR f = finalR (f undefined)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!!

@parsonsmatt
Copy link
Collaborator

parsonsmatt commented Oct 28, 2019

Is this still WIP? If it's ready to merge, please add a Changelog entry and update the version in the .cabal file.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to merge this for a 3.1.3 release. Please accept the suggestions to the PR (or reword if you'd like), add the changelog entry, and update the version in the Cabal file.

src/Database/Esqueleto/PostgreSQL.hs Outdated Show resolved Hide resolved
src/Database/Esqueleto/PostgreSQL.hs Outdated Show resolved Hide resolved
src/Database/Esqueleto/PostgreSQL.hs Show resolved Hide resolved
src/Database/Esqueleto/PostgreSQL.hs Show resolved Hide resolved
Copy link
Owner

@bitemyapp bitemyapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval pending the points @parsonsmatt highlighted

JoseD92 and others added 5 commits October 28, 2019 11:49
Co-Authored-By: Matt Parsons <parsonsmatt@gmail.com>
Co-Authored-By: Matt Parsons <parsonsmatt@gmail.com>
Co-Authored-By: Matt Parsons <parsonsmatt@gmail.com>
Co-Authored-By: Matt Parsons <parsonsmatt@gmail.com>
@JoseD92 JoseD92 changed the title WIP: Insert Select With Conflict for postgres Insert Select With Conflict for postgres Oct 28, 2019
@parsonsmatt
Copy link
Collaborator

Once CI is green I'll merge. Thanks @JoseD92 !!

@parsonsmatt parsonsmatt merged commit 40f7a0c into bitemyapp:master Oct 28, 2019
@parsonsmatt
Copy link
Collaborator

3.1.3 is released. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants