Add more support for non-id primary keys #1925
Conversation
The easiest way here might be to create a test IHP app with some CRUD controllers and then load the framework into the app. Check out https://github.com/digitallyinduced/ihp/blob/master/CONTRIBUTING.md#running-an-application-against-a-local-copy-of-the-framework |
Replace primaryKeyCondition with a version that takes the id instead of the whole record
|
Okay, but there's nothing that i could commit to prevent regressions, right? |
This way it supports tables with arbitrary primary keys
MonaMayrhofer
force-pushed
the
support_non_id_pks
branch
from
915a1f7
to
ad97647
Compare
|
I can't find any information about formatting guidelines, and the repository doesn't seem to match ormolu or fourmolu's formatting... is there an official formatter / formatting config, or shall I format stuff by hand to match the rest as good as possible? |
|
There's no official formatting config, so you can format how it fits best 👍 |
I am tempted to leave these two for a seperate PR in the future, as this one is already getting quite big and convoluted... The first one should be an easy fix, but it doesn't really fit the spirit of this PR as its in the IDE, so it belongs in a seperate PR imo... And the last two are a) a rather weird case, which shouldn't come up all too often (a many-to-many relation with unique properties which itself has many of something else...), and b) also doesn't really fit the spirit of this PR as they are more dealing with foreign keys... |
👍 |
|
I think as soon as these tests run through this is pretty much done and ready for review :D |
|
Oh whoops, that wasn't what I meant to do |
MonaMayrhofer
force-pushed
the
support_non_id_pks
branch
from
21822a1
to
0bb5b45
Compare
|
That's looking better^^ Can you take a look if #dc62630 has addressed your concerns? |
Looks great, thanks :) |
IHP/ModelSupport.hs
Outdated
| pure () | ||
| deleteRecordByIds :: forall record table. (?modelContext :: ModelContext, Show (PrimaryKey table), Table record, GetTableName record ~ table, record ~ GetModelByTableName table) => [Id' table] -> IO () | ||
| deleteRecordByIds [] = do | ||
| pure () -- If there are no ids, we wouldn't even know the pkCols, so we just don't do anything, as nothing happens anyways |
There was a problem hiding this comment.
This comment is outdated, we can know the pkCols now that we have primaryKeyConditionColumnSelector. I'll bring this up to date again
|
It's done! Please have a look at the new changes :D |
|
Thanks 👍 great work here! |
|
@MonaMayrhofer well done 🥳 |
These are my efforts to tackle the problems described in #1924.
This PR is still a work in progress.
Goals:
SchemaCompilerOther stuff that has to be done to truly support non-id pks, but isn't part of this PR, include (among others?):
more goals may get added when i stumble over something that doesnt work with weird primary keys.
Open questions:
- Is there currently any simple way to test database code, like (deleteRecord) either via mocking or using a testing postgres instance?