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

Implement inherits for Schema.sql #1998

Draft
wants to merge 66 commits into
base: master
Choose a base branch
from
Draft

Implement inherits for Schema.sql #1998

wants to merge 66 commits into from

Conversation

amitaibu
Copy link
Collaborator

@amitaibu amitaibu commented Aug 13, 2024

  • Parse inherits
  • Schema compile should allow setting parent fields
  • Allow saving revisions with triggers
  • Mark on UI when we use inherit
    image
  • Support migrate. If parent field was changed, we should adapt also inherits.
  • Inherited fields should copy constraints? Maybe not, since it saves data the was added. So if it passed a constraint some time in the past, it should remain.
  • Inherited fields should not have unique? Since a revision may hold the same "unique" values on several records
  • Update docs
  • Add tests

CONTRIBUTING.md Outdated
@@ -82,7 +82,7 @@ use `make console` to load your application together with the framework located

```
ghci
:l IHP/exe/IHP/IDE/DevServer.hs
:l IHP/ihp-ide/exe/IHP/IDE/DevServer.hs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mpscholten I think the doc is outdated.

  1. Is make console, mentioned above , needed?
  2. I'm getting an error
    image

Copy link
Member

Choose a reason for hiding this comment

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

It could just be ghci. make console is just an alias for ghci.

Can you try :set -iIHP/ihp-ide in the ghci? This will add the IHP/ihp-ide directory to the load path. After that it should load without the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, it helped (adapted docs)

But now I get

image

Copy link
Member

@mpscholten mpscholten Aug 13, 2024

Choose a reason for hiding this comment

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

I can reproduce that. Just tried it locally and here's an approach how I got it working with latest IHP:

# Assuming workingDirectory is the project

cd IHP # switch to IHP
direnv allow # load direnv things if this is freshly cloned

ghci # Load ghci will all IHP dependencies
:cd .. # switch back to project directory
:set -iIHP
:set -iIHP/ihp-ide
 :l IHP/ihp-ide/exe/IHP/IDE/DevServer.hs

That should work now

locally i run into a ghc bug while doing that, I hope it works for you:

ghci> :l IHP/ihp-ide/exe/IHP/IDE/DevServer.hs
Ok, 164 modules loaded.
ghci> main
panic! (the 'impossible' happened)
  GHC version 9.8.2:
	nameModule
  internal $fHasQueryBuilderkJoinQueryBuilderWrapperjoinRegister1_iefl
  Call stack:
      CallStack (from HasCallStack):
        callStackDoc, called at compiler/GHC/Utils/Panic.hs:191:37 in ghc-9.8.2-inplace:GHC.Utils.Panic
        pprPanic, called at compiler/GHC/Types/Name.hs:341:3 in ghc-9.8.2-inplace:GHC.Types.Name
  CallStack (from HasCallStack):
    panic, called at compiler/GHC/Utils/Error.hs:503:29 in ghc-9.8.2-inplace:GHC.Utils.Error


Please report this as a GHC bug:  https://www.haskell.org/ghc/reportabug

Copy link
Member

Choose a reason for hiding this comment

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

As a workaround for the GHC bug I reverted the commit that introduced the bug by running git revert 4fe8b56e5a8c5dcb394a16b0dfb5eb2d6f77b18f from within the IHP directory

Copy link
Member

Choose a reason for hiding this comment

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

I've ran into more issues with the dev server right now and added a new helper called mainInParentDirectory to fix that.

After you've synced this branch with latest master it should now work like this:

# Assuming workingDirectory is the project

cd IHP # switch to IHP
direnv allow # load direnv things if this is freshly cloned

ghci # Load ghci will all IHP dependencies
 :l ihp-ide/exe/IHP/IDE/DevServer.hs
mainInParentDirectory # Will run the IHP project in the parent project directory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for looking into it 🙏

I'm confused about which URL should be in my flake.nix

ihp/CONTRIBUTING.md

Lines 43 to 67 in 27a696a

### Nix Changes
If you're testing local nix changes, you need to change your `flake.nix` to point to the IHP project instead of pulling it from github:
```nix
{
inputs = {
# The path needs to be absolute
ihp.url = "path:///Users/myuser/someproject/IHP";
# ...
};
```
After that run `nix flake update`.
### Running the latest IHP `master`
When contributing to IHP core you will want to have your PRs synced with `master`. Your `flake.nix` should have this line:
```nix
{
ihp.url = "github:digitallyinduced/ihp";
}
```

I'm guessing we should always tell folks to point to the local IHP, or is there a different use case?

@amitaibu
Copy link
Collaborator Author

Failing tests belong to https://github.com/digitallyinduced/ihp/blob/27a696af90efcd0f84260d84805dd2dc753e818c/Test/SchemaCompilerSpec.hs that require some trailing spaces. Let's see if we can remove those :)

@amitaibu
Copy link
Collaborator Author

amitaibu commented Aug 14, 2024

Next, I want to see if we can have revisions created either manually or by using Postgress triggers.

Manual will be something like this:

CREATE TABLE post_revisions (
    revision_id UUID DEFAULT uuid_generate_v4() PRIMARY KEY NOT NULL,
    post_id UUID NOT NULL
) INHERITS (posts);
action CreatePostRevisionAction { .. } = do
    post <- fetch postId
    let revision = newRecord @PostRevision
            |> set #postId post.id
            |> set #title post.title
            |> set #body post.body
    createRecord revision

    redirectTo ShowPostAction { .. }

@amitaibu
Copy link
Collaborator Author

The INHERTIS keyword is now recognized in Schema.sql. Next is figuring how to auto-generate types/getter/setter based on the columns from the parent table

image

@amitaibu
Copy link
Collaborator Author

@mpscholten I've started working on the SchemaCompiler, but I've hit a panic

image

What are you doing in such cases?

@mpscholten
Copy link
Member

try git revert 4fe8b56e5a8c5dcb394a16b0dfb5eb2d6f77b18f (that commit causes the GHC panic). After that restart the ghc and it should work

@amitaibu
Copy link
Collaborator Author

try git revert 4fe8b56

Thanks. How did you know it was that commit? So I'll know for next time 😄

@mpscholten
Copy link
Member

When this specific bug happend the first time I was looking through the git history and just tried out various commits until I found the one that reproduced it. This was like a week or two weeks after that commit was created, so it was only a few commits I had to try out

@amitaibu
Copy link
Collaborator Author

Got it to compile with this code! 🎈

@amitaibu
Copy link
Collaborator Author

Next is adapting instance CanCreate and instance CanUpdate

@amitaibu
Copy link
Collaborator Author

I've updated the OP with a checklist. I'm not sure about some of the stuff, as I think it's not a small feature 😄

@mpscholten
Copy link
Member

Happy to help if there's more questions :)

@amitaibu
Copy link
Collaborator Author

amitaibu commented Aug 27, 2024

Using triggers

CREATE TABLE posts (
    id UUID DEFAULT uuid_generate_v4() PRIMARY KEY NOT NULL,
    title TEXT NOT NULL,
    body TEXT NOT NULL,
    user_id UUID NOT NULL
);

CREATE TABLE post_revisions (
    id UUID DEFAULT uuid_generate_v4() PRIMARY KEY NOT NULL,
    post_id UUID NOT NULL
) INHERITS (posts);

CREATE OR REPLACE FUNCTION create_post_revision() RETURNS TRIGGER AS $$
BEGIN
    INSERT INTO post_revisions (id, post_id, title, body, user_id)
    ### Notice we use `ihp_user_id()` user here to capture the acting user.
    VALUES (uuid_generate_v4(), NEW.id, NEW.title, NEW.body, ihp_user_id());
    RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER post_revision_trigger
AFTER UPDATE ON posts
FOR EACH ROW
EXECUTE FUNCTION create_post_revision();

I've logged in, and when I update a Post I get

image

What should I do in order to get ihp_user_id() working?

@mpscholten
Copy link
Member

The ihp_user_id() function can only be used together with row level security (which requires some setup described here https://ihp.digitallyinduced.com/Guide/realtime-spas.html ).

In our case we can just use NEW.user_id instead, so:

-- old
VALUES (uuid_generate_v4(), NEW.id, NEW.title, NEW.body, ihp_user_id());

-- new
VALUES (uuid_generate_v4(), NEW.id, NEW.title, NEW.body, NEW.user_id);

@amitaibu
Copy link
Collaborator Author

Thanks.

NEW.user_id

Right. I guess I wanted to have another column called revision_user_id and have that populated with the ihp_user_id().

Is there a technical reason not to inject the user ID also if row level security isn't used?

@mpscholten
Copy link
Member

mpscholten commented Aug 27, 2024

Is there a technical reason not to inject the user ID also if row level security isn't used?

Yes, right now the ihp_user_id() requires a second database role to be available. You can see some code here https://github.com/digitallyinduced/ihp/blob/master/IHP/ModelSupport.hs#L437 In theory we could also support ihp_user_id() without that second database role, but that would require some rework of the RLS code.

Initially ihp_user_id() was designed to be always used with Row level security. So we need to refactor it a bit to make it work independently.

@amitaibu
Copy link
Collaborator Author

amitaibu commented Sep 2, 2024

Indicate inherits on the UI

image

@amitaibu
Copy link
Collaborator Author

I'm trying to see how migration works. I'm assuming inherits isn't enough to update columns, in case the parent columns have changed.

So I've added a new col to the parent table.

image

But on migrate db I get this error

image

Server is running via the new mainInParentDirectory on ghci

@mpscholten
Copy link
Member

Could it be that the postgres server is not running? If that's the case, you could manually start one by running make postgres in a different tab

@amitaibu
Copy link
Collaborator Author

amitaibu commented Sep 13, 2024 via email

@mpscholten
Copy link
Member

If you run the pg_dump command from the error message in a terminal yourself, does it output some SQL?

@amitaibu
Copy link
Collaborator Author

➜  IHP git:(core-inherits) ✗ pg_dump
pg_dump: [archiver (db)] connection to database "amitaibu" failed: connection to server on socket "/var/run/postgresql/.s.PGSQL.5432" failed: FATAL:  role "amitaibu" does not exist

But it's running on ghci, and the IDE & app are running

image

image

@amitaibu
Copy link
Collaborator Author

To workaround the error, I'm checking it by running devenv up, and then the migration doesn't error due to missing connection

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.

2 participants